Re: [PATCH v5 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 3 potential issue(s) to consider:

New issues:
- [Medium] The patch unconditionally logs a warning if `esdhc_change_pinstate` returns an error, which will spam dmesg on every system resume for platforms that do not define pinctrl or specific pinctrl states.

Pre-existing issues:
- [High] Return value of `pm_runtime_force_resume()` is ignored, which can lead to an external abort (kernel panic) if it fails.
- [High] Main SDHCI interrupt remains permanently disabled after system resume if wakeup setup fails or is unconfigured.
--

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

mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

SDIO devices such as WiFi may keep power during suspend, so the MMC
core skips full card re-initialization on resume and directly restores
the host controller's ios timing to match the card. This patch restores
the pinctrl state based on current timing mode before
pm_runtime_force_resume() to prevent persistent read CRC errors.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> --- 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");

[Severity: Medium]
Will this unconditionally log a warning and spam dmesg on every system resume
for platforms that do not define pinctrl or specific pinctrl states? If
esdhc_change_pinstate() returns an error (like -EINVAL when imx_data->pinctrl
or pins_100mhz is ERR_PTR), it appears this warning will always be printed.

> +	}
> +
>  	pm_runtime_force_resume(dev);

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
pm_runtime_force_resume() allow execution to proceed with disabled clocks?

If pm_runtime_force_resume() fails to enable the clocks (e.g., in
sdhci_esdhc_runtime_resume()), the subsequent call to sdhci_esdhc_imx_hwinit()
will access hardware registers without an active clock. On i.MX platforms,
this can cause a synchronous external abort and crash the kernel.

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

There is also another pre-existing issue in the suspend/resume path regarding
interrupt handling:

[Severity: High]
This isn't a bug introduced by this patch, but can the main SDHCI interrupt
remain permanently disabled after system resume if wakeup setup fails or is
unconfigured?

In sdhci_esdhc_suspend():

    disable_irq(host->irq);
    ret = sdhci_enable_irq_wakeups(host);

If system suspend is initiated on a device where device_may_wakeup(dev) is
true, but sdhci_cd_irq_can_wakeup(host) and mmc_card_wake_sdio_irq(host->mmc)
are both false, sdhci_enable_irq_wakeups() returns false and sets
host->irq_wake_enabled to false.

Then on resume in sdhci_esdhc_resume():

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

Would this skip calling enable_irq() because host->irq_wake_enabled is false,
leaving the main MMC interrupt permanently disabled and causing all command
and data transfers to time out?

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