| Message ID | 20241007093447.33084-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] mmc: suspend MMC also when unbinding | expand |
On Mon, 7 Oct 2024 at 11:34, 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, _mmc_suspend gets extended to recognize another reason of being > called, namely when unbinding happens. The logic of 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 that. > Controllers which can only do power cycles in suspend, will send the > card to sleep. Finally, mmc_remove() calls _mmc_suspend now with the new > reason 'unbind'. From a principle point of view this makes perfect sense, but unfortunately it's not that easy. See below. > > 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/ > --- > > RFC to see if the direction is proper. Obvious improvements are removing > the debug printout and check if the forward declaration can be avoided. > This was lightly tested on a Renesas Salvator board. Accessing the eMMC > after unbind/bind and suspend/resume showed no regressions. > > drivers/mmc/core/mmc.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index 6a23be214543..bd4381fa182f 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_UNBIND, > +}; > + > static const unsigned int tran_exp[] = { > 10000, 100000, 1000000, 10000000, > 0, 0, 0, 0 > @@ -2032,11 +2038,13 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type) > return err; > } > > +static int _mmc_suspend(struct mmc_host *host, enum mmc_pm_reason reason); > /* > * Host is being removed. Free up the current card. > */ > static void mmc_remove(struct mmc_host *host) > { > + _mmc_suspend(host, MMC_PM_REASON_UNBIND); Calling _mmc_suspend() here, will put the mmc card into sleep/power-off state and the card will also be powered-off. During this period, we may receive I/O requests in the mmc-blk-queue, which then the mmc block device driver tries to serve. This may lead to that we call the host driver's ops, with the state MMC_POWER_OFF and asking it to serve requests. This doesn't work and will hang some of the host HW/drivers. To be able to put the mmc card into sleep/power-off state, we first need to prevent the mmc-blk-queue from serving any additional I/O requests, which is what mmc_remove_card() does. :-) Although, we can't call _mmc_suspend() after mmc_remove_card() as the mmc_card may have been freed by then. Hmm... > mmc_remove_card(host->card); > host->card = NULL; > } [...] Kind regards Uffe
> To be able to put the mmc card into sleep/power-off state, we first > need to prevent the mmc-blk-queue from serving any additional I/O > requests, which is what mmc_remove_card() does. :-) > > Although, we can't call _mmc_suspend() after mmc_remove_card() as the > mmc_card may have been freed by then. Hmm... Okay, this means there is basically only one place where we can suspend the card. I made another RFC. Will test it now and send it out later if all goes well.
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index 6a23be214543..bd4381fa182f 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_UNBIND, +}; + static const unsigned int tran_exp[] = { 10000, 100000, 1000000, 10000000, 0, 0, 0, 0 @@ -2032,11 +2038,13 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type) return err; } +static int _mmc_suspend(struct mmc_host *host, enum mmc_pm_reason reason); /* * Host is being removed. Free up the current card. */ static void mmc_remove(struct mmc_host *host) { + _mmc_suspend(host, MMC_PM_REASON_UNBIND); mmc_remove_card(host->card); host->card = NULL; } @@ -2104,11 +2112,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 +2132,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))) + (reason == MMC_PM_REASON_SHUTDOWN || can_pwr_cycle_now)) err = mmc_poweroff_notify(host->card, notify_type); else if (mmc_can_sleep(host->card)) err = mmc_sleep(host); @@ -2144,7 +2157,7 @@ static int mmc_suspend(struct mmc_host *host) { int err; - err = _mmc_suspend(host, true); + err = _mmc_suspend(host, MMC_PM_REASON_SUSPEND); 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);
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, _mmc_suspend gets extended to recognize another reason of being called, namely when unbinding happens. The logic of 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 that. Controllers which can only do power cycles in suspend, will send the card to sleep. Finally, mmc_remove() calls _mmc_suspend now with the new reason 'unbind'. 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/ --- RFC to see if the direction is proper. Obvious improvements are removing the debug printout and check if the forward declaration can be avoided. This was lightly tested on a Renesas Salvator board. Accessing the eMMC after unbind/bind and suspend/resume showed no regressions. drivers/mmc/core/mmc.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)