Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card
From: Brian Norris <briannorris@chromium.org>
Date: 2016-10-24 20:23:38
On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote:
quoted hunk ↗ jump to hunk
From: Xinming Hu <redacted> This patch ensures to wait for firmware dump completion in mwifiex_remove_card(). For sdio interface, reset_trigger variable is used to identify if mwifiex_sdio_remove() is called by sdio_work during reset or the call is from sdio subsystem. This patch fixes a kernel crash observed during reboot when firmware dump operation is in process. Signed-off-by: Xinming Hu <redacted> Signed-off-by: Amitkumar Karwar <redacted> --- drivers/net/wireless/marvell/mwifiex/pcie.c | 2 ++ drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index 986bf07..4512e86 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c@@ -528,6 +528,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev) if (!adapter || !adapter->priv_num) return; + cancel_work_sync(&pcie_work);
Is there a good reason you have to cancel the work? What if you were just to finish it (i.e., flush_work())? In any case, I think this is fine, so for the PCIe bits: Reviewed-by: Brian Norris <briannorris@chromium.org>
quoted hunk ↗ jump to hunk
+ if (user_rmmod && !adapter->mfg_mode) { #ifdef CONFIG_PM_SLEEP if (adapter->is_suspended)diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 4cad1c2..f974260 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c@@ -46,6 +46,15 @@ */ static u8 user_rmmod; +/* reset_trigger variable is used to identify if mwifiex_sdio_remove() + * is called by sdio_work during reset or the call is from sdio subsystem. + * We will cancel sdio_work only if the call is from sdio subsystem. + */ +static u8 reset_triggered; + +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;@@ -289,6 +298,9 @@ mwifiex_sdio_remove(struct sdio_func *func) if (!adapter || !adapter->priv_num) return; + if (!reset_triggered) + cancel_work_sync(&sdio_work); + mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num); if (user_rmmod && !adapter->mfg_mode) {@@ -2290,7 +2302,9 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card) * discovered and initializes them from scratch. */ + reset_triggered = 1; mwifiex_sdio_remove(func); + reset_triggered = 0;
Boy that's ugly! Couldn't you just create something like __mwifiex_sdio_remove(), which does everything except the cancel_work_sync()? Then you'd do this for the .remove() callback: cancel_work_sync(&sdio_work); __mwifiex_sdio_remove(func); and for mwifiex_recreate_adapter() you'd just call __mwifiex_sdio_remove()? The static variable that simply serves as a (non-reentrant) function call parameter seems like a really poor alternative to this simple refactoring. Or you could just address the TODO in this function, and you wouldn't have to do this dance at all... Brian
quoted hunk ↗ jump to hunk
/* power cycle the adapter */ sdio_claim_host(func);@@ -2621,7 +2635,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