Hi Brian, > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: Tuesday, October 25, 2016 1:54 AM > To: Amitkumar Karwar > Cc: linux-wireless@xxxxxxxxxxxxxxx; Cathy Luo; Nishant Sarmukadam; > dmitry.torokhov@xxxxxxxxx; Xinming Hu > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in > remove_card > > On Mon, Oct 24, 2016 at 07:51:32PM +0530, Amitkumar Karwar wrote: > > From: Xinming Hu <huxm@xxxxxxxxxxx> > > > > 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 <huxm@xxxxxxxxxxx> > > Signed-off-by: Amitkumar Karwar <akarwar@xxxxxxxxxxx> > > --- > > 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())? I assume if work is running, cancel_work_sync() will wait until completion. Otherwise it will cancel queued work. Firmware dump takes 20-30 seconds to complete. I think, it's ok to cancel it if work is just queued and not running yet. Reboot taking significant time due to our wait in remove handler may not be a good experience from end user point of view. If you prefer flush_work(), I can use the same. > > In any case, I think this is fine, so for the PCIe bits: > > Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx> > > > + > > 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. Thanks a lot for the suggestion. I will use this approach in updated version. Regards, Amitkumar
