[9610] wlbt: Fixed deadlock in mxlogger

The following sequence leads to a deadlock when mxlogger is
deinitialzing and a log collection is triggered:

mxlogger_deinit
---> gets mutex_lock(&mxlogger->lock);
***** preemption *****
__scsc_log_collector_collect_to_file
---> gets mutex_lock(&log_status.log_mutex);
//calls
mxlogger_collect:
---> sitting in already claimed mutex_lock(&mxlogger->lock);
***** preemption *****
mxlogger_deinit
//calls
scsc_log_collector_unregister_client
---> sitting in alreaday claimed mutex_lock(&log_status.log_mutex)
DEADLOCK

To avoid the deadlock condition, the scsc_log_collector_unregister_client
calls are moved outside the mxlogger_deinit lock.

Hip4.c has also been addressed

Client registration / unregistration serialization has also been
addressed

Change-Id: I5a85de46bbd0564c4409ed6049cb9f3a8c987ae1
Signed-off-by: Albert Cano <a.canocamps@samsung.com>

diff --git a/drivers/misc/samsung/scsc/mxlogger.c b/drivers/misc/samsung/scsc/mxlogger.c
index 360b2a59ab4d33c96df6c8740170cc45faead187..ba6390324d4d77c578569bfcbfcbabc9f6e8dcfa 100644 (file)
--- a/drivers/misc/samsung/scsc/mxlogger.c
+++ b/drivers/misc/samsung/scsc/mxlogger.c
@@ -383,10 +383,17 @@ static int mxlogger_collect(struct scsc_log_collector_client *collect_client, si
        if (mxlogger && mxlogger->mx)
                mif = scsc_mx_get_mif_abs(mxlogger->mx);
        else
-               return -EIO;
+               /* Return 0 as 'success' to continue the collection of other chunks */
+               return 0;
 
        mutex_lock(&mxlogger->lock);
 
+       if (mxlogger->initialized == false) {
+               SCSC_TAG_ERR(MXMAN, "MXLOGGER not initialized\n");
+               mutex_unlock(&mxlogger->lock);
+               return 0;
+       }
+
        if (collect_client->type == SCSC_LOG_CHUNK_SYNC)
                i = MXLOGGER_SYNC;
        else if (collect_client->type == SCSC_LOG_CHUNK_IMP)
@@ -419,7 +426,7 @@ static int mxlogger_collect(struct scsc_log_collector_client *collect_client, si
        }
 
        mutex_unlock(&mxlogger->lock);
-       return ret;
+       return 0;
 }
 
 static int mxlogger_collect_end(struct scsc_log_collector_client *collect_client)
@@ -654,6 +661,19 @@ void mxlogger_deinit(struct scsc_mx *mx, struct mxlogger *mxlogger)
        struct mxlogger_node *mn, *next;
        bool match = false;
 
+       /* Run deregistration before adquiring the mxlogger lock to avoid
+        * deadlock with log_collector.
+        */
+#ifdef CONFIG_SCSC_LOG_COLLECTION
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_sync);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_imp);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_common);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_bt);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_wlan);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_radio);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_mxl);
+       scsc_log_collector_unregister_client(&mxlogger_collect_client_udi);
+#endif
        mutex_lock(&mxlogger->lock);
        mxlogger->initialized = false;
        mxlogger_to_host(mxlogger);
@@ -676,16 +696,6 @@ void mxlogger_deinit(struct scsc_mx *mx, struct mxlogger *mxlogger)
        if (match == false)
                SCSC_TAG_ERR(MXMAN, "FATAL, no match for given scsc_mif_abs\n");
 
-#ifdef CONFIG_SCSC_LOG_COLLECTION
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_sync);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_imp);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_common);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_bt);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_wlan);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_rsv_radio);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_mxl);
-       scsc_log_collector_unregister_client(&mxlogger_collect_client_udi);
-#endif
        mutex_unlock(&mxlogger->lock);
 }
 
@@ -711,7 +721,7 @@ int mxlogger_unregister_observer(struct mxlogger *mxlogger, char *name)
        mutex_lock(&mxlogger->lock);
 
        if (mxlogger->observers == 0) {
-               SCSC_TAG_INFO(MXMAN, "Incorrect number of observers \n");
+               SCSC_TAG_INFO(MXMAN, "Incorrect number of observers\n");
                mutex_unlock(&mxlogger->lock);
                return -EIO;
        }
@@ -730,7 +740,8 @@ int mxlogger_unregister_observer(struct mxlogger *mxlogger, char *name)
 }
 
 /* Global observer are not associated to any [mx] mxlogger instance. So it registers as
- * an observer to all the [mx] mxlogger instances. */
+ * an observer to all the [mx] mxlogger instances.
+ */
 int mxlogger_register_global_observer(char *name)
 {
        struct mxlogger_node *mn, *next;

diff --git a/drivers/misc/samsung/scsc/scsc_log_collector.c b/drivers/misc/samsung/scsc/scsc_log_collector.c
index 49913c7b94e9f8355fdf6776a86a82ebabf71df5..7874d32fa70435f31096e9ef6650e1f2b2713f5d 100644 (file)
--- a/drivers/misc/samsung/scsc/scsc_log_collector.c
+++ b/drivers/misc/samsung/scsc/scsc_log_collector.c
@@ -59,14 +59,14 @@ struct scsc_log_status {
        loff_t pos;
        bool in_collection;
        char fapi_ver[SCSC_LOG_FAPI_VERSION_SIZE];
-       struct mutex log_mutex;
-       struct mutex collection_mutex;
 
        struct workqueue_struct *collection_workq;
        struct work_struct      collect_work;
        enum scsc_log_reason    collect_reason;
 } log_status;
 
+static DEFINE_MUTEX(log_mutex);
+
 static void collection_worker(struct work_struct *work)
 {
        struct scsc_log_status *ls;
@@ -83,8 +83,6 @@ int __init scsc_log_collector(void)
 {
        pr_info("Log Collector Init\n");
 
-       mutex_init(&log_status.log_mutex);
-       mutex_init(&log_status.collection_mutex);
        log_status.in_collection = false;
        log_status.collection_workq = create_workqueue("log_collector");
        if (log_status.collection_workq)
@@ -140,9 +138,12 @@ int scsc_log_collector_register_client(struct scsc_log_collector_client *collect
                return -EIO;
        }
 
+       mutex_lock(&log_mutex);
        lc = kzalloc(sizeof(*lc), GFP_KERNEL);
-       if (!lc)
+       if (!lc) {
+               mutex_unlock(&log_mutex);
                return -ENOMEM;
+       }
 
        lc->collect_client = collect_client;
        list_add_tail(&lc->list, &scsc_log_collector_list.list);
@@ -151,6 +152,7 @@ int scsc_log_collector_register_client(struct scsc_log_collector_client *collect
        list_sort(NULL, &scsc_log_collector_list.list, scsc_log_collector_compare);
 
        pr_info("Registered client: %s\n", collect_client->name);
+       mutex_unlock(&log_mutex);
        return 0;
 }
 EXPORT_SYMBOL(scsc_log_collector_register_client);
@@ -161,7 +163,7 @@ int scsc_log_collector_unregister_client(struct scsc_log_collector_client *colle
        bool match = false;
 
        /* block any attempt of unregistering while a collection is in progres */
-       mutex_lock(&log_status.log_mutex);
+       mutex_lock(&log_mutex);
        list_for_each_entry_safe(lc, next, &scsc_log_collector_list.list, list) {
                if (lc->collect_client == collect_client) {
                        match = true;
@@ -174,7 +176,7 @@ int scsc_log_collector_unregister_client(struct scsc_log_collector_client *colle
                pr_err("FATAL, no match for given scsc_log_collector_client\n");
 
        pr_info("Unregistered client: %s\n", collect_client->name);
-       mutex_unlock(&log_status.log_mutex);
+       mutex_unlock(&log_mutex);
 
        return 0;
 }
@@ -241,7 +243,7 @@ static inline int __scsc_log_collector_collect_to_file(enum scsc_log_reason reas
        struct scsc_log_chunk_header chk_header;
        u8 j;
 
-       mutex_lock(&log_status.log_mutex);
+       mutex_lock(&log_mutex);
 
        pr_info("Log collection to file triggered\n");
 
@@ -347,7 +349,7 @@ exit:
 
        pr_info("File %s collection end. Took: %lld\n", memdump_path, ktime_to_ns(ktime_sub(ktime_get(), start)));
 
-       mutex_unlock(&log_status.log_mutex);
+       mutex_unlock(&log_mutex);
        return ret;
 }
 
@@ -355,12 +357,10 @@ int scsc_log_collector_collect(enum scsc_log_reason reason)
 {
        int ret;
 
-       mutex_lock(&log_status.collection_mutex);
        if (collect_to_ram)
                ret = __scsc_log_collector_collect_to_ram(reason);
        else
                ret =  __scsc_log_collector_collect_to_file(reason);
-       mutex_unlock(&log_status.collection_mutex);
 
        return ret;
 }

diff --git a/drivers/net/wireless/scsc/hip4.c b/drivers/net/wireless/scsc/hip4.c
index 34d7e81d92b7fa883606c2d99941feb5d2706461..2d166100556e1f9e4287f65617102537bfcd0874 100755 (executable)
--- a/drivers/net/wireless/scsc/hip4.c
+++ b/drivers/net/wireless/scsc/hip4.c
@@ -1914,9 +1914,10 @@ void hip4_deinit(struct slsi_hip4 *hip)
        service = sdev->service;
 
 #ifdef CONFIG_SCSC_LOG_COLLECTION
-       mutex_lock(&hip->hip_priv->in_collection);
        hip4_collect_hcf_client.prv = NULL;
+       /* Unregister outside the in_collection_lock to avoid deadlock */
        scsc_log_collector_unregister_client(&hip4_collect_hcf_client);
+       mutex_lock(&hip->hip_priv->in_collection);
        if (hip->hip_priv->mib_collect) {
                hip->hip_priv->mib_sz = 0;
                vfree(hip->hip_priv->mib_collect);