| Message ID | 20191112124021.8718-2-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mmc: Fixup HW reset for SDIO cards | expand |
Ulf Hansson <ulf.hansson@linaro.org> writes: > 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. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Look good to me. Ulf, I assume you are going to take this so here's my ack: Acked-by: Kalle Valo <kvalo@codeaurora.org> But let me know if I should take it instead, whatever works the best for you.
On Thu, 14 Nov 2019 at 16:13, Kalle Valo <kvalo@codeaurora.org> wrote: > > Ulf Hansson <ulf.hansson@linaro.org> writes: > > > 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. > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Tested-by: Douglas Anderson <dianders@chromium.org> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Look good to me. Ulf, I assume you are going to take this so here's my > ack: > > Acked-by: Kalle Valo <kvalo@codeaurora.org> Thanks, I have queued it via my tree this time. Kind regards Uffe
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c index a9657ae6d782..d14e55e3c9da 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -631,6 +631,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context) mwifiex_drv_get_driver_version(adapter, fmt, sizeof(fmt) - 1); mwifiex_dbg(adapter, MSG, "driver_version = %s\n", fmt); + adapter->is_up = true; goto done; err_add_intf: @@ -1469,6 +1470,7 @@ int mwifiex_shutdown_sw(struct mwifiex_adapter *adapter) mwifiex_deauthenticate(priv, NULL); mwifiex_uninit_sw(adapter); + adapter->is_up = false; if (adapter->if_ops.down_dev) adapter->if_ops.down_dev(adapter); @@ -1730,7 +1732,8 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter) if (!adapter) return 0; - mwifiex_uninit_sw(adapter); + if (adapter->is_up) + mwifiex_uninit_sw(adapter); if (adapter->irq_wakeup >= 0) device_init_wakeup(adapter->dev, false); diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 095837fba300..547ff3c578ee 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -1017,6 +1017,7 @@ struct mwifiex_adapter { /* For synchronizing FW initialization with device lifecycle. */ struct completion *fw_done; + bool is_up; bool ext_scan; u8 fw_api_ver; diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 24c041dad9f6..fec38b6e86ff 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_up) + return -EBUSY; + mwifiex_enable_wake(adapter); /* Enable the Host Sleep */ @@ -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); - - ret = mwifiex_reinit_sw(adapter); - if (ret) - dev_err(&func->dev, "reinit failed: %d\n", ret); + switch (ret) { + case 1: + dev_dbg(&func->dev, "SDIO HW reset asynchronous\n"); + complete_all(adapter->fw_done); + break; + case 0: + ret = mwifiex_reinit_sw(adapter); + if (ret) + dev_err(&func->dev, "reinit failed: %d\n", ret); + break; + default: + dev_err(&func->dev, "SDIO HW reset failed: %d\n", ret); + break; + } } /* This function read/write firmware */