Re: [PATCH v7 4/8] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored in `sdhci_esdhc_suspend()`, which can lead to a synchronous external abort (kernel panic) if it fails.
- [High] The return value of `pm_runtime_force_resume(dev)` is ignored. If it fails, subsequent register accesses will cause a system crash.
- [High] Runtime PM usage count is leaked on the error paths of `sdhci_esdhc_suspend()` and `sdhci_esdhc_resume()`.
- [High] The IRQ remains permanently disabled if `sdhci_enable_irq_wakeups()` fails during system suspend.
--

--- Patch [4]: [PATCH v7 4/8] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume ---
commit fff04de9eca73d5e5d595f7bfc0d534292f22cd5
Author: Luke Wang <ziniu.wang_1@nxp.com>
mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

This commit fixes persistent read CRC errors on SDIO devices resuming in DDR
mode. It ensures the pinctrl state is correctly restored for the current timing
before enabling DDR mode during resume. Wakeup devices are skipped as they
keep their active pin state during suspend.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 2da5c8ab3e622..bc0864cb22b49 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2115,6 +2115,12 @@ static int sdhci_esdhc_resume(struct device *dev)
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>  	int ret;
>  
> +	if (!device_may_wakeup(dev)) {
> +		ret = esdhc_change_pinstate(host, host->timing);
> +		if (ret)
> +			dev_warn(dev, "Failed to restore pinctrl state\n");
> +	}
> +
>  	pm_runtime_force_resume(dev);

[Severity: High]
This is a pre-existing issue, but the return value of pm_runtime_force_resume()
is ignored here. If it fails, could the subsequent unclocked hardware register
accesses in sdhci_esdhc_imx_hwinit() cause a synchronous external abort?

>  
>  	ret = mmc_gpio_set_cd_wake(host->mmc, false);

[Severity: High]
This is a pre-existing issue, but if mmc_gpio_set_cd_wake() fails, the early
return skips the required pm_runtime_put_autosuspend() at the end of the
function:

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_resume() {
    ...
	ret = mmc_gpio_set_cd_wake(host->mmc, false);
	if (ret)
		return ret;
    ...
	pm_runtime_put_autosuspend(dev);
	return 0;
}

Could this leak the runtime PM usage count and prevent the device from ever
entering runtime suspend again?


[Severity: High]
This is a pre-existing issue, but could a similar leak happen in the suspend
path if pinctrl_pm_select_sleep_state() fails?

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_suspend() {
	...
	pm_runtime_get_sync(dev);
	...
	} else {
		ret = pinctrl_pm_select_sleep_state(dev);
		if (ret)
			return ret;
	}
	...
}

Does the early return here leak the PM reference acquired by
pm_runtime_get_sync()?


[Severity: High]
This is a pre-existing issue, but the return value of pm_runtime_get_sync()
in sdhci_esdhc_suspend() is completely ignored:

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_suspend() {
    ...
	pm_runtime_get_sync(dev);

	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
    ...
}

If pm_runtime_get_sync() fails to enable the clocks, could the subsequent
hardware register accesses via sdhc_esdhc_tuning_save() cause a kernel panic?


[Severity: High]
This is a pre-existing issue, but could the IRQ remain permanently disabled if
sdhci_enable_irq_wakeups() fails during suspend?

In sdhci_esdhc_suspend(), the IRQ is disabled unconditionally when the device
may wake up:

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_suspend() {
    ...
	if (device_may_wakeup(dev)) {
		disable_irq(host->irq);
		ret = sdhci_enable_irq_wakeups(host);
    ...
}

But in sdhci_esdhc_resume(), it is only re-enabled if host->irq_wake_enabled is
true, which requires sdhci_enable_irq_wakeups() to have succeeded:

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_resume() {
    ...
	if (host->irq_wake_enabled) {
		sdhci_disable_irq_wakeups(host);
		enable_irq(host->irq);
	}
    ...
}

If sdhci_enable_irq_wakeups() fails in the suspend path, will the device become
completely unresponsive to interrupts after system resume?

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