Re: [PATCH v6 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 2 potential issue(s) to consider:
- [High] Enabling `keep-power-in-suspend` without `reset-card-at-resume` leaves the host controller in a high-speed state, causing `mmc_init_card()` to fail on resume.
- [High] Back-powering unpowered MMC cards by driving the clock before the VDD regulator is enabled when `reset-card-at-resume` is set but `keep-power-in-suspend` is not.
--

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

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

This commit introduces support for keep-power-in-suspend and
reset-card-at-resume properties for (e)MMC devices. It allows platforms to
keep the card powered during suspend to DRAM for firmware access, and resets
the host controller state upon resume.

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

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

[ ... ]

> @@ -2217,9 +2237,22 @@ static int _mmc_resume(struct mmc_host *host)
>  	if (!mmc_card_suspended(host->card))
>  		goto out;
>  
> +	/*
> +	 * Firmware or other hardware may have accessed the card while it
> +	 * stayed powered through suspend, leaving it in a state the kernel
> +	 * can no longer assume it knows. Reset the host to its initial bus
> +	 * state like _mmc_hw_reset() does for a non-power-cycle reset,
> +	 * before mmc_init_card() re-identifies the card.
> +	 */
> +	if (host->caps2 & MMC_CAP2_RESET_AT_RESUME) {
> +		mmc_set_clock(host, host->f_init);
> +		mmc_set_initial_state(host);
> +	}

[Severity: High]
If a device tree specifies reset-card-at-resume but not
keep-power-in-suspend, _mmc_suspend() will call mmc_power_off(), disabling
the VDD regulator and setting power_mode = MMC_POWER_OFF.

When _mmc_resume() executes this MMC_CAP2_RESET_AT_RESUME block, it calls
mmc_set_clock() and mmc_set_initial_state() before mmc_power_up() is called.

Could this lead to back-powering the unpowered MMC card by actively driving
the clock pin before the VDD regulator is enabled?

> +
>  	mmc_power_up(host, host->card->ocr);
>  	err = mmc_init_card(host, host->card->ocr, host->card);

[Severity: High]
If a device tree specifies keep-power-in-suspend but lacks
reset-card-at-resume, _mmc_suspend() skips mmc_power_off(), leaving
power_mode == MMC_POWER_ON.

When _mmc_resume() is called, it skips the MMC_CAP2_RESET_AT_RESUME block
above. Then mmc_power_up() is called, but it returns immediately because
power_mode is already MMC_POWER_ON:

drivers/mmc/core/core.c:mmc_power_up() {
	if (host->ios.power_mode == MMC_POWER_ON)
		return;
	...
}

Does this cause mmc_init_card() to be called while the host is still
configured for high-speed operation (e.g. 200MHz, HS400), causing the 400kHz
initialization commands to fail?

>  	mmc_card_clr_suspended(host->card);
> +	host->pm_flags &= ~MMC_PM_KEEP_POWER;
>  
>  out:
>  	mmc_release_host(host);

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