On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > > > > The SDIO HW reset procedure in mwifiex_sdio_card_reset_work() is broken, > > when the SDIO card is shared with another SDIO func driver. This is the > > case when the Bluetooth btmrvl driver is being used in combination with > > mwifiex. More precisely, when mwifiex_sdio_card_reset_work() runs to resets > > the SDIO card, the btmrvl driver doesn't get notified about it. Beyond that > > point, the btmrvl driver will fail to communicate with the SDIO card. > > > > This is a generic problem for SDIO func drivers sharing an SDIO card, which > > are about to be addressed in subsequent changes to the mmc core and the > > mmc_hw_reset() interface. In principle, these changes means the > > mmc_hw_reset() interface starts to return 1 if the are multiple drivers for > > the SDIO card, as to indicate to the caller that the reset needed to be > > scheduled asynchronously through a hotplug mechanism of the SDIO card. > > > > Let's prepare the mwifiex driver to support the upcoming new behaviour of > > mmc_hw_reset(), which means extending the mwifiex_sdio_card_reset_work() to > > support the asynchronous SDIO HW reset path. This also means, we need to > > allow the ->remove() callback to run, without waiting for the FW to be > > loaded. Additionally, during system suspend, mwifiex_sdio_suspend() may be > > called when a reset has been scheduled, but waiting to be executed. In this > > scenario let's simply return -EBUSY to abort the suspend process, as to > > allow the reset to be completed first. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > > --- > > drivers/net/wireless/marvell/mwifiex/main.c | 6 +++- > > drivers/net/wireless/marvell/mwifiex/main.h | 1 + > > drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++------- > > 3 files changed, 28 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c > > index a9657ae6d782..dbdbdd6769a9 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/main.c > > +++ b/drivers/net/wireless/marvell/mwifiex/main.c > > @@ -76,6 +76,7 @@ static int mwifiex_register(void *card, struct device *dev, > > *padapter = adapter; > > adapter->dev = dev; > > adapter->card = card; > > + adapter->is_adapter_up = false; > > Probably not needed. The 'adapter' was kzalloc-ed a few lines above > and there's no need to re-init to 0. Right, let me re-spin and drop this. > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c > > index 24c041dad9f6..2417c94c29c0 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c > > @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev) > > return 0; > > } > > > > + if (!adapter->is_adapter_up) > > + return -EBUSY; > > I'm moderately concerned that there might be cases where firmware > never got loaded but we could suspend/resume OK. ...and now we never > will? I'm not familiar enough with the code to know if this is a real > concern, so I guess we can do this and then see... There is a completion variable that is used to make sure the firmware is loaded, before the mwifiex driver runs ->suspend|remove(). This is needed, because during ->probe() the FW will be loaded asynchronously, hence both mwifiex_sdio_remove() and mwifiex_sdio_suspend(), may be called while waiting for the FW to be loaded. If a HW reset has been scheduled but not completed, which would be the case if mmc_hw_reset() gets called after mmc_pm_notify() with a PM_SUSPEND_PREPARE. This is because mmc_pm_notify() then disables the rescan work, but then re-kicks/enables it at PM_POST_SUSPEND (after the system has resumed). Returning -EBUSY, should allow the mmc rescan work to be completed when the system have resumed. Of course, one could also considering using pm_wakeup_event(), in case mmc_hw_reset() needed to schedule the reset, as to prevent the system for suspending for a small amount of time. As to make sure the rescan work, gets to run. But I am not sure that's needed here. > > > > @@ -2220,22 +2223,30 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter) > > struct sdio_func *func = card->func; > > int ret; > > > > + /* Prepare the adapter for the reset. */ > > mwifiex_shutdown_sw(adapter); > > + clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags); > > + clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags); > > > > - /* power cycle the adapter */ > > + /* Run a HW reset of the SDIO interface. */ > > sdio_claim_host(func); > > - mmc_hw_reset(func->card->host); > > + ret = mmc_hw_reset(func->card->host); > > sdio_release_host(func); > > > > - /* Previous save_adapter won't be valid after this. We will cancel > > - * pending work requests. > > - */ > > - clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags); > > - clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags); > > I don't know enough about the clearing of these bits to confirm that > it's OK to move their clearing to be before the mmc_hw_reset(). > Possibly +Brian Norris does? That shouldn't matter, because we are running in the path of the mwifiex_sdio_work(), as work from the system_wq. Unless I am mistaken, only one work of the type mwifiex_sdio_work() can execute at the same time. By clearing these bits, we want to cancel any potential recently scheduled work. It should matter if that's done before or after mmc_hw_reset(). Moreover, this change makes it more consistent with how the pcie driver clears the bits. > > > I can't promise that I didn't miss anything, but to the best of my > knowledge this is good now: > > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx> Thanks! Finally, if you want to verify that the above system suspend path works fine, you could change the call to "_mmc_detect_change(host, 0, false)" in mmc_sdio_hw_reset(), into "_mmc_detect_change(host, msecs_to_jiffies(30000), false)", in patch3. This should leave you a 30s window of where you can try to system suspend the platform, while also waiting for the scheduled reset to be completed. Kind regards Uffe
