[PATCH v3 1/5] mwifiex: don't wait for main_process in shutdown_drv

[PATCH v3 1/5] mwifiex: don't wait for main_process in shutdown_drv

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

main_process is not expected to be running when shutdown_drv function
is called. currently we wait for main_process completion in the
function.

Actually the caller has already made sure main_process is completed by
performing below actions.
(1) disable interrupts in if_ops->disable_int.
(2) set adapter->surprise_removed = true, main_process wont be queued.
(3) mwifiex_terminate_workqueue(adapter), wait for workqueue to be
completed.

This patch removes redundant wait code and takes care of related
cleanup.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: a) This patch is introduced in v3. It replaces "[v2,2/5] mwifiex: use spinlock for
    'mwifiex_processing' in shutdown_drv" patch.
    b) "[v2,1/5] mwifiex: remove redundant condition in main process" is dropped in
    this patch series.
---
 drivers/net/wireless/marvell/mwifiex/init.c | 19 ++--------
 drivers/net/wireless/marvell/mwifiex/main.c | 55 ++++++++++-------------------
 drivers/net/wireless/marvell/mwifiex/main.h |  5 +--
 drivers/net/wireless/marvell/mwifiex/util.c | 15 --------
 4 files changed, 22 insertions(+), 72 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index b36cb3f..f581a15 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -656,10 +656,9 @@ void mwifiex_free_priv(struct mwifiex_private *priv)
  *      - Free the adapter
  *      - Notify completion
  */
-int
+void
 mwifiex_shutdown_drv(struct mwifiex_adapter *adapter)
 {
-	int ret = -EINPROGRESS;
 	struct mwifiex_private *priv;
 	s32 i;
 	unsigned long flags;
@@ -667,15 +666,7 @@ void mwifiex_free_priv(struct mwifiex_private *priv)
 
 	/* mwifiex already shutdown */
 	if (adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY)
-		return 0;
-
-	adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
-	/* wait for mwifiex_process to complete */
-	if (adapter->mwifiex_processing) {
-		mwifiex_dbg(adapter, WARN,
-			    "main process is still running\n");
-		return ret;
-	}
+		return;
 
 	/* cancel current command */
 	if (adapter->curr_cmd) {
@@ -726,11 +717,7 @@ void mwifiex_free_priv(struct mwifiex_private *priv)
 	mwifiex_adapter_cleanup(adapter);
 
 	spin_unlock(&adapter->mwifiex_lock);
-
-	/* Notify completion */
-	ret = mwifiex_shutdown_fw_complete(adapter);
-
-	return ret;
+	adapter->hw_status = MWIFIEX_HW_STATUS_NOT_READY;
 }
 
 /*
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index eac44fe..379e084 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -248,15 +248,14 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 	if (adapter->mwifiex_processing || adapter->main_locked) {
 		adapter->more_task_flag = true;
 		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
-		goto exit_main_proc;
+		return 0;
 	} else {
 		adapter->mwifiex_processing = true;
 		spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
 	}
 process_start:
 	do {
-		if ((adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) ||
-		    (adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY))
+		if (adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY)
 			break;
 
 		/* For non-USB interfaces, If we process interrupts first, it
@@ -464,9 +463,6 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 	adapter->mwifiex_processing = false;
 	spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
 
-exit_main_proc:
-	if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
-		mwifiex_shutdown_drv(adapter);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mwifiex_main_process);
@@ -648,16 +644,14 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 	if (adapter->if_ops.unregister_dev)
 		adapter->if_ops.unregister_dev(adapter);
 
+	adapter->surprise_removed = true;
+	mwifiex_terminate_workqueue(adapter);
+
 	if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
 		pr_debug("info: %s: shutdown mwifiex\n", __func__);
-		adapter->init_wait_q_woken = false;
-
-		if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
-			wait_event_interruptible(adapter->init_wait_q,
-						 adapter->init_wait_q_woken);
+		mwifiex_shutdown_drv(adapter);
 	}
-	adapter->surprise_removed = true;
-	mwifiex_terminate_workqueue(adapter);
+
 	init_failed = true;
 done:
 	if (adapter->cal_data) {
@@ -1402,11 +1396,8 @@ static void mwifiex_main_work_queue(struct work_struct *work)
 	}
 
 	mwifiex_dbg(adapter, CMD, "cmd: calling mwifiex_shutdown_drv...\n");
-	adapter->init_wait_q_woken = false;
 
-	if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
-		wait_event_interruptible(adapter->init_wait_q,
-					 adapter->init_wait_q_woken);
+	mwifiex_shutdown_drv(adapter);
 	if (adapter->if_ops.down_dev)
 		adapter->if_ops.down_dev(adapter);
 
@@ -1512,19 +1503,16 @@ static void mwifiex_main_work_queue(struct work_struct *work)
 	mwifiex_dbg(adapter, ERROR, "info: %s: unregister device\n", __func__);
 	if (adapter->if_ops.unregister_dev)
 		adapter->if_ops.unregister_dev(adapter);
+
+err_kmalloc:
+	adapter->surprise_removed = true;
+	mwifiex_terminate_workqueue(adapter);
 	if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
 		mwifiex_dbg(adapter, ERROR,
 			    "info: %s: shutdown mwifiex\n", __func__);
-		adapter->init_wait_q_woken = false;
-
-		if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
-			wait_event_interruptible(adapter->init_wait_q,
-						 adapter->init_wait_q_woken);
+		mwifiex_shutdown_drv(adapter);
 	}
 
-err_kmalloc:
-	mwifiex_terminate_workqueue(adapter);
-	adapter->surprise_removed = true;
 	complete_all(adapter->fw_done);
 	mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);
 
@@ -1684,17 +1672,13 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 	pr_debug("info: %s: unregister device\n", __func__);
 	if (adapter->if_ops.unregister_dev)
 		adapter->if_ops.unregister_dev(adapter);
-	if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
-		pr_debug("info: %s: shutdown mwifiex\n", __func__);
-		adapter->init_wait_q_woken = false;
-
-		if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
-			wait_event_interruptible(adapter->init_wait_q,
-						 adapter->init_wait_q_woken);
-	}
 err_registerdev:
 	adapter->surprise_removed = true;
 	mwifiex_terminate_workqueue(adapter);
+	if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
+		pr_debug("info: %s: shutdown mwifiex\n", __func__);
+		mwifiex_shutdown_drv(adapter);
+	}
 err_kmalloc:
 	mwifiex_free_adapter(adapter);
 
@@ -1744,11 +1728,8 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 
 	mwifiex_dbg(adapter, CMD,
 		    "cmd: calling mwifiex_shutdown_drv...\n");
-	adapter->init_wait_q_woken = false;
 
-	if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
-		wait_event_interruptible(adapter->init_wait_q,
-					 adapter->init_wait_q_woken);
+	mwifiex_shutdown_drv(adapter);
 	mwifiex_dbg(adapter, CMD,
 		    "cmd: mwifiex_shutdown_drv done\n");
 	if (atomic_read(&adapter->rx_pending) ||
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 5c9bd94..fd56b5d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -248,7 +248,6 @@ enum MWIFIEX_HARDWARE_STATUS {
 	MWIFIEX_HW_STATUS_INITIALIZING,
 	MWIFIEX_HW_STATUS_INIT_DONE,
 	MWIFIEX_HW_STATUS_RESET,
-	MWIFIEX_HW_STATUS_CLOSING,
 	MWIFIEX_HW_STATUS_NOT_READY
 };
 
@@ -1041,9 +1040,7 @@ void mwifiex_wake_up_net_dev_queue(struct net_device *netdev,
 
 int mwifiex_init_fw_complete(struct mwifiex_adapter *adapter);
 
-int mwifiex_shutdown_drv(struct mwifiex_adapter *adapter);
-
-int mwifiex_shutdown_fw_complete(struct mwifiex_adapter *adapter);
+void mwifiex_shutdown_drv(struct mwifiex_adapter *adapter);
 
 int mwifiex_dnld_fw(struct mwifiex_adapter *, struct mwifiex_fw_image *);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 18fbb96..b1ab8da 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -146,21 +146,6 @@ int mwifiex_init_fw_complete(struct mwifiex_adapter *adapter)
 }
 
 /*
- * Firmware shutdown complete callback handler.
- *
- * This function sets the hardware status to not ready and wakes up
- * the function waiting on the init wait queue for the firmware
- * shutdown to complete.
- */
-int mwifiex_shutdown_fw_complete(struct mwifiex_adapter *adapter)
-{
-	adapter->hw_status = MWIFIEX_HW_STATUS_NOT_READY;
-	adapter->init_wait_q_woken = true;
-	wake_up_interruptible(&adapter->init_wait_q);
-	return 0;
-}
-
-/*
  * This function sends init/shutdown command
  * to firmware.
  */
-- 
1.9.1

[PATCH v3 2/5] mwifiex: do not free firmware dump memory in shutdown_drv

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

mwifiex_upload_device_dump() already takes care of freeing firmware dump
memory. Doing the same thing in mwifiex_shutdown_drv() is redundant.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Same as v1. 
drv_info_dump and drv_info_size need not be in adapter structure. Saparate
patch is created for this (Dmitry Torokhov)
v3: Same as [v2,3/5]
---
 drivers/net/wireless/marvell/mwifiex/init.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index f581a15..87cda4f 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -408,8 +408,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
 static void
 mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
 {
-	int idx;
-
 	if (!adapter) {
 		pr_err("%s: adapter is NULL\n", __func__);
 		return;
@@ -427,23 +425,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
 	mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
 	mwifiex_free_cmd_buffer(adapter);
 
-	for (idx = 0; idx < adapter->num_mem_types; idx++) {
-		struct memory_type_mapping *entry =
-				&adapter->mem_type_mapping_tbl[idx];
-
-		if (entry->mem_ptr) {
-			vfree(entry->mem_ptr);
-			entry->mem_ptr = NULL;
-		}
-		entry->mem_size = 0;
-	}
-
-	if (adapter->drv_info_dump) {
-		vfree(adapter->drv_info_dump);
-		adapter->drv_info_dump = NULL;
-		adapter->drv_info_size = 0;
-	}
-
 	if (adapter->sleep_cfm)
 		dev_kfree_skb_any(adapter->sleep_cfm);
 }
-- 
1.9.1

[PATCH v3 3/5] mwifiex: get rid of drv_info* adapter variables

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

We can avoid drv_info_dump and drv_info_size adapter variables.
This info can be passed to mwifiex_upload_device_dump() as parameters

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: This patch is introduced in v2.
Also, I have dropped "v1 4/5 mwifiex: firmware dump code rearrangement
in pcie.c" patch in this series to avoid rebase efforts for other patches
from Rajat/Brian in queue. I will submit this later.
v3: same as [v2,4/5]
---
 drivers/net/wireless/marvell/mwifiex/main.c | 42 ++++++++++++-----------------
 drivers/net/wireless/marvell/mwifiex/main.h |  7 +++--
 drivers/net/wireless/marvell/mwifiex/pcie.c |  7 +++--
 drivers/net/wireless/marvell/mwifiex/sdio.c |  6 +++--
 4 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 379e084..55ac0cf 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1029,7 +1029,7 @@ void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter)
 }
 EXPORT_SYMBOL_GPL(mwifiex_multi_chan_resync);
 
-void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
+int mwifiex_drv_info_dump(struct mwifiex_adapter *adapter, void **drv_info)
 {
 	void *p;
 	char drv_version[64];
@@ -1039,21 +1039,17 @@ void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
 	int i, idx;
 	struct netdev_queue *txq;
 	struct mwifiex_debug_info *debug_info;
-
-	if (adapter->drv_info_dump) {
-		vfree(adapter->drv_info_dump);
-		adapter->drv_info_dump = NULL;
-		adapter->drv_info_size = 0;
-	}
+	void *drv_info_dump;
 
 	mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump start===\n");
 
-	adapter->drv_info_dump = vzalloc(MWIFIEX_DRV_INFO_SIZE_MAX);
+	/* memory allocate here should be free in mwifiex_upload_device_dump*/
+	drv_info_dump = vzalloc(MWIFIEX_DRV_INFO_SIZE_MAX);
 
-	if (!adapter->drv_info_dump)
-		return;
+	if (!drv_info_dump)
+		return 0;
 
-	p = (char *)(adapter->drv_info_dump);
+	p = (char *)(drv_info_dump);
 	p += sprintf(p, "driver_name = " "\"mwifiex\"\n");
 
 	mwifiex_drv_get_driver_version(adapter, drv_version,
@@ -1137,18 +1133,20 @@ void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
 		kfree(debug_info);
 	}
 
-	adapter->drv_info_size = p - adapter->drv_info_dump;
 	mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump end===\n");
+	*drv_info = drv_info_dump;
+	return p - drv_info_dump;
 }
 EXPORT_SYMBOL_GPL(mwifiex_drv_info_dump);
 
-void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
+void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter, void *drv_info,
+				int drv_info_size)
 {
 	u8 idx, *dump_data, *fw_dump_ptr;
 	u32 dump_len;
 
 	dump_len = (strlen("========Start dump driverinfo========\n") +
-		       adapter->drv_info_size +
+		       drv_info_size +
 		       strlen("\n========End dump========\n"));
 
 	for (idx = 0; idx < adapter->num_mem_types; idx++) {
@@ -1178,8 +1176,8 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
 
 	strcpy(fw_dump_ptr, "========Start dump driverinfo========\n");
 	fw_dump_ptr += strlen("========Start dump driverinfo========\n");
-	memcpy(fw_dump_ptr, adapter->drv_info_dump, adapter->drv_info_size);
-	fw_dump_ptr += adapter->drv_info_size;
+	memcpy(fw_dump_ptr, drv_info, drv_info_size);
+	fw_dump_ptr += drv_info_size;
 	strcpy(fw_dump_ptr, "\n========End dump========\n");
 	fw_dump_ptr += strlen("\n========End dump========\n");
 
@@ -1217,18 +1215,12 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
 		struct memory_type_mapping *entry =
 			&adapter->mem_type_mapping_tbl[idx];
 
-		if (entry->mem_ptr) {
-			vfree(entry->mem_ptr);
-			entry->mem_ptr = NULL;
-		}
+		vfree(entry->mem_ptr);
+		entry->mem_ptr = NULL;
 		entry->mem_size = 0;
 	}
 
-	if (adapter->drv_info_dump) {
-		vfree(adapter->drv_info_dump);
-		adapter->drv_info_dump = NULL;
-		adapter->drv_info_size = 0;
-	}
+	vfree(drv_info);
 }
 EXPORT_SYMBOL_GPL(mwifiex_upload_device_dump);
 
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index fd56b5d..d501d03 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -994,8 +994,6 @@ struct mwifiex_adapter {
 	u8 key_api_major_ver, key_api_minor_ver;
 	struct memory_type_mapping *mem_type_mapping_tbl;
 	u8 num_mem_types;
-	void *drv_info_dump;
-	u32 drv_info_size;
 	bool scan_chan_gap_enabled;
 	struct sk_buff_head rx_data_q;
 	bool mfg_mode;
@@ -1641,8 +1639,9 @@ void mwifiex_hist_data_add(struct mwifiex_private *priv,
 u8 mwifiex_adjust_data_rate(struct mwifiex_private *priv,
 			    u8 rx_rate, u8 ht_info);
 
-void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter);
-void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter);
+int mwifiex_drv_info_dump(struct mwifiex_adapter *adapter, void **drv_info);
+void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter, void *drv_info,
+				int drv_info_size);
 void *mwifiex_alloc_dma_align_buf(int rx_len, gfp_t flags);
 void mwifiex_queue_main_work(struct mwifiex_adapter *adapter);
 int mwifiex_get_wakeup_reason(struct mwifiex_private *priv, u16 action,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index e4f1f55..dd8f7aa 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2705,9 +2705,12 @@ static void mwifiex_pcie_fw_dump(struct mwifiex_adapter *adapter)
 
 static void mwifiex_pcie_device_dump_work(struct mwifiex_adapter *adapter)
 {
-	mwifiex_drv_info_dump(adapter);
+	int drv_info_size;
+	void *drv_info;
+
+	drv_info_size = mwifiex_drv_info_dump(adapter, &drv_info);
 	mwifiex_pcie_fw_dump(adapter);
-	mwifiex_upload_device_dump(adapter);
+	mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
 }
 
 static unsigned long iface_work_flags;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 1235f04..16d1d30 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2547,13 +2547,15 @@ static void mwifiex_sdio_generic_fw_dump(struct mwifiex_adapter *adapter)
 static void mwifiex_sdio_device_dump_work(struct mwifiex_adapter *adapter)
 {
 	struct sdio_mmc_card *card = adapter->card;
+	int drv_info_size;
+	void *drv_info;
 
-	mwifiex_drv_info_dump(adapter);
+	drv_info_size = mwifiex_drv_info_dump(adapter, &drv_info);
 	if (card->fw_dump_enh)
 		mwifiex_sdio_generic_fw_dump(adapter);
 	else
 		mwifiex_sdio_fw_dump(adapter);
-	mwifiex_upload_device_dump(adapter);
+	mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
 }
 
 static void mwifiex_sdio_work(struct work_struct *work)
-- 
1.9.1

[PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Amitkumar Karwar]

From: Xinming Hu <huxm@marvell.com>

Wait for firmware dump complete in card remove function.
For sdio interface, there are two diffenrent cases,
card reset trigger sdio_work and firmware dump trigger sdio_work.
Do code rearrangement for distinguish between these two cases.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: 1. Get rid of reset_triggered flag. Instead split the code and use
    __mwifiex_sdio_remove() (Brian Norris/Dmitry Torokhov)
    2. "v1 4/5 mwifiex: firmware dump code rearrangement.." is dropped. So
    rebased accordingly.
v3: same as [v2,5/5]. The improvement of 'moving pcie_work to card struct'
suggested by Brian is taken care in next patch.
---
 drivers/net/wireless/marvell/mwifiex/pcie.c |  6 +++++-
 drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index dd8f7aa..c8e69a4 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -51,6 +51,9 @@ static int mwifiex_pcie_probe_of(struct device *dev)
 	return 0;
 }
 
+static void mwifiex_pcie_work(struct work_struct *work);
+static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
+
 static int
 mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
 		       size_t size, int flags)
@@ -254,6 +257,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	if (!adapter || !adapter->priv_num)
 		return;
 
+	cancel_work_sync(&pcie_work);
+
 	if (user_rmmod && !adapter->mfg_mode) {
 		mwifiex_deauthenticate_all(adapter);
 
@@ -2722,7 +2727,6 @@ static void mwifiex_pcie_work(struct work_struct *work)
 		mwifiex_pcie_device_dump_work(save_adapter);
 }
 
-static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
 /* This function dumps FW information */
 static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
 {
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 16d1d30..78f2cc9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -46,6 +46,9 @@
  */
 static u8 user_rmmod;
 
+static void mwifiex_sdio_work(struct work_struct *work);
+static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
+
 static struct mwifiex_if_ops sdio_ops;
 static unsigned long iface_work_flags;
 
@@ -220,7 +223,7 @@ static int mwifiex_sdio_resume(struct device *dev)
  * This function removes the interface and frees up the card structure.
  */
 static void
-mwifiex_sdio_remove(struct sdio_func *func)
+__mwifiex_sdio_remove(struct sdio_func *func)
 {
 	struct sdio_mmc_card *card;
 	struct mwifiex_adapter *adapter;
@@ -249,6 +252,13 @@ static int mwifiex_sdio_resume(struct device *dev)
 	mwifiex_remove_card(adapter);
 }
 
+static void
+mwifiex_sdio_remove(struct sdio_func *func)
+{
+	cancel_work_sync(&sdio_work);
+	__mwifiex_sdio_remove(func);
+}
+
 /*
  * SDIO suspend.
  *
@@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
 	 * discovered and initializes them from scratch.
 	 */
 
-	mwifiex_sdio_remove(func);
+	__mwifiex_sdio_remove(func);
 
 	/*
 	 * Normally, we would let the driver core take care of releasing these.
@@ -2568,7 +2578,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
 		mwifiex_sdio_card_reset_work(save_adapter);
 }
 
-static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
 /* This function resets the card */
 static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
 {
-- 
1.9.1

[PATCH v3 5/5] mwifiex: move pcie_work and related variables inside card

[Amitkumar Karwar]

From: Ganapathi Bhat <gbhat@marvell.com>

Currently pcie_work and related variables are global. It may create
problem while supporting multiple devices simultaneously. Let's move
it inside card structure so that separate instance will be created/
cancelled in init/teardown threads of each connected devices.

Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: This patch is introduced in v3
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 23 ++++++++++++-----------
 drivers/net/wireless/marvell/mwifiex/pcie.h |  2 ++
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c8e69a4..32fa4ed 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -52,7 +52,6 @@ static int mwifiex_pcie_probe_of(struct device *dev)
 }
 
 static void mwifiex_pcie_work(struct work_struct *work);
-static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
 
 static int
 mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
@@ -222,6 +221,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 		card->pcie.mem_type_mapping_tbl = data->mem_type_mapping_tbl;
 		card->pcie.num_mem_types = data->num_mem_types;
 		card->pcie.can_ext_scan = data->can_ext_scan;
+		INIT_WORK(&card->work, mwifiex_pcie_work);
 	}
 
 	/* device tree node parsing and platform specific configuration*/
@@ -257,7 +257,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	if (!adapter || !adapter->priv_num)
 		return;
 
-	cancel_work_sync(&pcie_work);
+	cancel_work_sync(&card->work);
 
 	if (user_rmmod && !adapter->mfg_mode) {
 		mwifiex_deauthenticate_all(adapter);
@@ -2718,25 +2718,27 @@ static void mwifiex_pcie_device_dump_work(struct mwifiex_adapter *adapter)
 	mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
 }
 
-static unsigned long iface_work_flags;
-static struct mwifiex_adapter *save_adapter;
 static void mwifiex_pcie_work(struct work_struct *work)
 {
+	struct pcie_service_card *card =
+		container_of(work, struct pcie_service_card, work);
+
 	if (test_and_clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
-			       &iface_work_flags))
-		mwifiex_pcie_device_dump_work(save_adapter);
+			       &card->work_flags))
+		mwifiex_pcie_device_dump_work(card->adapter);
 }
 
 /* This function dumps FW information */
 static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
 {
-	save_adapter = adapter;
-	if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags))
+	struct pcie_service_card *card = adapter->card;
+
+	if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags))
 		return;
 
-	set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
+	set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
 
-	schedule_work(&pcie_work);
+	schedule_work(&card->work);
 }
 
 /*
@@ -3202,7 +3204,6 @@ static void mwifiex_pcie_cleanup_module(void)
 	/* Set the flag as user is removing this module. */
 	user_rmmod = 1;
 
-	cancel_work_sync(&pcie_work);
 	pci_unregister_driver(&mwifiex_pcie);
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index ae3365d..21ba5e6 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -386,6 +386,8 @@ struct pcie_service_card {
 #endif
 	struct mwifiex_msix_context msix_ctx[MWIFIEX_NUM_MSIX_VECTORS];
 	struct mwifiex_msix_context share_irq_ctx;
+	struct work_struct work;
+	unsigned long work_flags;
 };
 
 static inline int
-- 
1.9.1

Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Brian Norris]

On Wed, Nov 16, 2016 at 06:39:08PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> Wait for firmware dump complete in card remove function.
> For sdio interface, there are two diffenrent cases,
> card reset trigger sdio_work and firmware dump trigger sdio_work.
> Do code rearrangement for distinguish between these two cases.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: 1. Get rid of reset_triggered flag. Instead split the code and use
>     __mwifiex_sdio_remove() (Brian Norris/Dmitry Torokhov)
>     2. "v1 4/5 mwifiex: firmware dump code rearrangement.." is dropped. So
>     rebased accordingly.
> v3: same as [v2,5/5]. The improvement of 'moving pcie_work to card struct'
> suggested by Brian is taken care in next patch.
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  6 +++++-
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++---
>  2 files changed, 17 insertions(+), 4 deletions(-)

I've expressed my unhappiness with the SDIO card-reset code that already
exists (and prevents doing sane code improvements on the rest of the
driver), but the current change looks OK anyway.

I haven't reviewed patch 1 as closely, but for 2 to 5:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Brian Norris]

Hi,

On Wed, Nov 16, 2016 at 06:39:08PM +0530, Amitkumar Karwar wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> Wait for firmware dump complete in card remove function.
> For sdio interface, there are two diffenrent cases,
> card reset trigger sdio_work and firmware dump trigger sdio_work.
> Do code rearrangement for distinguish between these two cases.

On second review of the SDIO card reset code (which I'll repeat is quite
ugly), you seem to be making a bad distinction here. What if there is a
firmware dump happening concurrently with your card-reset handling? You
*do* want to synchronize with the firmware dump before completing the
card reset, or else you might be freeing up internal card resources that
are still in use. See below.

> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: 1. Get rid of reset_triggered flag. Instead split the code and use
>     __mwifiex_sdio_remove() (Brian Norris/Dmitry Torokhov)
>     2. "v1 4/5 mwifiex: firmware dump code rearrangement.." is dropped. So
>     rebased accordingly.
> v3: same as [v2,5/5]. The improvement of 'moving pcie_work to card struct'
> suggested by Brian is taken care in next patch.
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c |  6 +++++-
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++---
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index dd8f7aa..c8e69a4 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -51,6 +51,9 @@ static int mwifiex_pcie_probe_of(struct device *dev)
>  	return 0;
>  }
>  
> +static void mwifiex_pcie_work(struct work_struct *work);
> +static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
> +
>  static int
>  mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
>  		       size_t size, int flags)
> @@ -254,6 +257,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
>  	if (!adapter || !adapter->priv_num)
>  		return;
>  
> +	cancel_work_sync(&pcie_work);
> +
>  	if (user_rmmod && !adapter->mfg_mode) {
>  		mwifiex_deauthenticate_all(adapter);
>  
> @@ -2722,7 +2727,6 @@ static void mwifiex_pcie_work(struct work_struct *work)
>  		mwifiex_pcie_device_dump_work(save_adapter);
>  }
>  
> -static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
>  /* This function dumps FW information */
>  static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
>  {
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 16d1d30..78f2cc9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -46,6 +46,9 @@
>   */
>  static u8 user_rmmod;
>  
> +static void mwifiex_sdio_work(struct work_struct *work);
> +static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> +
>  static struct mwifiex_if_ops sdio_ops;
>  static unsigned long iface_work_flags;
>  
> @@ -220,7 +223,7 @@ static int mwifiex_sdio_resume(struct device *dev)
>   * This function removes the interface and frees up the card structure.
>   */
>  static void
> -mwifiex_sdio_remove(struct sdio_func *func)
> +__mwifiex_sdio_remove(struct sdio_func *func)
>  {
>  	struct sdio_mmc_card *card;
>  	struct mwifiex_adapter *adapter;
> @@ -249,6 +252,13 @@ static int mwifiex_sdio_resume(struct device *dev)
>  	mwifiex_remove_card(adapter);
>  }
>  
> +static void
> +mwifiex_sdio_remove(struct sdio_func *func)
> +{
> +	cancel_work_sync(&sdio_work);
> +	__mwifiex_sdio_remove(func);
> +}
> +
>  /*
>   * SDIO suspend.
>   *
> @@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
>  	 * discovered and initializes them from scratch.
>  	 */
>  
> -	mwifiex_sdio_remove(func);
> +	__mwifiex_sdio_remove(func);

^^ So here, you're trying to avoid syncing with the card-reset work
event, except that function will free up all your resources (including
the static save_adapter). Thus, you're explicitly allowing a
use-after-free error here. That seems unwise.

Instead, you should actually retain the invariant that you're doing a
full remove/reinitialize here, which includes doing the *same*
cancel_work_sync() here in mwifiex_recreate_adapter() as you would in
any other remove().

IOW, kill the __mwifiex_sdio_remove() and just call
mwifiex_sdio_remove() as you were.

That also means that you can do the same per-adapter cleanup in the
following patch as you do for PCIe.

Brian

>  
>  	/*
>  	 * Normally, we would let the driver core take care of releasing these.
> @@ -2568,7 +2578,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
>  		mwifiex_sdio_card_reset_work(save_adapter);
>  }
>  
> -static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
>  /* This function resets the card */
>  static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
>  {
> -- 
> 1.9.1
> 

RE: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Monday, November 21, 2016 11:06 PM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; dmitry.torokhov@gmail.com; Xinming Hu
> Subject: Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during
> card remove process
> 
> Hi,
> 
> On Wed, Nov 16, 2016 at 06:39:08PM +0530, Amitkumar Karwar wrote:
> > From: Xinming Hu <huxm@marvell.com>
> >
> > Wait for firmware dump complete in card remove function.
> > For sdio interface, there are two diffenrent cases, card reset
> trigger
> > sdio_work and firmware dump trigger sdio_work.
> > Do code rearrangement for distinguish between these two cases.
> 
> On second review of the SDIO card reset code (which I'll repeat is
> quite ugly), you seem to be making a bad distinction here. What if
> there is a firmware dump happening concurrently with your card-reset
> handling?
> You *do* want to synchronize with the firmware dump before completing the
> card reset, or else you might be freeing up internal card resources
> that are still in use. See below.

I ran some tests and observed that if same work function is scheduled by two threads, it won't have re-entrant calls. They will be executed one after another.
In SDIO work function, we have SDIO card reset call after completing firmware dump.
So firmware dump won't run concurrently with card-reset as per my understanding.

> 
> >
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> > v2: 1. Get rid of reset_triggered flag. Instead split the code and
> use
> >     __mwifiex_sdio_remove() (Brian Norris/Dmitry Torokhov)
> >     2. "v1 4/5 mwifiex: firmware dump code rearrangement.." is
> dropped. So
> >     rebased accordingly.
> > v3: same as [v2,5/5]. The improvement of 'moving pcie_work to card
> struct'
> > suggested by Brian is taken care in next patch.
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c |  6 +++++-
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++---
> >  2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index dd8f7aa..c8e69a4 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -51,6 +51,9 @@ static int mwifiex_pcie_probe_of(struct device
> *dev)
> >  	return 0;
> >  }
> >
> > +static void mwifiex_pcie_work(struct work_struct *work); static
> > +DECLARE_WORK(pcie_work, mwifiex_pcie_work);
> > +
> >  static int
> >  mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct
> sk_buff *skb,
> >  		       size_t size, int flags)
> > @@ -254,6 +257,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> *pdev)
> >  	if (!adapter || !adapter->priv_num)
> >  		return;
> >
> > +	cancel_work_sync(&pcie_work);
> > +
> >  	if (user_rmmod && !adapter->mfg_mode) {
> >  		mwifiex_deauthenticate_all(adapter);
> >
> > @@ -2722,7 +2727,6 @@ static void mwifiex_pcie_work(struct
> work_struct *work)
> >  		mwifiex_pcie_device_dump_work(save_adapter);
> >  }
> >
> > -static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
> >  /* This function dumps FW information */  static void
> > mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)  { diff
> > --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 16d1d30..78f2cc9 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -46,6 +46,9 @@
> >   */
> >  static u8 user_rmmod;
> >
> > +static void mwifiex_sdio_work(struct work_struct *work); static
> > +DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> > +
> >  static struct mwifiex_if_ops sdio_ops;  static unsigned long
> > iface_work_flags;
> >
> > @@ -220,7 +223,7 @@ static int mwifiex_sdio_resume(struct device
> *dev)
> >   * This function removes the interface and frees up the card
> structure.
> >   */
> >  static void
> > -mwifiex_sdio_remove(struct sdio_func *func)
> > +__mwifiex_sdio_remove(struct sdio_func *func)
> >  {
> >  	struct sdio_mmc_card *card;
> >  	struct mwifiex_adapter *adapter;
> > @@ -249,6 +252,13 @@ static int mwifiex_sdio_resume(struct device
> *dev)
> >  	mwifiex_remove_card(adapter);
> >  }
> >
> > +static void
> > +mwifiex_sdio_remove(struct sdio_func *func) {
> > +	cancel_work_sync(&sdio_work);
> > +	__mwifiex_sdio_remove(func);
> > +}
> > +
> >  /*
> >   * SDIO suspend.
> >   *
> > @@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct
> sdio_mmc_card *card)
> >  	 * discovered and initializes them from scratch.
> >  	 */
> >
> > -	mwifiex_sdio_remove(func);
> > +	__mwifiex_sdio_remove(func);
> 
> ^^ So here, you're trying to avoid syncing with the card-reset work
> event, except that function will free up all your resources (including
> the static save_adapter). Thus, you're explicitly allowing a use-after-
> free error here. That seems unwise.

Even if firmware dump is triggered after card reset is started, it will execute after card reset is completed as discussed above. Only problem I can see is with "save_adapter". We can put new_adapter pointer in "save_adapter" at the end of mwifiex_recreate_adapter() to solve the issue.

> 
> Instead, you should actually retain the invariant that you're doing a
> full remove/reinitialize here, which includes doing the *same*
> cancel_work_sync() here in mwifiex_recreate_adapter() as you would in
> any other remove().

We are executing mwifiex_recreate_adapter() as a part of sdio_work(). Calling cancel_work_sync() here would cause deadlock. The API is supposed to waits until sdio_work() is finished.

> 
> IOW, kill the __mwifiex_sdio_remove() and just call
> mwifiex_sdio_remove() as you were.
> 
> That also means that you can do the same per-adapter cleanup in the
> following patch as you do for PCIe.

Currently as sdio_work recreates "card", the work structure can't be moved inside card structure.
Let me know your suggestions.

Regards,
Amitkumar

Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Brian Norris]

Hi Amit,

On Thu, Nov 24, 2016 at 12:14:07PM +0000, Amitkumar Karwar wrote:
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Monday, November 21, 2016 11:06 PM
> > To: Amitkumar Karwar
> > Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > rajatja@google.com; dmitry.torokhov@gmail.com; Xinming Hu
> > Subject: Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during
> > card remove process
> > 
> > On Wed, Nov 16, 2016 at 06:39:08PM +0530, Amitkumar Karwar wrote:
> > > From: Xinming Hu <huxm@marvell.com>
> > >
> > > Wait for firmware dump complete in card remove function.
> > > For sdio interface, there are two diffenrent cases, card reset
> > trigger
> > > sdio_work and firmware dump trigger sdio_work.
> > > Do code rearrangement for distinguish between these two cases.
> > 
> > On second review of the SDIO card reset code (which I'll repeat is
> > quite ugly), you seem to be making a bad distinction here. What if
> > there is a firmware dump happening concurrently with your card-reset
> > handling?
> > You *do* want to synchronize with the firmware dump before completing the
> > card reset, or else you might be freeing up internal card resources
> > that are still in use. See below.
> 
> I ran some tests and observed that if same work function is scheduled
> by two threads, it won't have re-entrant calls. They will be executed
> one after another. In SDIO work function, we have SDIO card reset call
> after completing firmware dump. So firmware dump won't run
> concurrently with card-reset as per my understanding.

Ah, you're correct. It's somewhat obscure and potentially fragile, but
correct AFAICT. As you noted though, you do still have a use-after-free
bug, even if the concurrency isn't quite as high as I thought.

> > > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > > ---

...

> > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > @@ -46,6 +46,9 @@
> > >   */
> > >  static u8 user_rmmod;
> > >
> > > +static void mwifiex_sdio_work(struct work_struct *work); static
> > > +DECLARE_WORK(sdio_work, mwifiex_sdio_work);
> > > +
> > >  static struct mwifiex_if_ops sdio_ops;  static unsigned long
> > > iface_work_flags;
> > >
> > > @@ -220,7 +223,7 @@ static int mwifiex_sdio_resume(struct device
> > *dev)
> > >   * This function removes the interface and frees up the card
> > structure.
> > >   */
> > >  static void
> > > -mwifiex_sdio_remove(struct sdio_func *func)
> > > +__mwifiex_sdio_remove(struct sdio_func *func)
> > >  {
> > >  	struct sdio_mmc_card *card;
> > >  	struct mwifiex_adapter *adapter;
> > > @@ -249,6 +252,13 @@ static int mwifiex_sdio_resume(struct device
> > *dev)
> > >  	mwifiex_remove_card(adapter);
> > >  }
> > >
> > > +static void
> > > +mwifiex_sdio_remove(struct sdio_func *func) {
> > > +	cancel_work_sync(&sdio_work);
> > > +	__mwifiex_sdio_remove(func);
> > > +}
> > > +
> > >  /*
> > >   * SDIO suspend.
> > >   *
> > > @@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct
> > sdio_mmc_card *card)
> > >  	 * discovered and initializes them from scratch.
> > >  	 */
> > >
> > > -	mwifiex_sdio_remove(func);
> > > +	__mwifiex_sdio_remove(func);
> > 
> > ^^ So here, you're trying to avoid syncing with the card-reset work
> > event, except that function will free up all your resources (including
> > the static save_adapter). Thus, you're explicitly allowing a use-after-
> > free error here. That seems unwise.
> 
> Even if firmware dump is triggered after card reset is started, it
> will execute after card reset is completed as discussed above. Only
> problem I can see is with "save_adapter". We can put new_adapter
> pointer in "save_adapter" at the end of mwifiex_recreate_adapter() to
> solve the issue.

Ugh, yet another band-aid? You might do better to still cancel any
pending work, just don't do it synchronously. i.e., do cancel_work()
after you've removed the card. It doesn't make sense to do a FW dump on
the "new" adapter when it was requested for the old one.

> > Instead, you should actually retain the invariant that you're doing a
> > full remove/reinitialize here, which includes doing the *same*
> > cancel_work_sync() here in mwifiex_recreate_adapter() as you would in
> > any other remove().
> 
> We are executing mwifiex_recreate_adapter() as a part of sdio_work(). Calling
> cancel_work_sync() here would cause deadlock. The API is supposed to waits
> until sdio_work() is finished.

You are correct. So using the _sync() version would be wrong.

> > 
> > IOW, kill the __mwifiex_sdio_remove() and just call
> > mwifiex_sdio_remove() as you were.
> > 
> > That also means that you can do the same per-adapter cleanup in the
> > following patch as you do for PCIe.
> 
> Currently as sdio_work recreates "card", the work structure can't be moved inside card structure.
> Let me know your suggestions.

If you address the TODO in mwifiex_create_adapter() instead, you can get
past this problem:

        /* TODO mmc_hw_reset does not require destroying and re-probing the
         * whole adapter. Hence there was no need to for this rube-goldberg
         * design to reload the fw from an external workqueue. If we don't
         * destroy the adapter we could reload the fw from
         * mwifiex_main_work_queue directly.

The "save_adapter" is an abomination that should be terminated swiftly,
but it is perpetuated in part by the hacks noted in the TODO.

So I'd recommend addressing the TODO ASAP, but in the meantime, a hack
like my suggestion (cancel the FW dump work w/o synchronizing) or --
less preferably -- yours (manually set 'save_adapter' again) might be
OK.

I think I've asked elsewhere but didn't receive an answer: why is
SDIO's mwifiex_recreate_adapter() so much different from PCIe's
mwifiex_do_flr()? It seems like the latter should be refactored to
remove some of the PCIe-specific cruft from main.c and then reused for
SDIO.

Brian

RE: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Amitkumar Karwar]

Hi Brian,

> > > >   *
> > > > @@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct
> > > sdio_mmc_card *card)
> > > >  	 * discovered and initializes them from scratch.
> > > >  	 */
> > > >
> > > > -	mwifiex_sdio_remove(func);
> > > > +	__mwifiex_sdio_remove(func);
> > >
> > > ^^ So here, you're trying to avoid syncing with the card-reset work
> > > event, except that function will free up all your resources
> > > (including the static save_adapter). Thus, you're explicitly
> > > allowing a use-after- free error here. That seems unwise.
> >
> > Even if firmware dump is triggered after card reset is started, it
> > will execute after card reset is completed as discussed above. Only
> > problem I can see is with "save_adapter". We can put new_adapter
> > pointer in "save_adapter" at the end of mwifiex_recreate_adapter() to
> > solve the issue.
> 
> Ugh, yet another band-aid? You might do better to still cancel any
> pending work, just don't do it synchronously. i.e., do cancel_work()
> after you've removed the card. It doesn't make sense to do a FW dump on
> the "new" adapter when it was requested for the old one.

I could not find async version of cancel_work().
We can fix this problem with below change at the end of mwifiex_sdio_work(). All pending work requests would be ignored.

--------
@ -2571,6 +2571,8 @@ static void mwifiex_sdio_work(struct work_struct *work)
        if (test_and_clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET,
                               &iface_work_flags))
                mwifiex_sdio_card_reset_work(save_adapter);
+       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
+       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &iface_work_flags);
 }
----------

> 
> > > Instead, you should actually retain the invariant that you're doing
> > > a full remove/reinitialize here, which includes doing the *same*
> > > cancel_work_sync() here in mwifiex_recreate_adapter() as you would
> > > in any other remove().
> >
> > We are executing mwifiex_recreate_adapter() as a part of sdio_work().
> > Calling
> > cancel_work_sync() here would cause deadlock. The API is supposed to
> > waits until sdio_work() is finished.
> 
> You are correct. So using the _sync() version would be wrong.
> 
> > >
> > > IOW, kill the __mwifiex_sdio_remove() and just call
> > > mwifiex_sdio_remove() as you were.
> > >
> > > That also means that you can do the same per-adapter cleanup in the
> > > following patch as you do for PCIe.
> >
> > Currently as sdio_work recreates "card", the work structure can't be
> moved inside card structure.
> > Let me know your suggestions.
> 
> If you address the TODO in mwifiex_create_adapter() instead, you can
> get past this problem:
> 
>         /* TODO mmc_hw_reset does not require destroying and re-probing
> the
>          * whole adapter. Hence there was no need to for this rube-
> goldberg
>          * design to reload the fw from an external workqueue. If we
> don't
>          * destroy the adapter we could reload the fw from
>          * mwifiex_main_work_queue directly.
> 
> The "save_adapter" is an abomination that should be terminated swiftly,
> but it is perpetuated in part by the hacks noted in the TODO.
> 
> So I'd recommend addressing the TODO ASAP, but in the meantime, a hack
> like my suggestion (cancel the FW dump work w/o synchronizing) or --
> less preferably -- yours (manually set 'save_adapter' again) might be
> OK.
> 
> I think I've asked elsewhere but didn't receive an answer: why is
> SDIO's mwifiex_recreate_adapter() so much different from PCIe's
> mwifiex_do_flr()? It seems like the latter should be refactored to
> remove some of the PCIe-specific cruft from main.c and then reused for
> SDIO.

Our initial SDIO card reset implementation was based on MMC APIs where remove() and probe() would automatically get called by MMC subsystem after power cycle.
https://www.spinics.net/lists/linux-wireless/msg98435.html
Later it was improved by Andreas Fenkart by replacing those power cycle APIs with mmc_hw_reset().

For PCIe, function level reset is standard feature. We implemented ".reset_notify" handler which gets called after and before FLR.

You are right. We can have SDIO's handling similar to PCIe and avoid destroying+recreating adapter/card.
We have started working on this. We will get rid of global save_adapter, sdio_work etc. Meanwhile I will post above mentioned change if it looks good to you.

Regards,
Amitkumar

Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Brian Norris]

On Wed, Nov 30, 2016 at 12:39:11PM +0000, Amitkumar Karwar wrote:

> > Ugh, yet another band-aid? You might do better to still cancel any
> > pending work, just don't do it synchronously. i.e., do cancel_work()
> > after you've removed the card. It doesn't make sense to do a FW dump on
> > the "new" adapter when it was requested for the old one.
> 
> I could not find async version of cancel_work().

cancel_work() *is* asynchronous. It does not synchronize with the last
event, so you won't have the deadlock. (Remember: the synchronous
version is cancel_work_sync().)

> We can fix this problem with below change at the end of
> mwifiex_sdio_work(). All pending work requests would be ignored.
> 
> --------
> @ -2571,6 +2571,8 @@ static void mwifiex_sdio_work(struct work_struct *work)
>         if (test_and_clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET,
>                                &iface_work_flags))
>                 mwifiex_sdio_card_reset_work(save_adapter);
> +       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
> +       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &iface_work_flags);
>  }
> ----------

I don't think that's exactly what you want. That might lose events,
won't it? I'd rather this sort of hack go into
mwifiex_recreate_adapter(), in between the remove() and probe() calls,
where you don't expect any new events to trigger. And maybe include a
comment as to why.

> > I think I've asked elsewhere but didn't receive an answer: why is
> > SDIO's mwifiex_recreate_adapter() so much different from PCIe's
> > mwifiex_do_flr()? It seems like the latter should be refactored to
> > remove some of the PCIe-specific cruft from main.c and then reused for
> > SDIO.
> 
> Our initial SDIO card reset implementation was based on MMC APIs where
> remove() and probe() would automatically get called by MMC subsystem
> after power cycle.
> https://www.spinics.net/lists/linux-wireless/msg98435.html
> Later it was improved by Andreas Fenkart by replacing those power
> cycle APIs with mmc_hw_reset().

Right.

> For PCIe, function level reset is standard feature. We implemented
> ".reset_notify" handler which gets called after and before FLR.

OK.

> You are right. We can have SDIO's handling similar to PCIe and avoid
> destroying+recreating adapter/card.

So all in all, you're saying it's just an artifact of history, and
there's no good reason they are so different? If so, then this looks
like another instance where you would have done well to refactor and
improve the existing mechanisms at the same time as you added new
features (i.e., PCIe FLR). I've seen this problem already several times,
where it seems development for your SDIO/PCIe/USB interface drivers
occur almost in isolation. IMO, it'd do you well to notice these
patterns while implementing features in the first place. The more code
you can share, the fewer bugs you (or I) will have to chase down.

> We have started working on this. We will get rid of global
> save_adapter, sdio_work etc.

Great!

> Meanwhile I will post above mentioned change if it looks good to you.

It's not perfect, but it's a start.

Brian

RE: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Amitkumar Karwar]

Hi Brian,

> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Thursday, December 01, 2016 12:04 AM
> To: Amitkumar Karwar
> Cc: linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> rajatja@google.com; dmitry.torokhov@gmail.com; Xinming Hu
> Subject: Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during
> card remove process
> 
> On Wed, Nov 30, 2016 at 12:39:11PM +0000, Amitkumar Karwar wrote:
> 
> > > Ugh, yet another band-aid? You might do better to still cancel any
> > > pending work, just don't do it synchronously. i.e., do
> cancel_work()
> > > after you've removed the card. It doesn't make sense to do a FW
> dump
> > > on the "new" adapter when it was requested for the old one.
> >
> > I could not find async version of cancel_work().
> 
> cancel_work() *is* asynchronous. It does not synchronize with the last
> event, so you won't have the deadlock. (Remember: the synchronous
> version is cancel_work_sync().)

My bad! What I meant is "I could not find async version of cancel_work_sync()"
cancel_work() isn't available in http://lxr.free-electrons.com/source/kernel/workqueue.c
Anyways, clear_bit() after remove() during card reset would address the problem.

> 
> > We can fix this problem with below change at the end of
> > mwifiex_sdio_work(). All pending work requests would be ignored.
> >
> > --------
> > @ -2571,6 +2571,8 @@ static void mwifiex_sdio_work(struct work_struct
> *work)
> >         if (test_and_clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET,
> >                                &iface_work_flags))
> >                 mwifiex_sdio_card_reset_work(save_adapter);
> > +       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
> > +       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &iface_work_flags);
> >  }
> > ----------
> 
> I don't think that's exactly what you want. That might lose events,
> won't it? I'd rather this sort of hack go into
> mwifiex_recreate_adapter(), in between the remove() and probe() calls,
> where you don't expect any new events to trigger. And maybe include a
> comment as to why.

Right. I have just posted a patch for this.

> 
> > > I think I've asked elsewhere but didn't receive an answer: why is
> > > SDIO's mwifiex_recreate_adapter() so much different from PCIe's
> > > mwifiex_do_flr()? It seems like the latter should be refactored to
> > > remove some of the PCIe-specific cruft from main.c and then reused
> > > for SDIO.
> >
> > Our initial SDIO card reset implementation was based on MMC APIs
> where
> > remove() and probe() would automatically get called by MMC subsystem
> > after power cycle.
> > https://www.spinics.net/lists/linux-wireless/msg98435.html
> > Later it was improved by Andreas Fenkart by replacing those power
> > cycle APIs with mmc_hw_reset().
> 
> Right.
> 
> > For PCIe, function level reset is standard feature. We implemented
> > ".reset_notify" handler which gets called after and before FLR.
> 
> OK.
> 
> > You are right. We can have SDIO's handling similar to PCIe and avoid
> > destroying+recreating adapter/card.
> 
> So all in all, you're saying it's just an artifact of history, and
> there's no good reason they are so different? If so, then this looks
> like another instance where you would have done well to refactor and
> improve the existing mechanisms at the same time as you added new
> features (i.e., PCIe FLR). I've seen this problem already several
> times, where it seems development for your SDIO/PCIe/USB interface
> drivers occur almost in isolation. IMO, it'd do you well to notice
> these patterns while implementing features in the first place. The more
> code you can share, the fewer bugs you (or I) will have to chase down.

Thanks for your guidance. I'll follow this for future development.

Regards,
Amitkumar

Re: [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process

[Brian Norris]

Hi,

On Thu, Dec 01, 2016 at 02:02:43PM +0000, Amitkumar Karwar wrote:
> > > I could not find async version of cancel_work().
> > 
> > cancel_work() *is* asynchronous. It does not synchronize with the last
> > event, so you won't have the deadlock. (Remember: the synchronous
> > version is cancel_work_sync().)
> 
> My bad! What I meant is "I could not find async version of cancel_work_sync()"
> cancel_work() isn't available in http://lxr.free-electrons.com/source/kernel/workqueue.c

It's in 4.9-rc1 (and it's available at the above link, at least by now).
See:

commit f72b8792d180948b4b3898374998f5ac8c02e539
Author: Jens Axboe <axboe@fb.com>
Date:   Wed Aug 24 15:51:50 2016 -0600

    workqueue: add cancel_work()

But anyway:

> Anyways, clear_bit() after remove() during card reset would address the problem.

Yes, I think that's OK.

Brian

Re: [v3,1/5] mwifiex: don't wait for main_process in shutdown_drv

[Kalle Valo]

Amitkumar Karwar <akarwar@marvell.com> wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> main_process is not expected to be running when shutdown_drv function
> is called. currently we wait for main_process completion in the
> function.
> 
> Actually the caller has already made sure main_process is completed by
> performing below actions.
> (1) disable interrupts in if_ops->disable_int.
> (2) set adapter->surprise_removed = true, main_process wont be queued.
> (3) mwifiex_terminate_workqueue(adapter), wait for workqueue to be
> completed.
> 
> This patch removes redundant wait code and takes care of related
> cleanup.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>

5 patches applied to wireless-drivers-next.git, thanks.

5bf15e3fb85d mwifiex: don't wait for main_process in shutdown_drv
fb45bd0c6d6b mwifiex: do not free firmware dump memory in shutdown_drv
d27121fca129 mwifiex: get rid of drv_info* adapter variables
41efaf5824e7 mwifiex: wait firmware dump complete during card remove process
3860e5e39532 mwifiex: move pcie_work and related variables inside card

-- 
https://patchwork.kernel.org/patch/9431485/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches