[RFC PATCH] mmc: suspend MMC also when unbinding

[RFC PATCH] mmc: suspend MMC also when unbinding

[Wolfram Sang]

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

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);
-- 
2.45.2

Re: [RFC PATCH] mmc: suspend MMC also when unbinding

[kernel test robot]

Hi Wolfram,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.12-rc2 next-20241010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wolfram-Sang/mmc-suspend-MMC-also-when-unbinding/20241007-173705
base:   linus/master
patch link:    https://lore.kernel.org/r/20241007093447.33084-2-wsa%2Brenesas%40sang-engineering.com
patch subject: [RFC PATCH] mmc: suspend MMC also when unbinding
config: i386-randconfig-141-20241010 (https://download.01.org/0day-ci/archive/20241011/202410110517.E2SBXURn-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410110517.E2SBXURn-lkp@intel.com/

smatch warnings:
drivers/mmc/core/mmc.c:2124 _mmc_suspend() warn: inconsistent indenting

vim +2124 drivers/mmc/core/mmc.c

  2114	
  2115	static int _mmc_suspend(struct mmc_host *host, enum mmc_pm_reason reason)
  2116	{
  2117		int err = 0;
  2118		unsigned int notify_type = reason == MMC_PM_REASON_SUSPEND ?
  2119					   EXT_CSD_POWER_OFF_SHORT : EXT_CSD_POWER_OFF_LONG;
  2120		bool can_pwr_cycle_now = (host->caps2 & MMC_CAP2_FULL_PWR_CYCLE) ||
  2121					  ((host->caps2 & MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND) &&
  2122					    reason == MMC_PM_REASON_SUSPEND);
  2123	
> 2124	pr_info("%s: suspend reason %d, can pwr cycle %d\n", mmc_hostname(host), reason, can_pwr_cycle_now);
  2125	
  2126		mmc_claim_host(host);
  2127	
  2128		if (mmc_card_suspended(host->card))
  2129			goto out;
  2130	
  2131		err = _mmc_flush_cache(host);
  2132		if (err)
  2133			goto out;
  2134	
  2135		/* Notify if pwr_cycle is possible or power gets cut because of shutdown */
  2136		if (mmc_can_poweroff_notify(host->card) &&
  2137		    (reason == MMC_PM_REASON_SHUTDOWN || can_pwr_cycle_now))
  2138			err = mmc_poweroff_notify(host->card, notify_type);
  2139		else if (mmc_can_sleep(host->card))
  2140			err = mmc_sleep(host);
  2141		else if (!mmc_host_is_spi(host))
  2142			err = mmc_deselect_cards(host);
  2143	
  2144		if (!err) {
  2145			mmc_power_off(host);
  2146			mmc_card_set_suspended(host->card);
  2147		}
  2148	out:
  2149		mmc_release_host(host);
  2150		return err;
  2151	}
  2152	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH] mmc: suspend MMC also when unbinding

[Ulf Hansson]

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

Re: [RFC PATCH] mmc: suspend MMC also when unbinding

[Wolfram Sang]

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


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


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