Fix a deadlock in event/command interaction
Vinit Deshpande [Thu, 31 Jul 2014 18:05:14 +0000 (11:05 -0700)]
There are two locks, one in HAL which takes care of adding/removing event
handlers from a common data structure; and another in the Framework
which synchronizes access to WifiNative. Commands and events grab these
locks in opposite order, and hence can cause deadlock.

This change fixes the deadlock by holding on to the HAL lock only for
a short time. This is achieved by refcounting the commands so they
don't have to be destroyed while holding locks.

Bug: 16660861

Change-Id: I35f9f688c8c1ddb2116b228c52e0bb1125df0568

bcmdhd/wifi_hal/common.cpp
bcmdhd/wifi_hal/cpp_bindings.h
bcmdhd/wifi_hal/gscan.cpp
bcmdhd/wifi_hal/rtt.cpp
bcmdhd/wifi_hal/wifi_hal.cpp

index 9937bfb..7d517d7 100644 (file)
@@ -53,7 +53,7 @@ wifi_error wifi_register_handler(wifi_handle handle, int cmd, nl_recvmsg_msg_cb_
         info->event_cb[info->num_event_cb].cb_func = func;
         info->event_cb[info->num_event_cb].cb_arg  = arg;
         info->num_event_cb++;
-        ALOGI("Successfully added event handler %p for command %d", func, cmd);
+        ALOGI("Successfully added event handler %p:%p for command %d", arg, func, cmd);
         result = WIFI_SUCCESS;
     }
 
@@ -78,7 +78,8 @@ wifi_error wifi_register_vendor_handler(wifi_handle handle,
         info->event_cb[info->num_event_cb].cb_func = func;
         info->event_cb[info->num_event_cb].cb_arg  = arg;
         info->num_event_cb++;
-        ALOGI("Added event handler %p for vendor 0x%0x and subcmd 0x%0x", func, id, subcmd);
+        ALOGI("Added event handler %p:%p for vendor 0x%0x and subcmd 0x%0x",
+                arg, func, id, subcmd);
         result = WIFI_SUCCESS;
     }
 
@@ -99,10 +100,12 @@ void wifi_unregister_handler(wifi_handle handle, int cmd)
 
     for (int i = 0; i < info->num_event_cb; i++) {
         if (info->event_cb[i].nl_cmd == cmd) {
+            ALOGI("Successfully removed event handler %p:%p for cmd = 0x%0x",
+                    info->event_cb[i].cb_arg, info->event_cb[i].cb_func, cmd);
+
             memmove(&info->event_cb[i], &info->event_cb[i+1],
                 (info->num_event_cb - i) * sizeof(cb_info));
             info->num_event_cb--;
-            ALOGI("Successfully removed event handler for command %d", cmd);
             break;
         }
     }
@@ -121,11 +124,11 @@ void wifi_unregister_vendor_handler(wifi_handle handle, uint32_t id, int subcmd)
         if (info->event_cb[i].nl_cmd == NL80211_CMD_VENDOR
                 && info->event_cb[i].vendor_id == id
                 && info->event_cb[i].vendor_subcmd == subcmd) {
-
+            ALOGI("Successfully removed event handler %p:%p for vendor 0x%0x, subcmd = 0x%0x",
+                    info->event_cb[i].cb_arg, info->event_cb[i].cb_func, id, subcmd);
             memmove(&info->event_cb[i], &info->event_cb[i+1],
                 (info->num_event_cb - i) * sizeof(cb_info));
             info->num_event_cb--;
-            ALOGI("Successfully removed event handler for vendor 0x%0x", id);
             break;
         }
     }
index 715aa91..491c398 100644 (file)
@@ -218,9 +218,10 @@ protected:
     Condition mCondition;
     wifi_request_id mId;
     interface_info *mIfaceInfo;
+    int mRefs;
 public:
     WifiCommand(wifi_handle handle, wifi_request_id id)
-            : mMsg(getHalInfo(handle)->nl80211_family_id), mId(id)
+            : mMsg(getHalInfo(handle)->nl80211_family_id), mId(id), mRefs(1)
     {
         mIfaceInfo = NULL;
         mInfo = getHalInfo(handle);
@@ -228,7 +229,7 @@ public:
     }
 
     WifiCommand(wifi_interface_handle iface, wifi_request_id id)
-            : mMsg(getHalInfo(iface)->nl80211_family_id, getIfaceInfo(iface)->id), mId(id)
+            : mMsg(getHalInfo(iface)->nl80211_family_id, getIfaceInfo(iface)->id), mId(id), mRefs(1)
     {
         mIfaceInfo = getIfaceInfo(iface);
         mInfo = getHalInfo(iface);
@@ -243,6 +244,20 @@ public:
         return mId;
     }
 
+    virtual void addRef() {
+        int refs = __sync_add_and_fetch(&mRefs, 1);
+        // ALOGD("addRef: WifiCommand %p has %d references", this, refs);
+    }
+
+    virtual void releaseRef() {
+        int refs = __sync_sub_and_fetch(&mRefs, 1);
+        if (refs == 0) {
+            delete this;
+        } else {
+            // ALOGD("releaseRef: WifiCommand %p has %d references", this, refs);
+        }
+    }
+
     virtual int create() {
         /* by default there is no way to cancel */
         ALOGD("WifiCommand %p can't be created", this);
index a10f0de..61bef2f 100644 (file)
@@ -711,7 +711,7 @@ wifi_error wifi_stop_gscan(wifi_request_id id, wifi_interface_handle iface)
 
         ScanCommand *cmd = new ScanCommand(iface, id, &dummy_params, handler);
         cmd->cancel();
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
@@ -719,7 +719,7 @@ wifi_error wifi_stop_gscan(wifi_request_id id, wifi_interface_handle iface)
     WifiCommand *cmd = wifi_unregister_cmd(handle, id);
     if (cmd) {
         cmd->cancel();
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
@@ -755,14 +755,14 @@ wifi_error wifi_disable_full_scan_results(wifi_request_id id, wifi_interface_han
         memset(&handler, 0, sizeof(handler));
         FullScanResultsCommand *cmd = new FullScanResultsCommand(iface, 0, &params_dummy, handler);
         cmd->cancel();
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
     WifiCommand *cmd = wifi_unregister_cmd(handle, id);
     if (cmd) {
         cmd->cancel();
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
@@ -1091,7 +1091,7 @@ wifi_error wifi_reset_bssid_hotlist(wifi_request_id id, wifi_interface_handle if
     WifiCommand *cmd = wifi_unregister_cmd(handle, id);
     if (cmd) {
         cmd->cancel();
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
@@ -1311,7 +1311,7 @@ wifi_error wifi_reset_significant_change_handler(wifi_request_id id, wifi_interf
     WifiCommand *cmd = wifi_unregister_cmd(handle, id);
     if (cmd) {
         cmd->cancel();
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
index 934a295..1673aca 100644 (file)
@@ -300,12 +300,13 @@ wifi_error wifi_rtt_range_cancel(wifi_request_id id,  wifi_interface_handle ifac
     RttCommand *cmd = (RttCommand *)wifi_unregister_cmd(handle, id);
     if (cmd) {
         cmd->cancel_specific(num_devices, addr);
-        delete cmd;
+        cmd->releaseRef();
         return WIFI_SUCCESS;
     }
 
     return WIFI_ERROR_INVALID_ARGS;
 }
+
 /* API to get RTT capability */
 wifi_error wifi_get_rtt_capabilities(wifi_interface_handle iface,
         wifi_rtt_capabilities *capabilities)
index 345da3c..a8500be 100644 (file)
@@ -331,13 +331,22 @@ static int internal_valid_message_handler(nl_msg *msg, void *arg)
             }
 
             cb_info *cbi = &(info->event_cb[i]);
-            (*(cbi->cb_func))(msg, cbi->cb_arg);
-            dispatched = true;
-        }
-    }
+            nl_recvmsg_msg_cb_t cb_func = cbi->cb_func;
+            void *cb_arg = cbi->cb_arg;
+            WifiCommand *cmd = (WifiCommand *)cbi->cb_arg;
+            if (cmd != NULL) {
+                cmd->addRef();
+            }
+
+            pthread_mutex_unlock(&info->cb_lock);
 
-    if (!dispatched) {
-        // ALOGI("event ignored!!");
+            (*cb_func)(msg, cb_arg);
+            if (cmd != NULL) {
+                cmd->releaseRef();
+            }
+
+            return NL_OK;
+        }
     }
 
     pthread_mutex_unlock(&info->cb_lock);