Re: [PATCH v6 2/8] mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbalanced IRQ disable/enable during system suspend/resume when SDIO wakeup fails to enable or is not applicable.
- [Medium] Missing `pm_runtime_put` on error paths in `sdhci_esdhc_suspend()` leads to permanent runtime PM usage count leaks.
--

--- Patch [2]: [PATCH v6 2/8] mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume ---
commit 4b48b54434ffc2d8ee1f13e793a63d33908babc8
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume
    
Extract the DLL override setup from esdhc_set_uhs_signaling() into
a helper esdhc_set_dll_override(), and call it on the resume path
when the card kept power and is using a DDR timing mode.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6e06b3eeacce9..685e508c61279 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
[ ... ]
> @@ -2123,9 +2131,18 @@ static int sdhci_esdhc_resume(struct device *dev)
>  	 * restore the saved tuning delay value for the device which keep
>  	 * power during system PM.
>  	 */
> -	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data))
> +	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data)) {
>  		sdhc_esdhc_tuning_restore(host);
>  
> +		/*
> +		 * Restore DLL override for DDR modes. hwinit unconditionally
> +		 * clears ESDHC_DLL_CTRL, but the card is still in DDR mode.
> +		 */
> +		if (host->timing == MMC_TIMING_UHS_DDR50 ||
> +		    host->timing == MMC_TIMING_MMC_DDR52)
> +			esdhc_set_dll_override(host);
> +	}
> +
>  	pm_runtime_put_autosuspend(dev);
>  
>  	return ret;

[Severity: High]
This is a pre-existing issue, but I noticed a potential IRQ unbalance during
suspend/resume.

If device_may_wakeup(dev) is true but the controller doesn't support or
successfully enable in-band IRQ wakeups (for example, standard SD cards
using GPIO CD), sdhci_enable_irq_wakeups() returns false and
host->irq_wake_enabled is set to false.

In sdhci_esdhc_suspend(), the device IRQ is unconditionally disabled:

sdhci_esdhc_suspend() {
    ...
    disable_irq(host->irq);
    ret = sdhci_enable_irq_wakeups(host);
    ...
}

If host->irq_wake_enabled is false, will the IRQ remain disabled on resume?
Looking at sdhci_esdhc_resume(), it appears enable_irq() is bypassed if the
wakeup enablement failed:

sdhci_esdhc_resume() {
    ...
    if (host->irq_wake_enabled) {
        sdhci_disable_irq_wakeups(host);
        enable_irq(host->irq);
    }
    ...
}

Could this leave the SDHCI controller's main interrupt permanently disabled
after a suspend/resume cycle, stalling all future MMC/SD card transactions?

[Severity: Medium]
This is another pre-existing issue, but does sdhci_esdhc_suspend() leak the
runtime PM usage counter on error paths?

sdhci_esdhc_suspend() starts by incrementing the PM usage counter:

sdhci_esdhc_suspend() {
    ...
    pm_runtime_get_sync(dev);
    ...

Later, it can return early without dropping the acquired PM reference:

    ...
    ret = pinctrl_pm_select_sleep_state(dev);
    if (ret)
        return ret;
    ...
}

If operations like pinctrl_pm_select_sleep_state() or
mmc_gpio_set_cd_wake() fail, the function returns an error code and system
suspend is aborted. Since the PM core skips the resume callback for devices
whose suspend failed, wouldn't the matching pm_runtime_put_autosuspend() be
skipped, leaving the PM usage counter permanently leaked and preventing the
controller from entering runtime suspend again?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713043904.4144172-1-ziniu.wang_1@oss.nxp.com?part=2