[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