Re: [PATCH v5 2/2] mmc: core: Honor keep-power-in-suspend for (e)MMC in suspend/resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Leaving the eMMC card in high-speed mode (e.g., HS200/HS400) while abruptly resetting the host controller to legacy timing causes DLL desynchronization and breaks communication upon resume.
- [High] Missing eMMC Power-Off Notification during shutdown, unbind, and undervoltage when MMC_PM_KEEP_POWER is active.
- [Medium] The static MMC_PM_KEEP_POWER capability unintentionally disables aggressive runtime power management (Runtime PM) for the eMMC.
--

--- Patch [2]: [PATCH v5 2/2] mmc: core: Honor keep-power-in-suspend for (e)MMC in suspend/resume ---
commit 1dd1eea6a2934ec02e25c3c7fb1a083b652339a7
Author: Kamal Dasu <kamal.dasu@broadcom.com>

mmc: core: Honor keep-power-in-suspend for (e)MMC in suspend/resume

This commit introduces support for preserving eMMC power during suspend-to-DRAM
on platforms where firmware accesses the card directly during resume. It skips
the sleep and power-off notify sequence if MMC_PM_KEEP_POWER is set.

> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 05444ecf3909f..7cbe655327264 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -2157,6 +2157,35 @@ static int _mmc_suspend(struct mmc_host *host, enum mmc_poweroff_type pm_type)
>  			goto out;
>  	}
>  
> +	/*
> +	 * Keep the card powered across an actual suspend; shutdown, unbind
> +	 * and undervoltage still need the normal power-off path below,
> +	 * since they aren't guaranteed a subsequent _mmc_resume().
> +	 *
> +	 * Check pm_caps, not pm_flags: unlike SDIO, (e)MMC has no
> +	 * per-function driver to request this via
> +	 * sdio_set_host_pm_flags(), so it's a fixed platform trait here.
> +	 *
> +	 * Set MMC_PM_KEEP_POWER in pm_flags so host controller drivers
> +	 * know card power was preserved during suspend and perform a
> +	 * soft resume sequence.
> +	 *
> +	 * Reset the host to its initial bus state like _mmc_hw_reset()
> +	 * does.
> +	 */
> +	if (pm_type == MMC_POWEROFF_SUSPEND &&
> +	    (host->pm_caps & MMC_PM_KEEP_POWER)) {

[Severity: Medium]
Since mmc_runtime_suspend() also calls _mmc_suspend() with
MMC_POWEROFF_SUSPEND, does matching the static host->pm_caps here
inadvertently skip the power down sequence during routine runtime idle
suspend, effectively disabling aggressive runtime power management?

> +		if (!mmc_host_is_spi(host))
> +			err = mmc_deselect_cards(host);
> +		if (!err) {
> +			host->pm_flags |= MMC_PM_KEEP_POWER;
> +			mmc_set_clock(host, host->f_init);
> +			mmc_set_initial_state(host);

[Severity: High]
Does this abruptly drop the host controller's clock to f_init and switch
to legacy 1-bit timing while the eMMC card remains fully powered and
stranded in its high-speed state?

Could this cause the card's internal DLL to lose lock, resulting in
communication failure upon resume when mmc_init_card() sends CMD0?

> +			mmc_card_set_suspended(host->card);

[Severity: High]
Since the new MMC_PM_KEEP_POWER fast path marks the card as suspended
without powering it down, will the early exit in _mmc_suspend() mistakenly
skip subsequent power-off notifications?

If a shutdown, unbind, or undervoltage occurs later, it hits this check
in _mmc_suspend():

	if (mmc_card_suspended(host->card))
		goto out;

and skips sending EXT_CSD_POWER_OFF_LONG and powering off the card?

> +		}
> +		goto out;
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729201420.4047677-1-kamal.dasu@broadcom.com?part=2