[RFC,v2] mmc: suspend MMC also when unbinding

When unbinding a MMC host, the card should be suspended. Otherwise,
problems may arise. E.g. the card still expects power-off notifications
but there is no host to send them anymore. Shimoda-san tried disabling
notifications only, but there were issues with his approaches [1] [2].

Here is my take on it, based on the review comments:

a) 'In principle we would like to run the similar operations at "remove"
    as during "system suspend"' [1]
b) 'We want to support a graceful power off sequence or the card...' [2]

So, first, mmc_remove_card() gets improved to mark the card as "not
present" and to call the bus specific suspend() handler.

Then, _mmc_suspend gets extended to recognize another reason of being
called, namely when a card removal happens. Building upon the now
updated mmc_remove_card(), this is the case when the card is flagged as
"not present".

The logic of either sending a notification or sending the card to sleep
gets updated to handle this new reason. Controllers able to do full
power cycles will still do a full power cycle. Controllers which can
only do power cycles in suspend, will send the card to sleep.

All this is for MMC. SD/SDIO are unaffected because they are not using
the 'card present' flag.

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

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/1602581312-23607-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
[2] https://patchwork.kernel.org/project/linux-mmc/patch/1605005330-7178-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
---

Lightly tested with a Renesas R-Car S4 Spider board. It bascially works
as expected. Serious testing postponed until the generic direction of
this is approved :)

 drivers/mmc/core/bus.c |  3 +++
 drivers/mmc/core/mmc.c | 29 +++++++++++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

On Mon, Nov 04, 2024 at 11:18:42AM +0200, Wolfram Sang wrote:
> When unbinding a MMC host, the card should be suspended. Otherwise,
> problems may arise. E.g. the card still expects power-off notifications
> but there is no host to send them anymore. Shimoda-san tried disabling
> notifications only, but there were issues with his approaches [1] [2].
> 
> Here is my take on it, based on the review comments:
> 
> a) 'In principle we would like to run the similar operations at "remove"
>     as during "system suspend"' [1]
> b) 'We want to support a graceful power off sequence or the card...' [2]
> 
> So, first, mmc_remove_card() gets improved to mark the card as "not
> present" and to call the bus specific suspend() handler.
> 
> Then, _mmc_suspend gets extended to recognize another reason of being
> called, namely when a card removal happens. Building upon the now
> updated mmc_remove_card(), this is the case when the card is flagged as
> "not present".
> 
> The logic of either sending a notification or sending the card to sleep
> gets updated to handle this new reason. Controllers able to do full
> power cycles will still do a full power cycle. Controllers which can
> only do power cycles in suspend, will send the card to sleep.
> 
> All this is for MMC. SD/SDIO are unaffected because they are not using
> the 'card present' flag.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/1602581312-23607-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
> [2] https://patchwork.kernel.org/project/linux-mmc/patch/1605005330-7178-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
> ---
> 
> Lightly tested with a Renesas R-Car S4 Spider board. It bascially works
> as expected. Serious testing postponed until the generic direction of
> this is approved :)

Anything I can do to make the review easier? Some more testing maybe?

On Mon, 4 Nov 2024 at 10:22, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> When unbinding a MMC host, the card should be suspended. Otherwise,
> problems may arise. E.g. the card still expects power-off notifications
> but there is no host to send them anymore. Shimoda-san tried disabling
> notifications only, but there were issues with his approaches [1] [2].
>
> Here is my take on it, based on the review comments:
>
> a) 'In principle we would like to run the similar operations at "remove"
>     as during "system suspend"' [1]
> b) 'We want to support a graceful power off sequence or the card...' [2]
>
> So, first, mmc_remove_card() gets improved to mark the card as "not
> present" and to call the bus specific suspend() handler.
>
> Then, _mmc_suspend gets extended to recognize another reason of being
> called, namely when a card removal happens. Building upon the now
> updated mmc_remove_card(), this is the case when the card is flagged as
> "not present".
>
> The logic of either sending a notification or sending the card to sleep
> gets updated to handle this new reason. Controllers able to do full
> power cycles will still do a full power cycle. Controllers which can
> only do power cycles in suspend, will send the card to sleep.
>
> All this is for MMC. SD/SDIO are unaffected because they are not using
> the 'card present' flag.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/1602581312-23607-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
> [2] https://patchwork.kernel.org/project/linux-mmc/patch/1605005330-7178-1-git-send-email-yoshihiro.shimoda.uh@renesas.com/
> ---
>
> Lightly tested with a Renesas R-Car S4 Spider board. It bascially works
> as expected. Serious testing postponed until the generic direction of
> this is approved :)
>
>  drivers/mmc/core/bus.c |  3 +++
>  drivers/mmc/core/mmc.c | 29 +++++++++++++++++++++--------
>  2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 0ddaee0eae54..52704d39c6d5 100644
> --- a/drivers/mmc/core/bus.c
> +++ b/drivers/mmc/core/bus.c
> @@ -403,5 +403,8 @@ void mmc_remove_card(struct mmc_card *card)
>                 host->cqe_enabled = false;
>         }
>
> +       card->state &= ~MMC_STATE_PRESENT; // TBD: mmc_card_clear_present()

This is nice from a consistency point of view. Although, I don't want
us to use this bit as an indication to inform the bus_ops->suspend()
callback what to do. It seems prone to problems.

> +       host->bus_ops->suspend(host);

I think this is a step in the right direction. Somehow we need to be
able to call the bus_ops->suspend() before we call put_device() and
before we clear the host->card pointer.

However, we don't want to call bus_ops->suspend() in all cases from
mmc_remove_card(), but *only* when it gets called from
mmc_remove_host()->mmc_stop_host(), via the
"host->bus_ops->remove(host)" callback.

I am wondering whether we should just re-work/split-up the code a bit
to make this work. In principle, when mmc_remove_card() is called from
the path above, we should not let it call put_device(), but leave that
part to the caller (mmc_stop_host()) instead. Or something along those
lines.

Would that work?

> +
>         put_device(&card->dev);
>  }
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 6a23be214543..2bcf9ee0caa8 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -32,6 +32,12 @@
>  #define MIN_CACHE_EN_TIMEOUT_MS 1600
>  #define CACHE_FLUSH_TIMEOUT_MS 30000 /* 30s */
>
> +enum mmc_pm_reason {
> +       MMC_PM_REASON_SHUTDOWN,
> +       MMC_PM_REASON_SUSPEND,
> +       MMC_PM_REASON_REMOVAL,
> +};
> +
>  static const unsigned int tran_exp[] = {
>         10000,          100000,         1000000,        10000000,
>         0,              0,              0,              0
> @@ -2104,11 +2110,16 @@ 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_pm_reason reason)
>  {
>         int err = 0;
> -       unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
> -                                       EXT_CSD_POWER_OFF_LONG;
> +       unsigned int notify_type = reason == MMC_PM_REASON_SUSPEND ?
> +                                  EXT_CSD_POWER_OFF_SHORT : EXT_CSD_POWER_OFF_LONG;
> +       bool can_pwr_cycle_now = (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) ||
> +                                 ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND) &&
> +                                   reason == MMC_PM_REASON_SUSPEND);
> +
> +       pr_info("%s: suspend reason %d, can pwr cycle %d\n", mmc_hostname(host), reason, can_pwr_cycle_now);
>
>         mmc_claim_host(host);
>
> @@ -2119,9 +2130,9 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>         if (err)
>                 goto out;
>
> +       /* Notify if pwr_cycle is possible or power gets cut because of shutdown */
>         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)))
> +           (can_pwr_cycle_now || reason == MMC_PM_REASON_SHUTDOWN))
>                 err = mmc_poweroff_notify(host->card, notify_type);
>         else if (mmc_can_sleep(host->card))
>                 err = mmc_sleep(host);
> @@ -2142,9 +2153,11 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
>   */
>  static int mmc_suspend(struct mmc_host *host)
>  {
> +       enum mmc_pm_reason reason = mmc_card_present(host->card) ?
> +                                   MMC_PM_REASON_SUSPEND : MMC_PM_REASON_REMOVAL;

I don't think it makes sense to differentiate between a regular
"suspend" and a "host-removal". The point is, we don't know what will
happen beyond the host-removal. The platform may shutdown or the host
driver probes again.

Let's just use the same commands as we use for suspend.

>         int err;
>
> -       err = _mmc_suspend(host, true);
> +       err = _mmc_suspend(host, reason);
>         if (!err) {
>                 pm_runtime_disable(&host->card->dev);
>                 pm_runtime_set_suspended(&host->card->dev);
> @@ -2191,7 +2204,7 @@ static int mmc_shutdown(struct mmc_host *host)
>                 err = _mmc_resume(host);
>
>         if (!err)
> -               err = _mmc_suspend(host, false);
> +               err = _mmc_suspend(host, MMC_PM_REASON_SHUTDOWN);
>
>         return err;
>  }
> @@ -2215,7 +2228,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_PM_REASON_SUSPEND);
>         if (err)
>                 pr_err("%s: error %d doing aggressive suspend\n",
>                         mmc_hostname(host), err);
> --
> 2.45.2
>

Kind regards
Uffe

Hi Ulf,

slowly coming back to this one.

> > +       card->state &= ~MMC_STATE_PRESENT; // TBD: mmc_card_clear_present()
> 
> This is nice from a consistency point of view. Although, I don't want
> us to use this bit as an indication to inform the bus_ops->suspend()
> callback what to do. It seems prone to problems.

Makes sense. We can use another bit like MMC_STATE_NEEDS_SUSPEND. That
would also...

> > +       host->bus_ops->suspend(host);
> 
> I think this is a step in the right direction. Somehow we need to be
> able to call the bus_ops->suspend() before we call put_device() and
> before we clear the host->card pointer.
> 
> However, we don't want to call bus_ops->suspend() in all cases from
> mmc_remove_card(), but *only* when it gets called from
> mmc_remove_host()->mmc_stop_host(), via the
> "host->bus_ops->remove(host)" callback.
> 
> I am wondering whether we should just re-work/split-up the code a bit
> to make this work. In principle, when mmc_remove_card() is called from
> the path above, we should not let it call put_device(), but leave that
> part to the caller (mmc_stop_host()) instead. Or something along those
> lines.

... avoid this :) Refactoring every code which calls mmc_remove_card()
to handle the additional put_device() on its own seems intrusive to me.
And error prone, new users might forget to do it. And that for only one
use case where we want to do extra stuff.

Leaving out put_device() within mmc_remove_card() only for that one case
is bad API design, of course.

So, I envision more something along this:

	if (card->state & MMC_STATE_NEEDS_SUSPEND)
		mmc_suspend()

Maybe even more generic?

	if (card->state & MMC_STATE_PREPARE_REMOVE)
		bus_ops->prepare_remove()

Or am I missing something from your suggestion?

> > +       enum mmc_pm_reason reason = mmc_card_present(host->card) ?
> > +                                   MMC_PM_REASON_SUSPEND : MMC_PM_REASON_REMOVAL;
> 
> I don't think it makes sense to differentiate between a regular
> "suspend" and a "host-removal". The point is, we don't know what will
> happen beyond the host-removal. The platform may shutdown or the host
> driver probes again.
> 
> Let's just use the same commands as we use for suspend.

Sadly, I think it makes sense because of our HW. We cannot control the
regulators directly. PSCI handles them. And for shutdown, it will do
"something". For normal suspend, nothing will happen because PSCI will
not be called. This is why Shimoda-san introduced
"full-pwr-cycle-in-suspend" back then.

The differentiation is needed a little above:

> -	    ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) || !is_suspend ||
> -	     (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND)))
> +	    (can_pwr_cycle_now || reason == MMC_PM_REASON_SHUTDOWN))
>               err = mmc_poweroff_notify(host->card, notify_type);

Here. Poweroff notification is only valid in the POWEROFF case for us.

Thanks for your time,

   Wolfram