diff mbox series

[5/7] mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume

Message ID 20190618153448.27145-6-ulf.hansson@linaro.org (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series mmc: sdio: Various fixes/improvements for SDIO PM | expand

Commit Message

Ulf Hansson June 18, 2019, 3:34 p.m. UTC
It looks like the original idea behind always doing a re-initialization of
a removable SDIO card during system resume in mmc_sdio_resume(), is to try
to play safe to detect whether the card has been removed.

However, this seems like a really a bad idea as it will most likely screw
things up, especially when the card is expected to remain powered on during
system suspend by the SDIO func driver.

Let's fix this, simply by trusting that the detect work checks if the card
is alive and inserted, which is being scheduled at the PM_POST_SUSPEND
notification anyway.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Doug Anderson July 9, 2019, 9:26 p.m. UTC | #1
Hi,

On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> It looks like the original idea behind always doing a re-initialization of
> a removable SDIO card during system resume in mmc_sdio_resume(), is to try
> to play safe to detect whether the card has been removed.
>
> However, this seems like a really a bad idea as it will most likely screw
> things up, especially when the card is expected to remain powered on during
> system suspend by the SDIO func driver.
>
> Let's fix this, simply by trusting that the detect work checks if the card
> is alive and inserted, which is being scheduled at the PM_POST_SUSPEND
> notification anyway.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

I'm not sure if it's even worth mentioning since the device I tested
was rk3288-veyron (both WiFi variants) and we have a non-removable,
powered-in-suspend, wakeup-disabled card.  That means it isn't
affected at all by your change.  ...but I suppose I can at least
confirm that your change didn't break me if that's worth anything.
:-P

Tested-by: Douglas Anderson <dianders@chromium.org>

I would also say that, though I don't have any history here, your
patch seems reasonable to me.  So you can add a:

Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff mbox series

Patch

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index a9bfcae8db5b..945416c53b56 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -982,7 +982,11 @@  static int mmc_sdio_resume(struct mmc_host *host)
 	/* Basic card reinitialization. */
 	mmc_claim_host(host);
 
-	/* Restore power if needed */
+	/*
+	 * Restore power and reinitialize the card when needed. Note that a
+	 * removable card is checked from a detect work later on in the resume
+	 * process.
+	 */
 	if (!mmc_card_keep_power(host)) {
 		mmc_power_up(host, host->card->ocr);
 		/*
@@ -996,12 +1000,8 @@  static int mmc_sdio_resume(struct mmc_host *host)
 			pm_runtime_set_active(&host->card->dev);
 			pm_runtime_enable(&host->card->dev);
 		}
-	}
-
-	/* No need to reinitialize powered-resumed nonremovable cards */
-	if (mmc_card_is_removable(host) || !mmc_card_keep_power(host)) {
-		err = mmc_sdio_reinit_card(host, mmc_card_keep_power(host));
-	} else if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host)) {
+		err = mmc_sdio_reinit_card(host, 0);
+	} else if (mmc_card_wake_sdio_irq(host)) {
 		/* We may have switched to 1-bit mode during suspend */
 		err = sdio_enable_4bit_bus(host->card);
 	}