[PATCH v2 0/5] mmc: core: Add support for graceful host removal for eMMC/SD

[PATCH v2 0/5] mmc: core: Add support for graceful host removal for eMMC/SD

[Ulf Hansson]

Changes in v2:
	- Added reviewed/tested-by tags.
	- Updated patch 2.

As pointed out by Wolfram Sang and already discussed at LKML [1] - an mmc host
driver may allow to unbind from its corresponding host device. If there is and
eMMC/SD card attached to the host, the mmc core will just try to cut the power
for it, without trying to make a graceful power-off, thus potentially we could
damage the card.

This series intends to fix this problem for eMMC/SD cards.

Please help to test and review!

Kind regards
Ulf Hansson

[1]
https://lore.kernel.org/all/20241007093447.33084-2-wsa+renesas@sang-engineering.com/

Ulf Hansson (5):
  mmc: core: Convert mmc_can_poweroff_notify() into a bool
  mmc: core: Further avoid re-storing power to the eMMC before a
    shutdown
  mmc: core: Convert into an enum for the poweroff-type for eMMC
  mmc: core: Add support for graceful host removal for eMMC
  mmc: core: Add support for graceful host removal for SD

 drivers/mmc/core/mmc.c | 71 +++++++++++++++++++++++++++++-------------
 drivers/mmc/core/sd.c  | 25 +++++++++------
 2 files changed, 64 insertions(+), 32 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] mmc: core: Convert mmc_can_poweroff_notify() into a bool

[Ulf Hansson]

It's really a true/false value that matters, let's make it clear by
returning a bool instead.

Reviewed-by: Avri Altman <avri.altman@sandisk.com>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 drivers/mmc/core/mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 1522fd2b517d..3424bc9e20c5 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2007,7 +2007,7 @@ static int mmc_sleep(struct mmc_host *host)
 	return err;
 }
 
-static int mmc_can_poweroff_notify(const struct mmc_card *card)
+static bool mmc_can_poweroff_notify(const struct mmc_card *card)
 {
 	return card &&
 		mmc_card_mmc(card) &&
-- 
2.43.0

[PATCH v2 2/5] mmc: core: Further avoid re-storing power to the eMMC before a shutdown

[Ulf Hansson]

To manage a graceful power-off of the eMMC card during platform shutdown,
the prioritized option is to use the poweroff-notification command, if the
eMMC card supports it.

During a suspend request we may decide to fall back to use the sleep
command, instead of the poweroff-notification, unless the mmc host supports
a complete power-cycle of the eMMC. For this reason, we may need to restore
power and re-initialize the card, if it remains suspended when a shutdown
request is received.

However, the current condition to restore power and re-initialize the card
doesn't take into account MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND properly. This
may lead to doing a re-initialization when it really isn't needed, as the
eMMC may already have been powered-off using the poweroff-notification
command. Let's fix the condition to avoid this.

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Updated commit message, clarified comment in the code and renamed a
	function, according to Wolfram/Avri's comments.

---
 drivers/mmc/core/mmc.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3424bc9e20c5..ee65c5b85f95 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2014,6 +2014,18 @@ static bool mmc_can_poweroff_notify(const struct mmc_card *card)
 		(card->ext_csd.power_off_notification == EXT_CSD_POWER_ON);
 }
 
+static bool mmc_host_can_poweroff_notify(const struct mmc_host *host,
+					 bool is_suspend)
+{
+	if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)
+		return true;
+
+	if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND && is_suspend)
+		return true;
+
+	return !is_suspend;
+}
+
 static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 {
 	unsigned int timeout = card->ext_csd.generic_cmd6_time;
@@ -2124,8 +2136,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 		goto out;
 
 	if (mmc_can_poweroff_notify(host->card) &&
-	    ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
-	     (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND)))
+	    mmc_host_can_poweroff_notify(host, is_suspend))
 		err = mmc_poweroff_notify(host->card, notify_type);
 	else if (mmc_can_sleep(host->card))
 		err = mmc_sleep(host);
@@ -2187,11 +2198,12 @@ static int mmc_shutdown(struct mmc_host *host)
 	int err = 0;
 
 	/*
-	 * In a specific case for poweroff notify, we need to resume the card
-	 * before we can shutdown it properly.
+	 * If the card remains suspended at this point and it was done by using
+	 * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow
+	 * us to send the preferred poweroff-notification cmd at shutdown.
 	 */
 	if (mmc_can_poweroff_notify(host->card) &&
-		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
+	    !mmc_host_can_poweroff_notify(host, true))
 		err = _mmc_resume(host);
 
 	if (!err)
-- 
2.43.0

[PATCH v2 3/5] mmc: core: Convert into an enum for the poweroff-type for eMMC

[Ulf Hansson]

Currently we are only distinguishing between the suspend and the shutdown
scenarios, which make a bool sufficient as in-parameter to the various PM
functions for eMMC. However, to prepare for adding support for another
scenario in a subsequent change, let's convert into using an enum.

Suggested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 drivers/mmc/core/mmc.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index ee65c5b85f95..c41cee7ef267 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -33,6 +33,11 @@
 #define MIN_CACHE_EN_TIMEOUT_MS 1600
 #define CACHE_FLUSH_TIMEOUT_MS 30000 /* 30s */
 
+enum mmc_poweroff_type {
+	MMC_POWEROFF_SUSPEND,
+	MMC_POWEROFF_SHUTDOWN,
+};
+
 static const unsigned int tran_exp[] = {
 	10000,		100000,		1000000,	10000000,
 	0,		0,		0,		0
@@ -2015,15 +2020,16 @@ static bool mmc_can_poweroff_notify(const struct mmc_card *card)
 }
 
 static bool mmc_host_can_poweroff_notify(const struct mmc_host *host,
-					 bool is_suspend)
+					 enum mmc_poweroff_type pm_type)
 {
 	if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE)
 		return true;
 
-	if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND && is_suspend)
+	if (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND &&
+	    pm_type == MMC_POWEROFF_SUSPEND)
 		return true;
 
-	return !is_suspend;
+	return pm_type == MMC_POWEROFF_SHUTDOWN;
 }
 
 static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
@@ -2120,11 +2126,13 @@ static int _mmc_flush_cache(struct mmc_host *host)
 	return err;
 }
 
-static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
+static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
 {
+	unsigned int notify_type = EXT_CSD_POWER_OFF_SHORT;
 	int err = 0;
-	unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
-					EXT_CSD_POWER_OFF_LONG;
+
+	if (pm_type == MMC_POWEROFF_SHUTDOWN)
+		notify_type = EXT_CSD_POWER_OFF_LONG;
 
 	mmc_claim_host(host);
 
@@ -2136,7 +2144,7 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
 		goto out;
 
 	if (mmc_can_poweroff_notify(host->card) &&
-	    mmc_host_can_poweroff_notify(host, is_suspend))
+	    mmc_host_can_poweroff_notify(host, pm_type))
 		err = mmc_poweroff_notify(host->card, notify_type);
 	else if (mmc_can_sleep(host->card))
 		err = mmc_sleep(host);
@@ -2159,7 +2167,7 @@ static int mmc_suspend(struct mmc_host *host)
 {
 	int err;
 
-	err = _mmc_suspend(host, true);
+	err = _mmc_suspend(host, MMC_POWEROFF_SUSPEND);
 	if (!err) {
 		pm_runtime_disable(&host->card->dev);
 		pm_runtime_set_suspended(&host->card->dev);
@@ -2203,11 +2211,11 @@ static int mmc_shutdown(struct mmc_host *host)
 	 * us to send the preferred poweroff-notification cmd at shutdown.
 	 */
 	if (mmc_can_poweroff_notify(host->card) &&
-	    !mmc_host_can_poweroff_notify(host, true))
+	    !mmc_host_can_poweroff_notify(host, MMC_POWEROFF_SUSPEND))
 		err = _mmc_resume(host);
 
 	if (!err)
-		err = _mmc_suspend(host, false);
+		err = _mmc_suspend(host, MMC_POWEROFF_SHUTDOWN);
 
 	return err;
 }
@@ -2231,7 +2239,7 @@ static int mmc_runtime_suspend(struct mmc_host *host)
 	if (!(host->caps & MMC_CAP_AGGRESSIVE_PM))
 		return 0;
 
-	err = _mmc_suspend(host, true);
+	err = _mmc_suspend(host, MMC_POWEROFF_SUSPEND);
 	if (err)
 		pr_err("%s: error %d doing aggressive suspend\n",
 			mmc_hostname(host), err);
-- 
2.43.0

[PATCH v2 4/5] mmc: core: Add support for graceful host removal for eMMC

[Ulf Hansson]

An mmc host driver may allow to unbind from its corresponding host device.
If an eMMC card is attached to the host, the mmc core will just try to cut
the power for it, without obeying to the eMMC spec.

Potentially this may damage the card and it may also prevent us from
successfully doing a re-initialization of it, which would typically happen
if/when we try to re-bind the mmc host driver.

To fix these problems, let's implement a graceful power-down of the card at
host removal.

Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 drivers/mmc/core/mmc.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index c41cee7ef267..48656dadf93b 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -36,6 +36,7 @@
 enum mmc_poweroff_type {
 	MMC_POWEROFF_SUSPEND,
 	MMC_POWEROFF_SHUTDOWN,
+	MMC_POWEROFF_UNBIND,
 };
 
 static const unsigned int tran_exp[] = {
@@ -2054,15 +2055,6 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
 	return err;
 }
 
-/*
- * Host is being removed. Free up the current card.
- */
-static void mmc_remove(struct mmc_host *host)
-{
-	mmc_remove_card(host->card);
-	host->card = NULL;
-}
-
 /*
  * Card detection - card is alive.
  */
@@ -2088,7 +2080,8 @@ static void mmc_detect(struct mmc_host *host)
 	mmc_put_card(host->card, NULL);
 
 	if (err) {
-		mmc_remove(host);
+		mmc_remove_card(host->card);
+		host->card = NULL;
 
 		mmc_claim_host(host);
 		mmc_detach_bus(host);
@@ -2160,6 +2153,20 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
 	return err;
 }
 
+/*
+ * Host is being removed. Free up the current card and do a graceful power-off.
+ */
+static void mmc_remove(struct mmc_host *host)
+{
+	get_device(&host->card->dev);
+	mmc_remove_card(host->card);
+
+	_mmc_suspend(host, MMC_POWEROFF_UNBIND);
+
+	put_device(&host->card->dev);
+	host->card = NULL;
+}
+
 /*
  * Suspend callback
  */
-- 
2.43.0

[PATCH v2 5/5] mmc: core: Add support for graceful host removal for SD

[Ulf Hansson]

An mmc host driver may allow to unbind from its corresponding host device.
If an SD card is attached to the host, the mmc core will just try to cut
the power for it, without obeying to the SD spec that potentially may
damage the card.

Let's fix this problem by implementing a graceful power-down of the card at
host removal.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 drivers/mmc/core/sd.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 8eba697d3d86..cb4254a43f85 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -1596,15 +1596,6 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
 	return err;
 }
 
-/*
- * Host is being removed. Free up the current card.
- */
-static void mmc_sd_remove(struct mmc_host *host)
-{
-	mmc_remove_card(host->card);
-	host->card = NULL;
-}
-
 /*
  * Card detection - card is alive.
  */
@@ -1630,7 +1621,8 @@ static void mmc_sd_detect(struct mmc_host *host)
 	mmc_put_card(host->card, NULL);
 
 	if (err) {
-		mmc_sd_remove(host);
+		mmc_remove_card(host->card);
+		host->card = NULL;
 
 		mmc_claim_host(host);
 		mmc_detach_bus(host);
@@ -1730,6 +1722,19 @@ static int _mmc_sd_suspend(struct mmc_host *host)
 	return err;
 }
 
+/*
+ * Host is being removed. Free up the current card and do a graceful power-off.
+ */
+static void mmc_sd_remove(struct mmc_host *host)
+{
+	get_device(&host->card->dev);
+	mmc_remove_card(host->card);
+
+	_mmc_sd_suspend(host);
+
+	put_device(&host->card->dev);
+	host->card = NULL;
+}
 /*
  * Callback for suspend
  */
-- 
2.43.0

Re: [PATCH v2 2/5] mmc: core: Further avoid re-storing power to the eMMC before a shutdown

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]


> @@ -2187,11 +2198,12 @@ static int mmc_shutdown(struct mmc_host *host)
>  	int err = 0;
>  
>  	/*
> -	 * In a specific case for poweroff notify, we need to resume the card
> -	 * before we can shutdown it properly.
> +	 * If the card remains suspended at this point and it was done by using
> +	 * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow
> +	 * us to send the preferred poweroff-notification cmd at shutdown.
>  	 */
>  	if (mmc_can_poweroff_notify(host->card) &&
> -		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> +	    !mmc_host_can_poweroff_notify(host, true))

Ooookay, I think I got this logic now. I think it makes sense to make it
more explicit in the comment, though:

"This is then the case when the card is able to handle poweroff
notifications in general but the host could not initiate those for
suspend."

Something like this?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/5] mmc: core: Further avoid re-storing power to the eMMC before a shutdown

[Ulf Hansson]

On Tue, 8 Apr 2025 at 10:09, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > @@ -2187,11 +2198,12 @@ static int mmc_shutdown(struct mmc_host *host)
> >       int err = 0;
> >
> >       /*
> > -      * In a specific case for poweroff notify, we need to resume the card
> > -      * before we can shutdown it properly.
> > +      * If the card remains suspended at this point and it was done by using
> > +      * the sleep-cmd (CMD5), we may need to re-initialize it first, to allow
> > +      * us to send the preferred poweroff-notification cmd at shutdown.
> >        */
> >       if (mmc_can_poweroff_notify(host->card) &&
> > -             !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> > +         !mmc_host_can_poweroff_notify(host, true))
>
> Ooookay, I think I got this logic now. I think it makes sense to make it
> more explicit in the comment, though:
>
> "This is then the case when the card is able to handle poweroff
> notifications in general but the host could not initiate those for
> suspend."
>
> Something like this?

Well, in my opinion I think this would become a bit too much comments
in the code.

The rather long function-names "mmc_can_poweroff_notify" (that will
change to mmc_card_can_poweroff_notify with your series) and
"mmc_host_can_poweroff_notify" are rather self-explanatory, don't you
think?

Kind regards
Uffe

Re: [PATCH v2 2/5] mmc: core: Further avoid re-storing power to the eMMC before a shutdown

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]


> The rather long function-names "mmc_can_poweroff_notify" (that will
> change to mmc_card_can_poweroff_notify with your series) and
> "mmc_host_can_poweroff_notify" are rather self-explanatory, don't you
> think?

Well, you are the boss here, but frankly, I don't think it is obvious
enough. I had to look twice and very closely to understand the logic.
Not because of the function name, but for the reason why 'is_suspend' is
true despite being in _shutdown(). Adrian was wondering about it the
first time, too. So, I honestly think the comment is

  for a maintainer -> superfluous
  for a part-time-MMC-core-hacker -> helpful to remember
  for someone new to the code -> essential

Something like this.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/5] mmc: core: Further avoid re-storing power to the eMMC before a shutdown

[Ulf Hansson]

On Tue, 8 Apr 2025 at 17:07, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > The rather long function-names "mmc_can_poweroff_notify" (that will
> > change to mmc_card_can_poweroff_notify with your series) and
> > "mmc_host_can_poweroff_notify" are rather self-explanatory, don't you
> > think?
>
> Well, you are the boss here, but frankly, I don't think it is obvious
> enough. I had to look twice and very closely to understand the logic.
> Not because of the function name, but for the reason why 'is_suspend' is
> true despite being in _shutdown(). Adrian was wondering about it the
> first time, too. So, I honestly think the comment is
>
>   for a maintainer -> superfluous
>   for a part-time-MMC-core-hacker -> helpful to remember
>   for someone new to the code -> essential
>
> Something like this.
>

I understand what you are saying and I agree. However, the problem is
that your concern applies to a lot more code in the mmc core, but this
condition.

Don't get me wrong, I don't mind useful comments and good
documentation, but perhaps what we are really missing is a general mmc
documentation that describes how the core is working and in particular
the power-management  part of it. Unfortunately, I don't think I will
have the bandwidth currently to work on this.

That said, I am going to apply the $subject patch as is - but feel
free to send a patch on top if you want to add and improve any further
comments in the code. I would be happy to apply it!

Kind regards
Uffe

Re: [PATCH v2 2/5] mmc: core: Further avoid re-storing power to the eMMC before a shutdown

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]


> I understand what you are saying and I agree. However, the problem is
> that your concern applies to a lot more code in the mmc core, but this
> condition.

We can easily agree on that :)

> Don't get me wrong, I don't mind useful comments and good
> documentation, but perhaps what we are really missing is a general mmc
> documentation that describes how the core is working and in particular
> the power-management  part of it.

That would be the ideal solution, no doubt.

> Unfortunately, I don't think I will have the bandwidth currently to
> work on this.

Same here. Plus, I don't have a complete understanding of it. Obtaining
this understanding and then write some docs about my findings would be
awesome, of course. But -EBUSY, too...

> That said, I am going to apply the $subject patch as is - but feel

I still think that having the comment is better than not having it, but
I accept your decision and will still be happy that we finally solved
the power-off-notification issue \o/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]