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

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

[Ulf Hansson]

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 | 66 +++++++++++++++++++++++++++++-------------
 drivers/mmc/core/sd.c  | 25 +++++++++-------
 2 files changed, 61 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH 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.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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

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

[Avri Altman]

> It's really a true/false value that matters, let's make it clear by returning a bool
> instead.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>

> ---
>  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
> 

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

[Wolfram Sang]

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

On Thu, Mar 20, 2025 at 03:00:32PM +0100, Ulf Hansson wrote:
> It's really a true/false value that matters, let's make it clear by
> returning a bool instead.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

[PATCH 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 preferred option is to use the poweroff-notification command.

Due to an earlier suspend request the eMMC may already have been properly
powered-off, hence we are sometimes leaving the eMMC in its current state.
However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND
set we may unnecessarily restore the power to the eMMC, let's avoid this.

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

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3424bc9e20c5..400dd0449fec 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_may_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_may_poweroff_notify(host, is_suspend))
 		err = mmc_poweroff_notify(host->card, notify_type);
 	else if (mmc_can_sleep(host->card))
 		err = mmc_sleep(host);
@@ -2191,7 +2202,7 @@ static int mmc_shutdown(struct mmc_host *host)
 	 * before we can shutdown it properly.
 	 */
 	if (mmc_can_poweroff_notify(host->card) &&
-		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
+	    !mmc_may_poweroff_notify(host, true))
 		err = _mmc_resume(host);
 
 	if (!err)
-- 
2.43.0

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

[Avri Altman]

> To manage a graceful power-off of the eMMC card during platform shutdown,
> the preferred option is to use the poweroff-notification command.
> 
> Due to an earlier suspend request the eMMC may already have been properly
> powered-off, hence we are sometimes leaving the eMMC in its current state.
> However, in one case when the host has
> MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND
> set we may unnecessarily restore the power to the eMMC, let's avoid this.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/mmc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> 3424bc9e20c5..400dd0449fec 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_may_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_may_poweroff_notify(host, is_suspend))
>  		err = mmc_poweroff_notify(host->card, notify_type);
>  	else if (mmc_can_sleep(host->card))
>  		err = mmc_sleep(host);
> @@ -2191,7 +2202,7 @@ static int mmc_shutdown(struct mmc_host *host)
>  	 * before we can shutdown it properly.
>  	 */
>  	if (mmc_can_poweroff_notify(host->card) &&
> -		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> +	    !mmc_may_poweroff_notify(host, true))
I guess this deserve some extra documentation because:
If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set,
!mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true.

Thanks,
Avri

>  		err = _mmc_resume(host);
> 
>  	if (!err)
> --
> 2.43.0
> 

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

[Wolfram Sang]

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


> > +static bool mmc_may_poweroff_notify(const struct mmc_host *host,
> > +				    bool is_suspend)

Maybe add some comments about the difference between
mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it
super-obvious, so I will easily remember next year again :)

> >  	if (mmc_can_poweroff_notify(host->card) &&
> > -		!(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> > +	    !mmc_may_poweroff_notify(host, true))
> I guess this deserve some extra documentation because:
> If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set,
> !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true.

I agree, I neither get this. Another way to express my confusion is: Why
do we set the 'is_suspend' flag to true in the shutdown function?


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

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

[Ulf Hansson]

On Mon, 31 Mar 2025 at 10:21, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host,
> > > +                               bool is_suspend)
>
> Maybe add some comments about the difference between
> mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it
> super-obvious, so I will easily remember next year again :)

mmc_can_* functions are mostly about checking what the card is capable
of. So mmc_can_poweroff_notify() should be consistent with the other
similar functions.

For eMMC power-off notifications in particular, it has become more
complicated as we need to check the power-off scenario along with the
host's capabilities, to understand what we should do.

I am certainly open to another name than mmc_may_power_off_notify(),
if that is what you are suggesting. Do you have a proposal? :-)

>
> > >     if (mmc_can_poweroff_notify(host->card) &&
> > > -           !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> > > +       !mmc_may_poweroff_notify(host, true))
> > I guess this deserve some extra documentation because:
> > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set,
> > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true.

Right. See more below.

>
> I agree, I neither get this. Another way to express my confusion is: Why
> do we set the 'is_suspend' flag to true in the shutdown function?
>

I understand your concern and I agree that this is rather messy.
Anyway, for shutdown, we set the is_suspend flag to false. The
reasoning behind this is that during shutdown we know that the card
will be fully powered-down (both vcc and vccq will be cut).

In suspend/runtime_suspend, we don't really know as it depends on what
the platform/host is capable of. If we can't do a full power-off
(maybe just vcc can be cut), then we prefer the sleep command instead.

I was hoping that patch3 should make this more clear (using an enum
type), but I can try to add some comment(s) in the code to further
clarify the policy.

Thanks for reviewing and testing!

Kind regards
Uffe

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

[Wolfram Sang]

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

Hi Ulf,

> > > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host,
> > > > +                               bool is_suspend)
> >
> > Maybe add some comments about the difference between
> > mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it
> > super-obvious, so I will easily remember next year again :)
> 
> mmc_can_* functions are mostly about checking what the card is capable
> of. So mmc_can_poweroff_notify() should be consistent with the other
> similar functions.
> 
> For eMMC power-off notifications in particular, it has become more
> complicated as we need to check the power-off scenario along with the
> host's capabilities, to understand what we should do.
> 
> I am certainly open to another name than mmc_may_power_off_notify(),
> if that is what you are suggesting. Do you have a proposal? :-)

Initially, I didn't think of new names but some explanation in comments.
But since you are mentioning a rename now, how about:

mmc_card_can_poweroff_notify() and mmc_host_can_poweroff_notify()?

Similar to the commit 32f18e596141 ("mmc: improve API to make clear
hw_reset callback is for cards") where I renamed 'hw_reset' to
'card_hw_reset' for AFAICS similar reasons.

> > > >     if (mmc_can_poweroff_notify(host->card) &&
> > > > -           !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> > > > +       !mmc_may_poweroff_notify(host, true))
> > > I guess this deserve some extra documentation because:
> > > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set,
> > > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true.
> 
> Right. See more below.
> 
> >
> > I agree, I neither get this. Another way to express my confusion is: Why
> > do we set the 'is_suspend' flag to true in the shutdown function?
> >
> 
> I understand your concern and I agree that this is rather messy.
> Anyway, for shutdown, we set the is_suspend flag to false. The
> reasoning behind this is that during shutdown we know that the card
> will be fully powered-down (both vcc and vccq will be cut).
> 
> In suspend/runtime_suspend, we don't really know as it depends on what
> the platform/host is capable of. If we can't do a full power-off
> (maybe just vcc can be cut), then we prefer the sleep command instead.

I do understand that. I don't see why this needs a change in the
existing logic as Alan pointed out above.

> I was hoping that patch3 should make this more clear (using an enum

Sadly, it didn't. Using MMC_POWEROFF_SUSPEND first and then later
MMC_POWEROFF_SHUTDOWN in mmc_shutdown() is still confusing. Do you want
to return false in case none of the two PWR_CYCLE flags is set?

> type), but I can try to add some comment(s) in the code to further
> clarify the policy.

Please do.

All the best,

   Wolfram


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

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

[Ulf Hansson]

On Mon, 31 Mar 2025 at 12:46, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Ulf,
>
> > > > > +static bool mmc_may_poweroff_notify(const struct mmc_host *host,
> > > > > +                               bool is_suspend)
> > >
> > > Maybe add some comments about the difference between
> > > mmc_can_poweroff_notify() and mmc_may_poweroff_notify()? Like make it
> > > super-obvious, so I will easily remember next year again :)
> >
> > mmc_can_* functions are mostly about checking what the card is capable
> > of. So mmc_can_poweroff_notify() should be consistent with the other
> > similar functions.
> >
> > For eMMC power-off notifications in particular, it has become more
> > complicated as we need to check the power-off scenario along with the
> > host's capabilities, to understand what we should do.
> >
> > I am certainly open to another name than mmc_may_power_off_notify(),
> > if that is what you are suggesting. Do you have a proposal? :-)
>
> Initially, I didn't think of new names but some explanation in comments.
> But since you are mentioning a rename now, how about:
>
> mmc_card_can_poweroff_notify() and mmc_host_can_poweroff_notify()?

mmc_card_can_poweroff_notify() would not be consistent with all the
other mmc_can_* helpers, so I rather stay with
mmc_can_poweroff_notify(), for now. If you think a rename makes sense,
I suggest we do that as a follow up and rename all the helpers.

mmc_host_can_poweroff_notify() seems fine to me!

>
> Similar to the commit 32f18e596141 ("mmc: improve API to make clear
> hw_reset callback is for cards") where I renamed 'hw_reset' to
> 'card_hw_reset' for AFAICS similar reasons.
>
> > > > >     if (mmc_can_poweroff_notify(host->card) &&
> > > > > -           !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE))
> > > > > +       !mmc_may_poweroff_notify(host, true))
> > > > I guess this deserve some extra documentation because:
> > > > If MMC_CAP2_FULL_PWR_CYCLE is not set but MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is set,
> > > > !mmc_may_poweroff_notify(host, true) will evaluate to false while !(host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) will evaluate to true.
> >
> > Right. See more below.
> >
> > >
> > > I agree, I neither get this. Another way to express my confusion is: Why
> > > do we set the 'is_suspend' flag to true in the shutdown function?
> > >
> >
> > I understand your concern and I agree that this is rather messy.
> > Anyway, for shutdown, we set the is_suspend flag to false. The
> > reasoning behind this is that during shutdown we know that the card
> > will be fully powered-down (both vcc and vccq will be cut).
> >
> > In suspend/runtime_suspend, we don't really know as it depends on what
> > the platform/host is capable of. If we can't do a full power-off
> > (maybe just vcc can be cut), then we prefer the sleep command instead.
>
> I do understand that. I don't see why this needs a change in the
> existing logic as Alan pointed out above.

Aha. I get your point now. As stated in the commit message:

Due to an earlier suspend request the eMMC may already have been properly
powered-off, hence we are sometimes leaving the eMMC in its current state.
However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND
set we may unnecessarily restore the power to the eMMC, let's avoid this.

To further clarify, at a system suspend we issue a poweroff-notify for
the case above. At system resume we leave the card in powered-off
state until there is I/O (when we runtime resume it). If a shutdown
occurs before I/O, we would unnecessarily re-initialize the card as
it's already in the correct state.

Let me try to clarify the commit message a bit with this information.

>
> > I was hoping that patch3 should make this more clear (using an enum
>
> Sadly, it didn't. Using MMC_POWEROFF_SUSPEND first and then later
> MMC_POWEROFF_SHUTDOWN in mmc_shutdown() is still confusing. Do you want
> to return false in case none of the two PWR_CYCLE flags is set?
>
> > type), but I can try to add some comment(s) in the code to further
> > clarify the policy.
>
> Please do.
>
> All the best,
>
>    Wolfram
>

Thanks!

Kind regards
Uffe

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

[Wolfram Sang]

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

Hi Ulf,

> mmc_card_can_poweroff_notify() would not be consistent with all the
> other mmc_can_* helpers, so I rather stay with
> mmc_can_poweroff_notify(), for now. If you think a rename makes sense,
> I suggest we do that as a follow up and rename all the helpers.

I vageuly recall that the commit I mentioned below (renaming hw_reset to
card_hw_reset) should have been a start to do exactly this, renaming
more of the helpers. I drifted away. Yet, I still think this would make
MMC core code a lot easier to understand. I'll work on it today, timing
seems good with rc1 on the horizon...

> mmc_host_can_poweroff_notify() seems fine to me!

Great!

> > I do understand that. I don't see why this needs a change in the
> > existing logic as Alan pointed out above.
> 
> Aha. I get your point now. As stated in the commit message:
> 
> Due to an earlier suspend request the eMMC may already have been properly
> powered-off, hence we are sometimes leaving the eMMC in its current state.
> However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND
> set we may unnecessarily restore the power to the eMMC, let's avoid this.

Oookay, now I see what you are aiming at. It seems I got the PWR_CYCLE
flags wrong? I thought MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is only a
subset of MMC_CAP2_FULL_PWR_CYCLE. The former can do the power cycles
only in suspend, while the latter can do them in suspend and shutdown.
So, in my thinking, full power cycle might also have the eMMC
powered-off during shutdown. This is wrong?

> Let me try to clarify the commit message a bit with this information.

Whatever is the final outcome, it needs a comment in the code, I am
quite sure.

Happy hacking,

   Wolfram


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

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

[Ulf Hansson]

On Tue, 1 Apr 2025 at 08:51, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Ulf,
>
> > mmc_card_can_poweroff_notify() would not be consistent with all the
> > other mmc_can_* helpers, so I rather stay with
> > mmc_can_poweroff_notify(), for now. If you think a rename makes sense,
> > I suggest we do that as a follow up and rename all the helpers.
>
> I vageuly recall that the commit I mentioned below (renaming hw_reset to
> card_hw_reset) should have been a start to do exactly this, renaming
> more of the helpers. I drifted away. Yet, I still think this would make
> MMC core code a lot easier to understand. I'll work on it today, timing
> seems good with rc1 on the horizon...

Alright!

>
> > mmc_host_can_poweroff_notify() seems fine to me!
>
> Great!
>
> > > I do understand that. I don't see why this needs a change in the
> > > existing logic as Alan pointed out above.
> >
> > Aha. I get your point now. As stated in the commit message:
> >
> > Due to an earlier suspend request the eMMC may already have been properly
> > powered-off, hence we are sometimes leaving the eMMC in its current state.
> > However, in one case when the host has MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND
> > set we may unnecessarily restore the power to the eMMC, let's avoid this.
>
> Oookay, now I see what you are aiming at. It seems I got the PWR_CYCLE
> flags wrong? I thought MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND is only a
> subset of MMC_CAP2_FULL_PWR_CYCLE. The former can do the power cycles
> only in suspend, while the latter can do them in suspend and shutdown.

Not exactly. In shutdown we don't need specific caps. The card will be
fully powered off no matter what. In other words, it's always better
to do poweroff-notification if the card supports it.

> So, in my thinking, full power cycle might also have the eMMC
> powered-off during shutdown. This is wrong?

See above.

>
> > Let me try to clarify the commit message a bit with this information.
>
> Whatever is the final outcome, it needs a comment in the code, I am
> quite sure.

I will add it!

>
> Happy hacking,
>
>    Wolfram
>

Kind regards
Uffe

[PATCH 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>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 400dd0449fec..60af88ac0213 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_may_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_may_poweroff_notify(host, is_suspend))
+	    mmc_may_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);
@@ -2202,11 +2210,11 @@ static int mmc_shutdown(struct mmc_host *host)
 	 * before we can shutdown it properly.
 	 */
 	if (mmc_can_poweroff_notify(host->card) &&
-	    !mmc_may_poweroff_notify(host, true))
+	    !mmc_may_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;
 }
@@ -2230,7 +2238,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

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

[Wolfram Sang]

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

On Thu, Mar 20, 2025 at 03:00:34PM +0100, Ulf Hansson wrote:
> 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>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

[PATCH 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>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 60af88ac0213..5a62a3d3df32 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

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

[Avri Altman]

> +/*
> + * 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);
Couldn't find how _mmc_suspend handles this new power off flag?

Thanks,
Avri

> +
> +	put_device(&host->card->dev);
> +	host->card = NULL;
> +}
> +
>  /*
>   * Suspend callback
>   */
> --
> 2.43.0
> 

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

[Ulf Hansson]

On Fri, 28 Mar 2025 at 09:13, Avri Altman <Avri.Altman@sandisk.com> wrote:
>
> > +/*
> > + * 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);
> Couldn't find how _mmc_suspend handles this new power off flag?

Right. You need to look closer at mmc_may_poweroff_notify() as it
should return false if MMC_POWEROFF_UNBIND, unless
MMC_CAP2_FULL_PWR_CYCLE.

[...]

Kind regards
Uffe

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

[Wolfram Sang]

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


> > > +     _mmc_suspend(host, MMC_POWEROFF_UNBIND);
> > Couldn't find how _mmc_suspend handles this new power off flag?
> 
> Right. You need to look closer at mmc_may_poweroff_notify() as it
> should return false if MMC_POWEROFF_UNBIND, unless
> MMC_CAP2_FULL_PWR_CYCLE.

And this is what we need for Renesas SDHI which only has
MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND. What this means is:
For suspend, we have some blackbox firmware (PSCI) handling the power
cycle. But for unbind, we cannot trigger the firmware, so we can't use
the notification but need mmc_sleep().

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

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

[Wolfram Sang]

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

On Thu, Mar 20, 2025 at 03:00:35PM +0100, Ulf Hansson wrote:
> 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>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

[PATCH 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.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 5/5] mmc: core: Add support for graceful host removal for SD

[Wolfram Sang]

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

On Thu, Mar 20, 2025 at 03:00:36PM +0100, Ulf Hansson wrote:
> 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.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Testing needs another day, can't do this via remotelab.


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

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

[Wolfram Sang]

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

On Mon, Mar 31, 2025 at 10:30:35AM +0200, Wolfram Sang wrote:
> On Thu, Mar 20, 2025 at 03:00:36PM +0100, Ulf Hansson wrote:
> > 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.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I couldn't find a card yet which has SD_EXT_POWER_OFF_NOTIFY set, but at
least I can confirm that the actual code paths are taken:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

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

[Wolfram Sang]

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


> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Which also means that I tested the whole series on a Renesas Salvator-X
board with a R-Car M3-W SoC (Gen3).


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

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

[Ulf Hansson]

On Tue, 1 Apr 2025 at 10:51, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
>
> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> Which also means that I tested the whole series on a Renesas Salvator-X
> board with a R-Car M3-W SoC (Gen3).
>

Great thanks! I will send a new version addressing your comments on patch2.

Kind regards
Uffe

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

[Wolfram Sang]

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

On Thu, Mar 20, 2025 at 03:00:31PM +0100, Ulf Hansson wrote:
> 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!

Awesome! Thank you. Will surely check it out. Not before next week I am
afraid. But already looking forward to testing it!


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

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

[Wolfram Sang]

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


> Awesome! Thank you. Will surely check it out. Not before next week I am
> afraid. But already looking forward to testing it!

So, I tested this with a Renesas Spider board (R-Car S4). The three
cases (shutdown, suspend, unbind) all lead to proper flagging if notify
can't be used or not. Only the question from patch 2 is left open.

But thank you, this has come a long way :)


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