Re: [PATCH v8 3/3] mmc: core: Honor keep-power-in-suspend and reset-card-at-resume for (e)MMC

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The shutdown/undervoltage fallback path in _mmc_suspend attempts to reselect a firmware-clobbered card without resetting the host controller, guaranteeing a selection failure that silently skips the required power-off.
--

--- Patch [3]: [PATCH v8 3/3] mmc: core: Honor keep-power-in-suspend and reset-card-at-resume for (e)MMC ---
commit 035aca0a123c885d88b4aa6c2a934f57e12bf03c
Author: Kamal Dasu <[email protected]>

mmc: core: Honor keep-power-in-suspend and reset-card-at-resume for (e)MMC

This commit adds support for keep-power-in-suspend and reset-card-at-resume
for (e)MMC devices. It keeps the card powered during suspend so firmware can
access it, and resets the card when the kernel resumes. It also adjusts
_mmc_suspend() to handle shutdown and undervoltage cases when the card is
already in the keep-power suspended state.

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 05444ecf3909f..a081907bbcf53 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c

[ ... ]

> @@ -2144,8 +2144,28 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
>  
>  	mmc_claim_host(host);
>  
> -	if (mmc_card_suspended(host->card))
> -		goto out;
> +	if (mmc_card_suspended(host->card)) {
> +		/*
> +		 * Nothing to do for a redundant suspend call. Otherwise, the
> +		 * card can only still be marked suspended here because the
> +		 * keep-power fast path below left it powered and merely
> +		 * deselected -- reselect it and continue into the normal
> +		 * power-off sequence below, since shutdown, unbind and
> +		 * undervoltage need mmc_power_off() regardless of how the
> +		 * card got here.
> +		 */
> +		if (pm_type == MMC_POWEROFF_SUSPEND ||
> +		    !(host->pm_flags & MMC_PM_KEEP_POWER))
> +			goto out;
> +
> +		if (!mmc_host_is_spi(host)) {
> +			err = mmc_select_card(host->card);
> +			if (err)
> +				goto out;

[Severity: High]
Since the firmware might have modified the card state during suspend
(which is why reset-card-at-resume is used), will mmc_select_card() fail
here if we haven't reset the host and re-initialized the card first?

If mmc_select_card() fails (e.g. CMD7 timeout), taking the goto out
path will completely skip the mmc_power_off() sequence. Does this leave
the card powered on during shutdown or undervoltage, defeating the
purpose of this fallback path?

> +		}
> +		mmc_card_clr_suspended(host->card);
> +		host->pm_flags &= ~MMC_PM_KEEP_POWER;
> +	}
>  
>  	/*
>  	 * For the undervoltage case, we care more about device integrity.
> @@ -2157,6 +2177,33 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
>  			goto out;

[Severity: High]
Just below this hunk in _mmc_suspend(), the cache is flushed:

	if (pm_type != MMC_POWEROFF_UNDERVOLTAGE) {
		err = _mmc_flush_cache(host);
		if (err)
			goto out;
	}

Even if mmc_select_card() were to succeed or its error were ignored,
would _mmc_flush_cache() also fail due to the clobbered card state and
abort the power-off sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3