All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
@ 2016-11-11 13:10 Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 02/11] mwifiex: complete blocked power save handshake in main process Amitkumar Karwar
                   ` (11 more replies)
  0 siblings, 12 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Shengzhen Li, Amitkumar Karwar

From: Shengzhen Li <szli@marvell.com>

We may get SLEEP event from firmware even if TXDone interrupt
for last Tx packet is still pending. In this case, we may
end up accessing PCIe memory for handling TXDone after power
save handshake is completed. This causes kernel crash with
external abort.

This patch will only allow downloading sleep confirm
when no tx done interrupt is pending in the hardware.

Signed-off-by: Cathy Luo <cluo@marvell.com>
Signed-off-by: Shengzhen Li <szli@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: address format issues(Brain)
RESEND v2(Applicable for complete patch series):
    1) Fixed syntax issue "changelog not placed after the  Sign-offs"
       pointed by Brian.
    2) Dropped "[v2,03/12] mwifiex: don't do unbalanced free()'ing in
       cleanup_if()" patch in this series. It was already sent by Brian
       separately.
v3: Same as RESEND v2.

Hi Kalle,

There are multiple mwifiex patches under review. I want you consider them
in following sequence(first being oldest) to avoid conflicts

[v3] mwifiex: report wakeup for wowlan
mwifiex: add power save parameters in hs_cfg cmd
[2/2] mwifiex: ignore calibration data failure (Note: 1/2 has dropped)
[v6] mwifiex: parse device tree node for PCIe
[v2,1/3] mwifiex: Allow mwifiex early access to device structure
[v2,2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties
[v2,3/3] mwifiex: Enable WoWLAN for both sdio and pcie
mwifiex: don't do unbalanced free()'ing in cleanup_if()
mwifiex: printk() overflow with 32-byte SSIDs
mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels()
[v3,01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
[v3,02/11] mwifiex: complete blocked power save handshake in main process
[v3,03/11] mwifiex: resolve races between async FW init (failure) and device removal
[v3,04/11] mwifiex: remove redundant pdev check in suspend/resume handlers
[v3,05/11] mwifiex: don't pretend to resume while remove()'ing
[v3,06/11] mwifiex: resolve suspend() race with async FW init failure
[v3,07/11] mwifiex: reset card->adapter during device unregister
[v3,08/11] mwifiex: usb: handle HS failures
[v3,09/11] mwifiex: sdio: don't check for NULL sdio_func
[v3,10/11] mwifiex: stop checking for NULL drvata/intfdata
[v3,11/11] mwifiex: pcie: stop checking for NULL adapter->card
---
 drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++--
 drivers/net/wireless/marvell/mwifiex/init.c   | 1 +
 drivers/net/wireless/marvell/mwifiex/main.h   | 1 +
 drivers/net/wireless/marvell/mwifiex/pcie.c   | 5 +++++
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 5347728..25a7475 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -1118,13 +1118,14 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
 void
 mwifiex_check_ps_cond(struct mwifiex_adapter *adapter)
 {
-	if (!adapter->cmd_sent &&
+	if (!adapter->cmd_sent && !atomic_read(&adapter->tx_hw_pending) &&
 	    !adapter->curr_cmd && !IS_CARD_RX_RCVD(adapter))
 		mwifiex_dnld_sleep_confirm_cmd(adapter);
 	else
 		mwifiex_dbg(adapter, CMD,
-			    "cmd: Delay Sleep Confirm (%s%s%s)\n",
+			    "cmd: Delay Sleep Confirm (%s%s%s%s)\n",
 			    (adapter->cmd_sent) ? "D" : "",
+			    atomic_read(&adapter->tx_hw_pending) ? "T" : "",
 			    (adapter->curr_cmd) ? "C" : "",
 			    (IS_CARD_RX_RCVD(adapter)) ? "R" : "");
 }
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 82839d9..b36cb3f 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -270,6 +270,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter *adapter)
 	adapter->adhoc_11n_enabled = false;
 
 	mwifiex_wmm_init(adapter);
+	atomic_set(&adapter->tx_hw_pending, 0);
 
 	sleep_cfm_buf = (struct mwifiex_opt_sleep_confirm *)
 					adapter->sleep_cfm->data;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 11abc49..b0c501d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -857,6 +857,7 @@ struct mwifiex_adapter {
 	atomic_t rx_pending;
 	atomic_t tx_pending;
 	atomic_t cmd_pending;
+	atomic_t tx_hw_pending;
 	struct workqueue_struct *workqueue;
 	struct work_struct main_work;
 	struct workqueue_struct *rx_workqueue;
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ba1fa17..509c156 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -522,6 +522,7 @@ static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
 		}
 	}
 
+	atomic_set(&adapter->tx_hw_pending, 0);
 	return 0;
 }
 
@@ -721,6 +722,7 @@ static void mwifiex_cleanup_txq_ring(struct mwifiex_adapter *adapter)
 		card->tx_buf_list[i] = NULL;
 	}
 
+	atomic_set(&adapter->tx_hw_pending, 0);
 	return;
 }
 
@@ -1158,6 +1160,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter)
 							    -1);
 			else
 				mwifiex_write_data_complete(adapter, skb, 0, 0);
+			atomic_dec(&adapter->tx_hw_pending);
 		}
 
 		card->tx_buf_list[wrdoneidx] = NULL;
@@ -1250,6 +1253,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter)
 		wrindx = (card->txbd_wrptr & reg->tx_mask) >> reg->tx_start_ptr;
 		buf_pa = MWIFIEX_SKB_DMA_ADDR(skb);
 		card->tx_buf_list[wrindx] = skb;
+		atomic_inc(&adapter->tx_hw_pending);
 
 		if (reg->pfu_enabled) {
 			desc2 = card->txbd_ring[wrindx];
@@ -1327,6 +1331,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter)
 done_unmap:
 	mwifiex_unmap_pci_memory(adapter, skb, PCI_DMA_TODEVICE);
 	card->tx_buf_list[wrindx] = NULL;
+	atomic_dec(&adapter->tx_hw_pending);
 	if (reg->pfu_enabled)
 		memset(desc2, 0, sizeof(*desc2));
 	else
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 02/11] mwifiex: complete blocked power save handshake in main process
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 03/11] mwifiex: resolve races between async FW init (failure) and device removal Amitkumar Karwar
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Shengzhen Li, Amitkumar Karwar

From: Shengzhen Li <szli@marvell.com>

Power save handshake with firmware might be blocked by on-going
data transfer.
this patch check the PS status in main process and complete
previous blocked PS handshake.
this patch also remove redudant check before call
mwifiex_check_ps_cond function.

Signed-off-by: Cathy Luo <cluo@marvell.com>
Signed-off-by: Shengzhen Li <szli@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: 1. remove redudant check(Brain)
        2. reorgnized to follow tx_hw_pending patch
v3: Same as v2
---
 drivers/net/wireless/marvell/mwifiex/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 20c9b77..c710d5e 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -308,6 +308,9 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 			/* We have tried to wakeup the card already */
 			if (adapter->pm_wakeup_fw_try)
 				break;
+			if (adapter->ps_state == PS_STATE_PRE_SLEEP)
+				mwifiex_check_ps_cond(adapter);
+
 			if (adapter->ps_state != PS_STATE_AWAKE)
 				break;
 			if (adapter->tx_lock_flag) {
@@ -355,10 +358,8 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
 
 		/* Check if we need to confirm Sleep Request
 		   received previously */
-		if (adapter->ps_state == PS_STATE_PRE_SLEEP) {
-			if (!adapter->cmd_sent && !adapter->curr_cmd)
-				mwifiex_check_ps_cond(adapter);
-		}
+		if (adapter->ps_state == PS_STATE_PRE_SLEEP)
+			mwifiex_check_ps_cond(adapter);
 
 		/* * The ps_state may have been changed during processing of
 		 * Sleep Request event.
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 03/11] mwifiex: resolve races between async FW init (failure) and device removal
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 02/11] mwifiex: complete blocked power save handshake in main process Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 04/11] mwifiex: remove redundant pdev check in suspend/resume handlers Amitkumar Karwar
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris, Amitkumar Karwar

From: Brian Norris <briannorris@chromium.org>

It's possible for the FW init sequence to fail, which will trigger a
device cleanup sequence in mwifiex_fw_dpc(). This sequence can race with
device suspend() or remove() (e.g., reboot or unbind), and can trigger
use-after-free issues. Currently, this driver attempts (poorly) to
synchronize remove() using a semaphore, but it doesn't protect some of
the critical sections properly. Particularly, we grab a pointer to the
adapter struct (card->adapter) without checking if it's being freed or
not. We later do a NULL check on the adapter, but that doesn't work if
the adapter was freed.

Also note that the PCIe interface driver doesn't ever set card->adapter
to NULL, so even if we get the synchronization right, we still might try
to redo the cleanup in ->remove(), even if the FW init failure sequence
already did it.

This patch replaces the static semaphore with a per-device completion
struct, and uses that completion to synchronize the remove() thread with
the mwifiex_fw_dpc(). A future patch will utilize this completion to
synchronize the suspend() thread as well.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Same as v1
v3: Included Brian's suggested change which fixes new use-after-free
introduced by this patch.
---
 drivers/net/wireless/marvell/mwifiex/main.c | 47 +++++++++++------------------
 drivers/net/wireless/marvell/mwifiex/main.h | 13 +++++---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 18 +++++------
 drivers/net/wireless/marvell/mwifiex/pcie.h |  2 ++
 drivers/net/wireless/marvell/mwifiex/sdio.c | 18 +++++------
 drivers/net/wireless/marvell/mwifiex/sdio.h |  2 ++
 drivers/net/wireless/marvell/mwifiex/usb.c  | 23 ++++++--------
 drivers/net/wireless/marvell/mwifiex/usb.h  |  2 ++
 8 files changed, 56 insertions(+), 69 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index c710d5e..1c81ca0 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -521,9 +521,9 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 	struct mwifiex_private *priv;
 	struct mwifiex_adapter *adapter = context;
 	struct mwifiex_fw_image fw;
-	struct semaphore *sem = adapter->card_sem;
 	bool init_failed = false;
 	struct wireless_dev *wdev;
+	struct completion *fw_done = adapter->fw_done;
 
 	if (!firmware) {
 		mwifiex_dbg(adapter, ERROR,
@@ -670,7 +670,8 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 	}
 	if (init_failed)
 		mwifiex_free_adapter(adapter);
-	up(sem);
+	/* Tell all current and future waiters we're finished */
+	complete_all(fw_done);
 	return;
 }
 
@@ -1365,7 +1366,7 @@ static void mwifiex_main_work_queue(struct work_struct *work)
  * code is extracted from mwifiex_remove_card()
  */
 static int
-mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem)
+mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
 {
 	struct mwifiex_private *priv;
 	int i;
@@ -1373,8 +1374,9 @@ static void mwifiex_main_work_queue(struct work_struct *work)
 	if (!adapter)
 		goto exit_return;
 
-	if (down_interruptible(sem))
-		goto exit_sem_err;
+	wait_for_completion(adapter->fw_done);
+	/* Caller should ensure we aren't suspending while this happens */
+	reinit_completion(adapter->fw_done);
 
 	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
 	mwifiex_deauthenticate(priv, NULL);
@@ -1431,8 +1433,6 @@ static void mwifiex_main_work_queue(struct work_struct *work)
 		rtnl_unlock();
 	}
 
-	up(sem);
-exit_sem_err:
 	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
 exit_return:
 	return 0;
@@ -1442,21 +1442,18 @@ static void mwifiex_main_work_queue(struct work_struct *work)
  * code is extracted from mwifiex_add_card()
  */
 static int
-mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem,
+mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct completion *fw_done,
 		  struct mwifiex_if_ops *if_ops, u8 iface_type)
 {
 	char fw_name[32];
 	struct pcie_service_card *card = adapter->card;
 
-	if (down_interruptible(sem))
-		goto exit_sem_err;
-
 	mwifiex_init_lock_list(adapter);
 	if (adapter->if_ops.up_dev)
 		adapter->if_ops.up_dev(adapter);
 
 	adapter->iface_type = iface_type;
-	adapter->card_sem = sem;
+	adapter->fw_done = fw_done;
 
 	adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;
 	adapter->surprise_removed = false;
@@ -1507,7 +1504,8 @@ static void mwifiex_main_work_queue(struct work_struct *work)
 	}
 	strcpy(adapter->fw_name, fw_name);
 	mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
-	up(sem);
+
+	complete_all(adapter->fw_done);
 	return 0;
 
 err_init_fw:
@@ -1527,8 +1525,7 @@ static void mwifiex_main_work_queue(struct work_struct *work)
 err_kmalloc:
 	mwifiex_terminate_workqueue(adapter);
 	adapter->surprise_removed = true;
-	up(sem);
-exit_sem_err:
+	complete_all(adapter->fw_done);
 	mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);
 
 	return -1;
@@ -1543,12 +1540,12 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
 	struct mwifiex_if_ops if_ops;
 
 	if (!prepare) {
-		mwifiex_reinit_sw(adapter, adapter->card_sem, &if_ops,
+		mwifiex_reinit_sw(adapter, adapter->fw_done, &if_ops,
 				  adapter->iface_type);
 	} else {
 		memcpy(&if_ops, &adapter->if_ops,
 		       sizeof(struct mwifiex_if_ops));
-		mwifiex_shutdown_sw(adapter, adapter->card_sem);
+		mwifiex_shutdown_sw(adapter);
 	}
 }
 EXPORT_SYMBOL_GPL(mwifiex_do_flr);
@@ -1618,15 +1615,12 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
  *      - Add logical interfaces
  */
 int
-mwifiex_add_card(void *card, struct semaphore *sem,
+mwifiex_add_card(void *card, struct completion *fw_done,
 		 struct mwifiex_if_ops *if_ops, u8 iface_type,
 		 struct device *dev, bool of_node_valid)
 {
 	struct mwifiex_adapter *adapter;
 
-	if (down_interruptible(sem))
-		goto exit_sem_err;
-
 	if (mwifiex_register(card, if_ops, (void **)&adapter)) {
 		pr_err("%s: software init failed\n", __func__);
 		goto err_init_sw;
@@ -1637,7 +1631,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 		mwifiex_probe_of(adapter);
 
 	adapter->iface_type = iface_type;
-	adapter->card_sem = sem;
+	adapter->fw_done = fw_done;
 
 	adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;
 	adapter->surprise_removed = false;
@@ -1706,9 +1700,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
 	mwifiex_free_adapter(adapter);
 
 err_init_sw:
-	up(sem);
 
-exit_sem_err:
 	return -1;
 }
 EXPORT_SYMBOL_GPL(mwifiex_add_card);
@@ -1724,14 +1716,11 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
  *      - Unregister the device
  *      - Free the adapter structure
  */
-int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
+int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 {
 	struct mwifiex_private *priv = NULL;
 	int i;
 
-	if (down_trylock(sem))
-		goto exit_sem_err;
-
 	if (!adapter)
 		goto exit_remove;
 
@@ -1801,8 +1790,6 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
 	mwifiex_free_adapter(adapter);
 
 exit_remove:
-	up(sem);
-exit_sem_err:
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mwifiex_remove_card);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index b0c501d..d0fbb2f 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -20,6 +20,7 @@
 #ifndef _MWIFIEX_MAIN_H_
 #define _MWIFIEX_MAIN_H_
 
+#include <linux/completion.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/sched.h>
@@ -985,7 +986,10 @@ struct mwifiex_adapter {
 	u32 usr_dot_11ac_mcs_support;
 
 	atomic_t pending_bridged_pkts;
-	struct semaphore *card_sem;
+
+	/* For synchronizing FW initialization with device lifecycle. */
+	struct completion *fw_done;
+
 	bool ext_scan;
 	u8 fw_api_ver;
 	u8 key_api_major_ver, key_api_minor_ver;
@@ -1438,9 +1442,10 @@ static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
 
 int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
 			     u32 func_init_shutdown);
-int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
-		     struct device *, bool);
-int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
+int mwifiex_add_card(void *card, struct completion *fw_done,
+		     struct mwifiex_if_ops *if_ops, u8 iface_type,
+		     struct device *dev, bool of_node_valid);
+int mwifiex_remove_card(struct mwifiex_adapter *adapter);
 
 void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
 			 int maxlen);
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 509c156..c4bd64b 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -35,8 +35,6 @@
 
 static struct mwifiex_if_ops pcie_ops;
 
-static struct semaphore add_remove_card_sem;
-
 static const struct of_device_id mwifiex_pcie_of_match_table[] = {
 	{ .compatible = "pci11ab,2b42" },
 	{ .compatible = "pci1b4b,2b42" },
@@ -213,6 +211,8 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 	if (!card)
 		return -ENOMEM;
 
+	init_completion(&card->fw_done);
+
 	card->dev = pdev;
 
 	if (ent->driver_data) {
@@ -234,7 +234,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
 		valid_of_node = true;
 	}
 
-	ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
+	ret = mwifiex_add_card(card, &card->fw_done, &pcie_ops,
 			       MWIFIEX_PCIE, &pdev->dev, valid_of_node);
 	if (ret) {
 		pr_err("%s failed\n", __func__);
@@ -260,6 +260,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	if (!card)
 		return;
 
+	wait_for_completion(&card->fw_done);
+
 	adapter = card->adapter;
 	if (!adapter || !adapter->priv_num)
 		return;
@@ -279,7 +281,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 		mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
 	}
 
-	mwifiex_remove_card(card->adapter, &add_remove_card_sem);
+	mwifiex_remove_card(adapter);
 }
 
 static void mwifiex_pcie_shutdown(struct pci_dev *pdev)
@@ -3181,8 +3183,7 @@ static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter)
 /*
  * This function initializes the PCIE driver module.
  *
- * This initiates the semaphore and registers the device with
- * PCIE bus.
+ * This registers the device with PCIE bus.
  */
 static int mwifiex_pcie_init_module(void)
 {
@@ -3190,8 +3191,6 @@ static int mwifiex_pcie_init_module(void)
 
 	pr_debug("Marvell PCIe Driver\n");
 
-	sema_init(&add_remove_card_sem, 1);
-
 	/* Clear the flag in case user removes the card. */
 	user_rmmod = 0;
 
@@ -3215,9 +3214,6 @@ static int mwifiex_pcie_init_module(void)
  */
 static void mwifiex_pcie_cleanup_module(void)
 {
-	if (!down_interruptible(&add_remove_card_sem))
-		up(&add_remove_card_sem);
-
 	/* Set the flag as user is removing this module. */
 	user_rmmod = 1;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index 46f99ca..ae3365d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -22,6 +22,7 @@
 #ifndef	_MWIFIEX_PCIE_H
 #define	_MWIFIEX_PCIE_H
 
+#include    <linux/completion.h>
 #include    <linux/pci.h>
 #include    <linux/interrupt.h>
 
@@ -345,6 +346,7 @@ struct pcie_service_card {
 	struct pci_dev *dev;
 	struct mwifiex_adapter *adapter;
 	struct mwifiex_pcie_device pcie;
+	struct completion fw_done;
 
 	u8 txbd_flush;
 	u32 txbd_wrptr;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index ca5b0aa..90f45d9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -49,8 +49,6 @@
 static struct mwifiex_if_ops sdio_ops;
 static unsigned long iface_work_flags;
 
-static struct semaphore add_remove_card_sem;
-
 static struct memory_type_mapping generic_mem_type_map[] = {
 	{"DUMP", NULL, 0, 0xDD},
 };
@@ -116,6 +114,8 @@ static int mwifiex_sdio_probe_of(struct device *dev)
 	if (!card)
 		return -ENOMEM;
 
+	init_completion(&card->fw_done);
+
 	card->func = func;
 	card->device_id = id;
 
@@ -156,7 +156,7 @@ static int mwifiex_sdio_probe_of(struct device *dev)
 		valid_of_node = true;
 	}
 
-	ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
+	ret = mwifiex_add_card(card, &card->fw_done, &sdio_ops,
 			       MWIFIEX_SDIO, &func->dev, valid_of_node);
 	if (ret) {
 		dev_err(&func->dev, "add card failed\n");
@@ -237,6 +237,8 @@ static int mwifiex_sdio_resume(struct device *dev)
 	if (!card)
 		return;
 
+	wait_for_completion(&card->fw_done);
+
 	adapter = card->adapter;
 	if (!adapter || !adapter->priv_num)
 		return;
@@ -254,7 +256,7 @@ static int mwifiex_sdio_resume(struct device *dev)
 		mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
 	}
 
-	mwifiex_remove_card(card->adapter, &add_remove_card_sem);
+	mwifiex_remove_card(adapter);
 }
 
 /*
@@ -2716,14 +2718,11 @@ static void mwifiex_sdio_device_dump(struct mwifiex_adapter *adapter)
 /*
  * This function initializes the SDIO driver.
  *
- * This initiates the semaphore and registers the device with
- * SDIO bus.
+ * This registers the device with SDIO bus.
  */
 static int
 mwifiex_sdio_init_module(void)
 {
-	sema_init(&add_remove_card_sem, 1);
-
 	/* Clear the flag in case user removes the card. */
 	user_rmmod = 0;
 
@@ -2742,9 +2741,6 @@ static void mwifiex_sdio_device_dump(struct mwifiex_adapter *adapter)
 static void
 mwifiex_sdio_cleanup_module(void)
 {
-	if (!down_interruptible(&add_remove_card_sem))
-		up(&add_remove_card_sem);
-
 	/* Set the flag as user is removing this module. */
 	user_rmmod = 1;
 	cancel_work_sync(&sdio_work);
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
index b9fbc5c..cdbf3a3a 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -21,6 +21,7 @@
 #define	_MWIFIEX_SDIO_H
 
 
+#include <linux/completion.h>
 #include <linux/mmc/sdio.h>
 #include <linux/mmc/sdio_ids.h>
 #include <linux/mmc/sdio_func.h>
@@ -238,6 +239,7 @@ struct sdio_mmc_card {
 	struct sdio_func *func;
 	struct mwifiex_adapter *adapter;
 
+	struct completion fw_done;
 	const char *firmware;
 	const struct mwifiex_sdio_card_reg *reg;
 	u8 max_ports;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 889203d..14f89fe 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -24,7 +24,6 @@
 
 static u8 user_rmmod;
 static struct mwifiex_if_ops usb_ops;
-static struct semaphore add_remove_card_sem;
 
 static struct usb_device_id mwifiex_usb_table[] = {
 	/* 8766 */
@@ -386,6 +385,8 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 	if (!card)
 		return -ENOMEM;
 
+	init_completion(&card->fw_done);
+
 	id_vendor = le16_to_cpu(udev->descriptor.idVendor);
 	id_product = le16_to_cpu(udev->descriptor.idProduct);
 	bcd_device = le16_to_cpu(udev->descriptor.bcdDevice);
@@ -475,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
 
 	usb_set_intfdata(intf, card);
 
-	ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
+	ret = mwifiex_add_card(card, &card->fw_done, &usb_ops,
 			       MWIFIEX_USB, &card->udev->dev, false);
 	if (ret) {
 		pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
@@ -601,13 +602,15 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 	struct usb_card_rec *card = usb_get_intfdata(intf);
 	struct mwifiex_adapter *adapter;
 
-	if (!card || !card->adapter) {
-		pr_err("%s: card or card->adapter is NULL\n", __func__);
+	if (!card) {
+		dev_err(&intf->dev, "%s: card is NULL\n", __func__);
 		return;
 	}
 
+	wait_for_completion(&card->fw_done);
+
 	adapter = card->adapter;
-	if (!adapter->priv_num)
+	if (!adapter || !adapter->priv_num)
 		return;
 
 	if (user_rmmod && !adapter->mfg_mode) {
@@ -627,7 +630,7 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 
 	mwifiex_dbg(adapter, FATAL,
 		    "%s: removing card\n", __func__);
-	mwifiex_remove_card(adapter, &add_remove_card_sem);
+	mwifiex_remove_card(adapter);
 
 	usb_put_dev(interface_to_usbdev(intf));
 }
@@ -1200,8 +1203,7 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
 
 /* This function initializes the USB driver module.
  *
- * This initiates the semaphore and registers the device with
- * USB bus.
+ * This registers the device with USB bus.
  */
 static int mwifiex_usb_init_module(void)
 {
@@ -1209,8 +1211,6 @@ static int mwifiex_usb_init_module(void)
 
 	pr_debug("Marvell USB8797 Driver\n");
 
-	sema_init(&add_remove_card_sem, 1);
-
 	ret = usb_register(&mwifiex_usb_driver);
 	if (ret)
 		pr_err("Driver register failed!\n");
@@ -1230,9 +1230,6 @@ static int mwifiex_usb_init_module(void)
  */
 static void mwifiex_usb_cleanup_module(void)
 {
-	if (!down_interruptible(&add_remove_card_sem))
-		up(&add_remove_card_sem);
-
 	/* set the flag as user is removing this module */
 	user_rmmod = 1;
 
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h
index 30e8eb8..e5f204e 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.h
+++ b/drivers/net/wireless/marvell/mwifiex/usb.h
@@ -20,6 +20,7 @@
 #ifndef _MWIFIEX_USB_H
 #define _MWIFIEX_USB_H
 
+#include <linux/completion.h>
 #include <linux/usb.h>
 
 #define USB8XXX_VID		0x1286
@@ -75,6 +76,7 @@ struct usb_card_rec {
 	struct mwifiex_adapter *adapter;
 	struct usb_device *udev;
 	struct usb_interface *intf;
+	struct completion fw_done;
 	u8 rx_cmd_ep;
 	struct urb_context rx_cmd;
 	atomic_t rx_cmd_urb_pending;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 04/11] mwifiex: remove redundant pdev check in suspend/resume handlers
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 02/11] mwifiex: complete blocked power save handshake in main process Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 03/11] mwifiex: resolve races between async FW init (failure) and device removal Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 05/11] mwifiex: don't pretend to resume while remove()'ing Amitkumar Karwar
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Amitkumar Karwar, Brian Norris

to_pci_dev() would just do struct offset arithmetic on struct
device to get 'pdev' pointer. We never get NULL pdev pointer.

Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c4bd64b..6152f08 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -117,14 +117,9 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct pcie_service_card *card;
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (pdev) {
-		card = pci_get_drvdata(pdev);
-		if (!card || !card->adapter) {
-			pr_err("Card or adapter structure is not valid\n");
-			return 0;
-		}
-	} else {
-		pr_err("PCIE device is not specified\n");
+	card = pci_get_drvdata(pdev);
+	if (!card || !card->adapter) {
+		pr_err("Card or adapter structure is not valid\n");
 		return 0;
 	}
 
@@ -162,14 +157,9 @@ static int mwifiex_pcie_resume(struct device *dev)
 	struct pcie_service_card *card;
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	if (pdev) {
-		card = pci_get_drvdata(pdev);
-		if (!card || !card->adapter) {
-			pr_err("Card or adapter structure is not valid\n");
-			return 0;
-		}
-	} else {
-		pr_err("PCIE device is not specified\n");
+	card = pci_get_drvdata(pdev);
+	if (!card || !card->adapter) {
+		dev_err(dev, "Card or adapter structure is not valid\n");
 		return 0;
 	}
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 05/11] mwifiex: don't pretend to resume while remove()'ing
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (2 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 04/11] mwifiex: remove redundant pdev check in suspend/resume handlers Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 06/11] mwifiex: resolve suspend() race with async FW init failure Amitkumar Karwar
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris, Amitkumar Karwar

From: Brian Norris <briannorris@chromium.org>

The device core will not allow suspend() to race with remove().

Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 5 -----
 drivers/net/wireless/marvell/mwifiex/sdio.c | 3 ---
 drivers/net/wireless/marvell/mwifiex/usb.c  | 5 -----
 3 files changed, 13 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 6152f08..a2353f9 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -257,11 +257,6 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 		return;
 
 	if (user_rmmod && !adapter->mfg_mode) {
-#ifdef CONFIG_PM_SLEEP
-		if (adapter->is_suspended)
-			mwifiex_pcie_resume(&pdev->dev);
-#endif
-
 		mwifiex_deauthenticate_all(adapter);
 
 		priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 90f45d9..a2257a4 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -246,9 +246,6 @@ static int mwifiex_sdio_resume(struct device *dev)
 	mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
 
 	if (user_rmmod && !adapter->mfg_mode) {
-		if (adapter->is_suspended)
-			mwifiex_sdio_resume(adapter->dev);
-
 		mwifiex_deauthenticate_all(adapter);
 
 		priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 14f89fe..558a7f1 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -614,11 +614,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 		return;
 
 	if (user_rmmod && !adapter->mfg_mode) {
-#ifdef CONFIG_PM
-		if (adapter->is_suspended)
-			mwifiex_usb_resume(intf);
-#endif
-
 		mwifiex_deauthenticate_all(adapter);
 
 		mwifiex_init_shutdown_fw(mwifiex_get_priv(adapter,
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 06/11] mwifiex: resolve suspend() race with async FW init failure
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (3 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 05/11] mwifiex: don't pretend to resume while remove()'ing Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 07/11] mwifiex: reset card->adapter during device unregister Amitkumar Karwar
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris, Amitkumar Karwar

From: Brian Norris <briannorris@chromium.org>

Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 12 ++++++++++--
 drivers/net/wireless/marvell/mwifiex/sdio.c | 12 ++++++++++--
 drivers/net/wireless/marvell/mwifiex/usb.c  | 12 ++++++++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index a2353f9..4d96683 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -118,12 +118,20 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 
 	card = pci_get_drvdata(pdev);
-	if (!card || !card->adapter) {
-		pr_err("Card or adapter structure is not valid\n");
+	if (!card) {
+		dev_err(dev, "card structure is not valid\n");
 		return 0;
 	}
 
+	/* Might still be loading firmware */
+	wait_for_completion(&card->fw_done);
+
 	adapter = card->adapter;
+	if (!adapter) {
+		dev_err(dev, "adapter is not valid\n");
+		return 0;
+	}
+
 	mwifiex_enable_wake(adapter);
 
 	/* Enable the Host Sleep */
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index a2257a4..39ffe7d 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -285,8 +285,8 @@ static int mwifiex_sdio_suspend(struct device *dev)
 		}
 
 		card = sdio_get_drvdata(func);
-		if (!card || !card->adapter) {
-			pr_err("suspend: invalid card or adapter\n");
+		if (!card) {
+			dev_err(dev, "suspend: invalid card\n");
 			return 0;
 		}
 	} else {
@@ -294,7 +294,15 @@ static int mwifiex_sdio_suspend(struct device *dev)
 		return 0;
 	}
 
+	/* Might still be loading firmware */
+	wait_for_completion(&card->fw_done);
+
 	adapter = card->adapter;
+	if (!adapter) {
+		dev_err(dev, "adapter is not valid\n");
+		return 0;
+	}
+
 	mwifiex_enable_wake(adapter);
 
 	/* Enable the Host Sleep */
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 558a7f1..671702c 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -503,11 +503,19 @@ static int mwifiex_usb_suspend(struct usb_interface *intf, pm_message_t message)
 	struct usb_tx_data_port *port;
 	int i, j;
 
-	if (!card || !card->adapter) {
-		pr_err("%s: card or card->adapter is NULL\n", __func__);
+	if (!card) {
+		dev_err(&intf->dev, "%s: card is NULL\n", __func__);
 		return 0;
 	}
+
+	/* Might still be loading firmware */
+	wait_for_completion(&card->fw_done);
+
 	adapter = card->adapter;
+	if (!adapter) {
+		dev_err(&intf->dev, "card is not valid\n");
+		return 0;
+	}
 
 	if (unlikely(adapter->is_suspended))
 		mwifiex_dbg(adapter, WARN,
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 07/11] mwifiex: reset card->adapter during device unregister
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (4 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 06/11] mwifiex: resolve suspend() race with async FW init failure Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 08/11] mwifiex: usb: handle HS failures Amitkumar Karwar
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Xinming Hu, Amitkumar Karwar, Brian Norris

From: Xinming Hu <huxm@marvell.com>

card->adapter gets initialized in mwifiex_register_dev(). As it's not
cleared in mwifiex_unregister_dev(), we may end up accessing the memory
which is already free in below scenario.

Scenario: Driver initialization is failed due to incorrect firmware or
some other reason. Meanwhile device reboot/unload occurs.

This is safe, now that we've properly synchronized suspend() and
remove() with the FW initialization thread; now that code can simply
check for 'card->adapter == NULL' and exit safely.

Signed-off-by: Xinming Hu <huxm@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 4d96683..1ab366c 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3048,6 +3048,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 			if (card->msi_enable)
 				pci_disable_msi(pdev);
 	       }
+		card->adapter = NULL;
 	}
 }
 
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 39ffe7d..4c4b012 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2019,6 +2019,7 @@ static int mwifiex_alloc_sdio_mpa_buffers(struct mwifiex_adapter *adapter,
 	struct sdio_mmc_card *card = adapter->card;
 
 	if (adapter->card) {
+		card->adapter = NULL;
 		sdio_claim_host(card->func);
 		sdio_disable_func(card->func);
 		sdio_release_host(card->func);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 08/11] mwifiex: usb: handle HS failures
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (5 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 07/11] mwifiex: reset card->adapter during device unregister Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 09/11] mwifiex: sdio: don't check for NULL sdio_func Amitkumar Karwar
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris, Amitkumar Karwar

From: Brian Norris <briannorris@chromium.org>

SDIO and PCIe drivers handle this. Let's imitate it.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/usb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 671702c..8bcfd92 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -521,7 +521,14 @@ static int mwifiex_usb_suspend(struct usb_interface *intf, pm_message_t message)
 		mwifiex_dbg(adapter, WARN,
 			    "Device already suspended\n");
 
-	mwifiex_enable_hs(adapter);
+	/* Enable the Host Sleep */
+	if (!mwifiex_enable_hs(adapter)) {
+		mwifiex_dbg(adapter, ERROR,
+			    "cmd: failed to suspend\n");
+		adapter->hs_enabling = false;
+		return -EFAULT;
+	}
+
 
 	/* 'is_suspended' flag indicates device is suspended.
 	 * It must be set here before the usb_kill_urb() calls. Reason
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 09/11] mwifiex: sdio: don't check for NULL sdio_func
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (6 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 08/11] mwifiex: usb: handle HS failures Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 10/11] mwifiex: stop checking for NULL drvata/intfdata Amitkumar Karwar
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris, Amitkumar Karwar

From: Brian Norris <briannorris@chromium.org>

sdio_func is retrieved via container_of() and should never be NULL.
Checking for NULL just makes the logic more confusing than necessary.
Stop doing that.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/sdio.c | 40 +++++++++++------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 4c4b012..bee7326 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -190,15 +190,10 @@ static int mwifiex_sdio_resume(struct device *dev)
 	struct mwifiex_adapter *adapter;
 	mmc_pm_flag_t pm_flag = 0;
 
-	if (func) {
-		pm_flag = sdio_get_host_pm_caps(func);
-		card = sdio_get_drvdata(func);
-		if (!card || !card->adapter) {
-			pr_err("resume: invalid card or adapter\n");
-			return 0;
-		}
-	} else {
-		pr_err("resume: sdio_func is not specified\n");
+	pm_flag = sdio_get_host_pm_caps(func);
+	card = sdio_get_drvdata(func);
+	if (!card || !card->adapter) {
+		dev_err(dev, "resume: invalid card or adapter\n");
 		return 0;
 	}
 
@@ -274,23 +269,18 @@ static int mwifiex_sdio_suspend(struct device *dev)
 	mmc_pm_flag_t pm_flag = 0;
 	int ret = 0;
 
-	if (func) {
-		pm_flag = sdio_get_host_pm_caps(func);
-		pr_debug("cmd: %s: suspend: PM flag = 0x%x\n",
-			 sdio_func_id(func), pm_flag);
-		if (!(pm_flag & MMC_PM_KEEP_POWER)) {
-			pr_err("%s: cannot remain alive while host is"
-				" suspended\n", sdio_func_id(func));
-			return -ENOSYS;
-		}
+	pm_flag = sdio_get_host_pm_caps(func);
+	pr_debug("cmd: %s: suspend: PM flag = 0x%x\n",
+		 sdio_func_id(func), pm_flag);
+	if (!(pm_flag & MMC_PM_KEEP_POWER)) {
+		dev_err(dev, "%s: cannot remain alive while host is"
+			" suspended\n", sdio_func_id(func));
+		return -ENOSYS;
+	}
 
-		card = sdio_get_drvdata(func);
-		if (!card) {
-			dev_err(dev, "suspend: invalid card\n");
-			return 0;
-		}
-	} else {
-		pr_err("suspend: sdio_func is not specified\n");
+	card = sdio_get_drvdata(func);
+	if (!card) {
+		dev_err(dev, "suspend: invalid card\n");
 		return 0;
 	}
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 10/11] mwifiex: stop checking for NULL drvata/intfdata
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (7 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 09/11] mwifiex: sdio: don't check for NULL sdio_func Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-11 13:10 ` [PATCH v3 11/11] mwifiex: pcie: stop checking for NULL adapter->card Amitkumar Karwar
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris, Amitkumar Karwar

From: Brian Norris <briannorris@chromium.org>

These are never NULL, so stop making people think they might be.

I don't change this for SDIO because SDIO has a racy card-reset handler
that reallocates this struct. I'd rather not touch that mess right now.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 14 +++++---------
 drivers/net/wireless/marvell/mwifiex/usb.c  | 15 +++------------
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 1ab366c..3b6d908 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -118,10 +118,6 @@ static int mwifiex_pcie_suspend(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 
 	card = pci_get_drvdata(pdev);
-	if (!card) {
-		dev_err(dev, "card structure is not valid\n");
-		return 0;
-	}
 
 	/* Might still be loading firmware */
 	wait_for_completion(&card->fw_done);
@@ -166,8 +162,9 @@ static int mwifiex_pcie_resume(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 
 	card = pci_get_drvdata(pdev);
-	if (!card || !card->adapter) {
-		dev_err(dev, "Card or adapter structure is not valid\n");
+
+	if (!card->adapter) {
+		dev_err(dev, "adapter structure is not valid\n");
 		return 0;
 	}
 
@@ -255,8 +252,6 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
 	struct mwifiex_private *priv;
 
 	card = pci_get_drvdata(pdev);
-	if (!card)
-		return;
 
 	wait_for_completion(&card->fw_done);
 
@@ -2249,7 +2244,8 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void *context)
 	}
 
 	card = pci_get_drvdata(pdev);
-	if (!card || !card->adapter) {
+
+	if (!card->adapter) {
 		pr_err("info: %s: card=%p adapter=%p\n", __func__, card,
 		       card ? card->adapter : NULL);
 		goto exit;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 8bcfd92..806ded6 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -503,11 +503,6 @@ static int mwifiex_usb_suspend(struct usb_interface *intf, pm_message_t message)
 	struct usb_tx_data_port *port;
 	int i, j;
 
-	if (!card) {
-		dev_err(&intf->dev, "%s: card is NULL\n", __func__);
-		return 0;
-	}
-
 	/* Might still be loading firmware */
 	wait_for_completion(&card->fw_done);
 
@@ -574,8 +569,9 @@ static int mwifiex_usb_resume(struct usb_interface *intf)
 	struct mwifiex_adapter *adapter;
 	int i;
 
-	if (!card || !card->adapter) {
-		pr_err("%s: card or card->adapter is NULL\n", __func__);
+	if (!card->adapter) {
+		dev_err(&intf->dev, "%s: card->adapter is NULL\n",
+			__func__);
 		return 0;
 	}
 	adapter = card->adapter;
@@ -617,11 +613,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
 	struct usb_card_rec *card = usb_get_intfdata(intf);
 	struct mwifiex_adapter *adapter;
 
-	if (!card) {
-		dev_err(&intf->dev, "%s: card is NULL\n", __func__);
-		return;
-	}
-
 	wait_for_completion(&card->fw_done);
 
 	adapter = card->adapter;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 11/11] mwifiex: pcie: stop checking for NULL adapter->card
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (8 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 10/11] mwifiex: stop checking for NULL drvata/intfdata Amitkumar Karwar
@ 2016-11-11 13:10 ` Amitkumar Karwar
  2016-11-14 19:40 ` [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Brian Norris
  2016-11-18 11:30 ` [v3, " Kalle Valo
  11 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
  To: linux-wireless
  Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
	dmitry.torokhov, Brian Norris

From: Brian Norris <briannorris@chromium.org>

It should never be NULL here, and to think otherwise makes things
confusing.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v2: Same as v1
v3: Below checkpatch warnings are resolved
WARNING: please, no spaces at the start of a line
#50: FILE: drivers/net/wireless/marvell/mwifiex/pcie.c:3037:
+       } else {$

WARNING: please, no spaces at the start of a line
#62: FILE: drivers/net/wireless/marvell/mwifiex/pcie.c:3044:
+       }$
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 53 +++++++++++++----------------
 1 file changed, 23 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 3b6d908..fb2b22d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3021,31 +3021,28 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 {
 	struct pcie_service_card *card = adapter->card;
-	struct pci_dev *pdev;
+	struct pci_dev *pdev = card->dev;
 	int i;
 
-	if (card) {
-		pdev = card->dev;
-		if (card->msix_enable) {
-			for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
-				synchronize_irq(card->msix_entries[i].vector);
+	if (card->msix_enable) {
+		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
+			synchronize_irq(card->msix_entries[i].vector);
 
-			for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
-				free_irq(card->msix_entries[i].vector,
-					 &card->msix_ctx[i]);
+		for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
+			free_irq(card->msix_entries[i].vector,
+				 &card->msix_ctx[i]);
 
-			card->msix_enable = 0;
-			pci_disable_msix(pdev);
-	       } else {
-			mwifiex_dbg(adapter, INFO,
-				    "%s(): calling free_irq()\n", __func__);
-		       free_irq(card->dev->irq, &card->share_irq_ctx);
+		card->msix_enable = 0;
+		pci_disable_msix(pdev);
+	} else {
+		mwifiex_dbg(adapter, INFO,
+			    "%s(): calling free_irq()\n", __func__);
+	       free_irq(card->dev->irq, &card->share_irq_ctx);
 
-			if (card->msi_enable)
-				pci_disable_msi(pdev);
-	       }
-		card->adapter = NULL;
+		if (card->msi_enable)
+			pci_disable_msi(pdev);
 	}
+	card->adapter = NULL;
 }
 
 /* This function initializes the PCI-E host memory space, WCB rings, etc.
@@ -3128,18 +3125,14 @@ static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter)
 	adapter->seq_num = 0;
 	adapter->tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_4K;
 
-	if (card) {
-		if (reg->sleep_cookie)
-			mwifiex_pcie_delete_sleep_cookie_buf(adapter);
-
-		mwifiex_pcie_delete_cmdrsp_buf(adapter);
-		mwifiex_pcie_delete_evtbd_ring(adapter);
-		mwifiex_pcie_delete_rxbd_ring(adapter);
-		mwifiex_pcie_delete_txbd_ring(adapter);
-		card->cmdrsp_buf = NULL;
-	}
+	if (reg->sleep_cookie)
+		mwifiex_pcie_delete_sleep_cookie_buf(adapter);
 
-	return;
+	mwifiex_pcie_delete_cmdrsp_buf(adapter);
+	mwifiex_pcie_delete_evtbd_ring(adapter);
+	mwifiex_pcie_delete_rxbd_ring(adapter);
+	mwifiex_pcie_delete_txbd_ring(adapter);
+	card->cmdrsp_buf = NULL;
 }
 
 static struct mwifiex_if_ops pcie_ops = {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (9 preceding siblings ...)
  2016-11-11 13:10 ` [PATCH v3 11/11] mwifiex: pcie: stop checking for NULL adapter->card Amitkumar Karwar
@ 2016-11-14 19:40 ` Brian Norris
  2016-11-17 12:41   ` Kalle Valo
  2016-11-18 11:30 ` [v3, " Kalle Valo
  11 siblings, 1 reply; 16+ messages in thread
From: Brian Norris @ 2016-11-14 19:40 UTC (permalink / raw)
  To: Amitkumar Karwar, Kalle Valo
  Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
	dmitry.torokhov, Shengzhen Li

Hi Amit, Kalle,

On Fri, Nov 11, 2016 at 06:40:08PM +0530, Amitkumar Karwar wrote:
> From: Shengzhen Li <szli@marvell.com>
> 
> We may get SLEEP event from firmware even if TXDone interrupt
> for last Tx packet is still pending. In this case, we may
> end up accessing PCIe memory for handling TXDone after power
> save handshake is completed. This causes kernel crash with
> external abort.
> 
> This patch will only allow downloading sleep confirm
> when no tx done interrupt is pending in the hardware.
> 
> Signed-off-by: Cathy Luo <cluo@marvell.com>
> Signed-off-by: Shengzhen Li <szli@marvell.com>
> Tested-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: address format issues(Brain)
> RESEND v2(Applicable for complete patch series):
>     1) Fixed syntax issue "changelog not placed after the  Sign-offs"
>        pointed by Brian.
>     2) Dropped "[v2,03/12] mwifiex: don't do unbalanced free()'ing in
>        cleanup_if()" patch in this series. It was already sent by Brian
>        separately.
> v3: Same as RESEND v2.
> 
> Hi Kalle,
> 
> There are multiple mwifiex patches under review. I want you consider them
> in following sequence(first being oldest) to avoid conflicts

Thanks for doing this! It's a little confusing about what's outstanding
at the moment (and I think I was just confused on a review a bit ago; I
wasn't 100% sure what it was based on), so this listing helps.

If it helps, I'll put my comments here, since I've reviewed most of
these:

> [v3] mwifiex: report wakeup for wowlan

Reviewed, SGMT.

> mwifiex: add power save parameters in hs_cfg cmd

Didn't review. No comment.

> [2/2] mwifiex: ignore calibration data failure (Note: 1/2 has dropped)

Didn't review. But FWIW, Kalle expressed a preference for full series,
not partial.

> [v6] mwifiex: parse device tree node for PCIe

This one is marked Deferred in patchwork, and I had some comments about
it, since it introduced a double-free issue. I'd prefer it get fixed and
resent, and I expect Kalle is also waiting for this.

> [v2,1/3] mwifiex: Allow mwifiex early access to device structure
> [v2,2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties
> [v2,3/3] mwifiex: Enable WoWLAN for both sdio and pcie

You sent v3 for the above, and those LGTM (I provided my review). I was
probably also confused because they were based on the above "[v6]
mwifiex: parse device tree node for PCIe", which was not completely
correct.

> mwifiex: don't do unbalanced free()'ing in cleanup_if()
> mwifiex: printk() overflow with 32-byte SSIDs
> mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels()

I wrote or reviewed the above 3. LGTM.

> [v3,01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
> [v3,02/11] mwifiex: complete blocked power save handshake in main process
> [v3,03/11] mwifiex: resolve races between async FW init (failure) and device removal
> [v3,04/11] mwifiex: remove redundant pdev check in suspend/resume handlers
> [v3,05/11] mwifiex: don't pretend to resume while remove()'ing
> [v3,06/11] mwifiex: resolve suspend() race with async FW init failure
> [v3,07/11] mwifiex: reset card->adapter during device unregister
> [v3,08/11] mwifiex: usb: handle HS failures
> [v3,09/11] mwifiex: sdio: don't check for NULL sdio_func
> [v3,10/11] mwifiex: stop checking for NULL drvata/intfdata
> [v3,11/11] mwifiex: pcie: stop checking for NULL adapter->card

For this entire series, I looked over them again (and I wrote several in
the first place), so for all 11:

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

> ---
>  drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++--
>  drivers/net/wireless/marvell/mwifiex/init.c   | 1 +
>  drivers/net/wireless/marvell/mwifiex/main.h   | 1 +
>  drivers/net/wireless/marvell/mwifiex/pcie.c   | 5 +++++
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
[...] 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
  2016-11-14 19:40 ` [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Brian Norris
@ 2016-11-17 12:41   ` Kalle Valo
  2016-11-21 17:24     ` Brian Norris
  0 siblings, 1 reply; 16+ messages in thread
From: Kalle Valo @ 2016-11-17 12:41 UTC (permalink / raw)
  To: Brian Norris
  Cc: Amitkumar Karwar, linux-wireless, Cathy Luo, Nishant Sarmukadam,
	rajatja, dmitry.torokhov, Shengzhen Li

Brian Norris <briannorris@chromium.org> writes:

> On Fri, Nov 11, 2016 at 06:40:08PM +0530, Amitkumar Karwar wrote:
>
>> There are multiple mwifiex patches under review. I want you consider them
>> in following sequence(first being oldest) to avoid conflicts
>
> Thanks for doing this! It's a little confusing about what's outstanding
> at the moment (and I think I was just confused on a review a bit ago; I
> wasn't 100% sure what it was based on), so this listing helps.
>
> If it helps, I'll put my comments here, since I've reviewed most of
> these:
>
>> [v3] mwifiex: report wakeup for wowlan
>
> Reviewed, SGMT.
>
>> mwifiex: add power save parameters in hs_cfg cmd
>
> Didn't review. No comment.
>
>> [2/2] mwifiex: ignore calibration data failure (Note: 1/2 has dropped)
>
> Didn't review. But FWIW, Kalle expressed a preference for full series,
> not partial.

You are correct, but dropping patches in patchwork is easy so I usually
can do that myself. Changing or adding patches to a patchset is the
difficult part.

So I dropped patch 1 now.

>> [v6] mwifiex: parse device tree node for PCIe
>
> This one is marked Deferred in patchwork, and I had some comments about
> it, since it introduced a double-free issue. I'd prefer it get fixed and
> resent, and I expect Kalle is also waiting for this.

Correct, I dropped v6.

>> [v2,1/3] mwifiex: Allow mwifiex early access to device structure
>> [v2,2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties
>> [v2,3/3] mwifiex: Enable WoWLAN for both sdio and pcie
>
> You sent v3 for the above, and those LGTM (I provided my review). I was
> probably also confused because they were based on the above "[v6]
> mwifiex: parse device tree node for PCIe", which was not completely
> correct.

v4 of this patchset is now "Under Review", which in practise means that
the patches are pending for commit. (Too bad that patchwork doesn't have
a "Pending" state, so I have to use "Under Review" instead)

>> mwifiex: don't do unbalanced free()'ing in cleanup_if()
>> mwifiex: printk() overflow with 32-byte SSIDs
>> mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels()
>
> I wrote or reviewed the above 3. LGTM.

The first is now "Under Review" and the last two I have already applied.

>> [v3,01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
>> [v3,02/11] mwifiex: complete blocked power save handshake in main process
>> [v3,03/11] mwifiex: resolve races between async FW init (failure) and device removal
>> [v3,04/11] mwifiex: remove redundant pdev check in suspend/resume handlers
>> [v3,05/11] mwifiex: don't pretend to resume while remove()'ing
>> [v3,06/11] mwifiex: resolve suspend() race with async FW init failure
>> [v3,07/11] mwifiex: reset card->adapter during device unregister
>> [v3,08/11] mwifiex: usb: handle HS failures
>> [v3,09/11] mwifiex: sdio: don't check for NULL sdio_func
>> [v3,10/11] mwifiex: stop checking for NULL drvata/intfdata
>> [v3,11/11] mwifiex: pcie: stop checking for NULL adapter->card
>
> For this entire series, I looked over them again (and I wrote several in
> the first place), so for all 11:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

These 11 are now "Under Review".

So to summarise, this is what I'm planning to commit (it's sorted by
date but I try to follow the order Amit specified when I commit these):

[  1] mwifiex: add power save parameters in hs_cfg cmd             2016-10-14 Amitkumar Ka Under Review
[  2] [2/2] mwifiex: ignore calibration data failure               2016-10-21 Amitkumar Ka Under Review
[  3] mwifiex: don't do unbalanced free()'ing in cleanup_if()      2016-10-26 Brian Norris Under Review
[  4] [v3,01/11] mwifiex: check tx_hw_pending before downloadin... 2016-11-11 Amitkumar Ka Under Review
[  5] [v3,02/11] mwifiex: complete blocked power save handshake... 2016-11-11 Amitkumar Ka Under Review
[  6] [v3,03/11] mwifiex: resolve races between async FW init (... 2016-11-11 Amitkumar Ka Under Review
[  7] [v3,04/11] mwifiex: remove redundant pdev check in suspen... 2016-11-11 Amitkumar Ka Under Review
[  8] [v3,05/11] mwifiex: don't pretend to resume while remove(... 2016-11-11 Amitkumar Ka Under Review
[  9] [v3,06/11] mwifiex: resolve suspend() race with async FW.... 2016-11-11 Amitkumar Ka Under Review
[ 10] [v3,07/11] mwifiex: reset card->adapter during device unr... 2016-11-11 Amitkumar Ka Under Review
[ 11] [v3,08/11] mwifiex: usb: handle HS failures                  2016-11-11 Amitkumar Ka Under Review
[ 12] [v3,09/11] mwifiex: sdio: don't check for NULL sdio_func     2016-11-11 Amitkumar Ka Under Review
[ 13] [v3,10/11] mwifiex: stop checking for NULL drvata/intfdata   2016-11-11 Amitkumar Ka Under Review
[ 14] [v3,11/11] mwifiex: pcie: stop checking for NULL adapter-... 2016-11-11 Amitkumar Ka Under Review
[ 15] [v3,1/3] mwifiex: Allow mwifiex early access to device st... 2016-11-14 Amitkumar Ka Under Review
[ 16] [v3,2/3] mwifiex: Introduce mwifiex_probe_of() to parse c... 2016-11-14 Amitkumar Ka Under Review
[ 17] [v3,3/3] mwifiex: Enable WoWLAN for both sdio and pcie       2016-11-14 Amitkumar Ka Under Review
[ 18] [v4,1/3] mwifiex: Allow mwifiex early access to device st... 2016-11-15 Amitkumar Ka Under Review
[ 19] [v4,2/3] mwifiex: Introduce mwifiex_probe_of() to parse c... 2016-11-15 Amitkumar Ka Under Review
[ 20] [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie       2016-11-15 Amitkumar Ka Under Review

Patchwork link for the same:

https://patchwork.kernel.org/project/linux-wireless/list/?state=2&q=mwifiex

Does that look ok?

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [v3, 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
  2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
                   ` (10 preceding siblings ...)
  2016-11-14 19:40 ` [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Brian Norris
@ 2016-11-18 11:30 ` Kalle Valo
  2016-11-18 13:59   ` Amitkumar Karwar
  11 siblings, 1 reply; 16+ messages in thread
From: Kalle Valo @ 2016-11-18 11:30 UTC (permalink / raw)
  To: Amitkumar Karwar
  Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
	briannorris, dmitry.torokhov, Shengzhen Li, Amitkumar Karwar

Amitkumar Karwar <akarwar@marvell.com> wrote:
> From: Shengzhen Li <szli@marvell.com>
> 
> We may get SLEEP event from firmware even if TXDone interrupt
> for last Tx packet is still pending. In this case, we may
> end up accessing PCIe memory for handling TXDone after power
> save handshake is completed. This causes kernel crash with
> external abort.
> 
> This patch will only allow downloading sleep confirm
> when no tx done interrupt is pending in the hardware.
> 
> Signed-off-by: Cathy Luo <cluo@marvell.com>
> Signed-off-by: Shengzhen Li <szli@marvell.com>
> Tested-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Reviewed-by: Brian Norris <briannorris@chromium.org>

(A note to myself)

Depends on this patchset:

[ 12] [v4,1/3] mwifiex: Allow mwifiex early access to device st... 2016-11-15 Amitkumar Ka Awaiting Upstream
[ 13] [v4,2/3] mwifiex: Introduce mwifiex_probe_of() to parse c... 2016-11-15 Amitkumar Ka Awaiting Upstream
[ 14] [v4,3/3] mwifiex: Enable WoWLAN for both sdio and pcie       2016-11-15 Amitkumar Ka Awaiting Upstream

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

Documentation about submitting wireless patches and checking status
from patchwork:

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [v3, 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
  2016-11-18 11:30 ` [v3, " Kalle Valo
@ 2016-11-18 13:59   ` Amitkumar Karwar
  0 siblings, 0 replies; 16+ messages in thread
From: Amitkumar Karwar @ 2016-11-18 13:59 UTC (permalink / raw)
  To: Kalle Valo
  Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
	briannorris, dmitry.torokhov, Shengzhen Li

SGkgS2FsbGUsDQoNCj4gRnJvbTogbGludXgtd2lyZWxlc3Mtb3duZXJAdmdlci5rZXJuZWwub3Jn
IFttYWlsdG86bGludXgtd2lyZWxlc3MtDQo+IG93bmVyQHZnZXIua2VybmVsLm9yZ10gT24gQmVo
YWxmIE9mIEthbGxlIFZhbG8NCj4gU2VudDogRnJpZGF5LCBOb3ZlbWJlciAxOCwgMjAxNiA1OjAx
IFBNDQo+IFRvOiBBbWl0a3VtYXIgS2Fyd2FyDQo+IENjOiBsaW51eC13aXJlbGVzc0B2Z2VyLmtl
cm5lbC5vcmc7IENhdGh5IEx1bzsgTmlzaGFudCBTYXJtdWthZGFtOw0KPiByYWphdGphQGdvb2ds
ZS5jb207IGJyaWFubm9ycmlzQGdvb2dsZS5jb207IGRtaXRyeS50b3Jva2hvdkBnbWFpbC5jb207
DQo+IFNoZW5nemhlbiBMaTsgQW1pdGt1bWFyIEthcndhcg0KPiBTdWJqZWN0OiBSZTogW3YzLCAw
MS8xMV0gbXdpZmlleDogY2hlY2sgdHhfaHdfcGVuZGluZyBiZWZvcmUNCj4gZG93bmxvYWRpbmcg
c2xlZXAgY29uZmlybQ0KPiANCj4gQW1pdGt1bWFyIEthcndhciA8YWthcndhckBtYXJ2ZWxsLmNv
bT4gd3JvdGU6DQo+ID4gRnJvbTogU2hlbmd6aGVuIExpIDxzemxpQG1hcnZlbGwuY29tPg0KPiA+
DQo+ID4gV2UgbWF5IGdldCBTTEVFUCBldmVudCBmcm9tIGZpcm13YXJlIGV2ZW4gaWYgVFhEb25l
IGludGVycnVwdCBmb3INCj4gbGFzdA0KPiA+IFR4IHBhY2tldCBpcyBzdGlsbCBwZW5kaW5nLiBJ
biB0aGlzIGNhc2UsIHdlIG1heSBlbmQgdXAgYWNjZXNzaW5nDQo+IFBDSWUNCj4gPiBtZW1vcnkg
Zm9yIGhhbmRsaW5nIFRYRG9uZSBhZnRlciBwb3dlciBzYXZlIGhhbmRzaGFrZSBpcyBjb21wbGV0
ZWQuDQo+ID4gVGhpcyBjYXVzZXMga2VybmVsIGNyYXNoIHdpdGggZXh0ZXJuYWwgYWJvcnQuDQo+
ID4NCj4gPiBUaGlzIHBhdGNoIHdpbGwgb25seSBhbGxvdyBkb3dubG9hZGluZyBzbGVlcCBjb25m
aXJtIHdoZW4gbm8gdHggZG9uZQ0KPiA+IGludGVycnVwdCBpcyBwZW5kaW5nIGluIHRoZSBoYXJk
d2FyZS4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IENhdGh5IEx1byA8Y2x1b0BtYXJ2ZWxsLmNv
bT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBTaGVuZ3poZW4gTGkgPHN6bGlAbWFydmVsbC5jb20+DQo+
ID4gVGVzdGVkLWJ5OiBYaW5taW5nIEh1IDxodXhtQG1hcnZlbGwuY29tPg0KPiA+IFNpZ25lZC1v
ZmYtYnk6IEFtaXRrdW1hciBLYXJ3YXIgPGFrYXJ3YXJAbWFydmVsbC5jb20+DQo+ID4gUmV2aWV3
ZWQtYnk6IEJyaWFuIE5vcnJpcyA8YnJpYW5ub3JyaXNAY2hyb21pdW0ub3JnPg0KPiANCj4gKEEg
bm90ZSB0byBteXNlbGYpDQo+IA0KPiBEZXBlbmRzIG9uIHRoaXMgcGF0Y2hzZXQ6DQo+IA0KPiBb
IDEyXSBbdjQsMS8zXSBtd2lmaWV4OiBBbGxvdyBtd2lmaWV4IGVhcmx5IGFjY2VzcyB0byBkZXZp
Y2Ugc3QuLi4NCj4gMjAxNi0xMS0xNSBBbWl0a3VtYXIgS2EgQXdhaXRpbmcgVXBzdHJlYW0gWyAx
M10gW3Y0LDIvM10gbXdpZmlleDoNCj4gSW50cm9kdWNlIG13aWZpZXhfcHJvYmVfb2YoKSB0byBw
YXJzZSBjLi4uIDIwMTYtMTEtMTUgQW1pdGt1bWFyIEthDQo+IEF3YWl0aW5nIFVwc3RyZWFtDQo+
IFsgMTRdIFt2NCwzLzNdIG13aWZpZXg6IEVuYWJsZSBXb1dMQU4gZm9yIGJvdGggc2RpbyBhbmQg
cGNpZQ0KPiAyMDE2LTExLTE1IEFtaXRrdW1hciBLYSBBd2FpdGluZyBVcHN0cmVhbQ0KPiANCg0K
VGhlcmUgaXMgYSBtaW5vciBjb25mbGljdCB3aGlsZSBhcHBseWluZyBbdjMsIDMvMTFdIG9uIHRv
cCBvZiBhYm92ZSBwYXRjaCBzZXJpZXMuDQpJIHdpbGwgc3VibWl0IHY0IHBhdGNoIHNlcmllcygx
MSBwYXRjaGVzKSB3aXRoIGNvcnJlY3Rpb24uDQoNClJlZ2FyZHMsDQpBbWl0a3VtYXINCg==

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
  2016-11-17 12:41   ` Kalle Valo
@ 2016-11-21 17:24     ` Brian Norris
  0 siblings, 0 replies; 16+ messages in thread
From: Brian Norris @ 2016-11-21 17:24 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Amitkumar Karwar, linux-wireless, Cathy Luo, Nishant Sarmukadam,
	rajatja, dmitry.torokhov, Shengzhen Li

On Thu, Nov 17, 2016 at 02:41:11PM +0200, Kalle Valo wrote:
> Patchwork link for the same:
> 
> https://patchwork.kernel.org/project/linux-wireless/list/?state=2&q=mwifiex
> 
> Does that look ok?

FWIW, your merges (since you made the above comments) look sane to me.
Thanks!

Brian

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-11-21 17:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-11 13:10 [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 02/11] mwifiex: complete blocked power save handshake in main process Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 03/11] mwifiex: resolve races between async FW init (failure) and device removal Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 04/11] mwifiex: remove redundant pdev check in suspend/resume handlers Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 05/11] mwifiex: don't pretend to resume while remove()'ing Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 06/11] mwifiex: resolve suspend() race with async FW init failure Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 07/11] mwifiex: reset card->adapter during device unregister Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 08/11] mwifiex: usb: handle HS failures Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 09/11] mwifiex: sdio: don't check for NULL sdio_func Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 10/11] mwifiex: stop checking for NULL drvata/intfdata Amitkumar Karwar
2016-11-11 13:10 ` [PATCH v3 11/11] mwifiex: pcie: stop checking for NULL adapter->card Amitkumar Karwar
2016-11-14 19:40 ` [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm Brian Norris
2016-11-17 12:41   ` Kalle Valo
2016-11-21 17:24     ` Brian Norris
2016-11-18 11:30 ` [v3, " Kalle Valo
2016-11-18 13:59   ` Amitkumar Karwar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.