Re: [PATCH v7 8/8] mmc: sdhci-esdhc-imx: fix resume error handling

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Early return on clock enable failure leaks IRQ wake and disable depths for the host IRQ.
- [Medium] Unnecessary PM runtime usage counter leak on the error path.

Pre-existing issues:
- [Critical] The fix to prevent a kernel panic on unclocked register accesses is incomplete, leaving the unclocked device exposed to child MMC accesses.
- [High] Missing return value check for pm_runtime_get_sync() in sdhci_esdhc_imx_remove() leads to unclocked hardware register access and a kernel panic.
--

--- Patch [8]: [PATCH v7 8/8] mmc: sdhci-esdhc-imx: fix resume error handling ---
commit 2ddb161f38e3bf1f1836d2167b881e211d1fa57f
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: fix resume error handling

This commit adds a return value check for pm_runtime_force_resume() in
sdhci_esdhc_resume() to avoid unclocked hardware register access on failure.
It returns early if clock enablement fails, purposefully skipping IRQ
enablement. Additionally, it removes a redundant return check for
mmc_gpio_set_cd_wake() and explicitly returns 0 on success.

>     The early return intentionally skips enable_irq() and
>     sdhci_disable_irq_wakeups() because the IRQ handler reads
>     SDHCI_INT_STATUS, which would also fault without clocks. The PM runtime
>     usage counter leak only affects this already-broken device instance and
>     is an acceptable tradeoff to preserve system stability.

[Severity: Medium]
Could this usage counter leak be avoided entirely? Since the standard
kernel API pm_runtime_put_noidle() drops the usage counter without invoking
any idle callbacks or accessing hardware, would it be cleaner to use it
here instead of leaking the counter on the error path?

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 0824f598bf598..6ab59d3e346f9 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2123,12 +2123,12 @@ static int sdhci_esdhc_resume(struct device *dev)
>  			dev_warn(dev, "Failed to restore pinctrl state\n");
>  	}
>  
> -	pm_runtime_force_resume(dev);
> -
> -	ret = mmc_gpio_set_cd_wake(host->mmc, false);
> +	ret = pm_runtime_force_resume(dev);
>  	if (ret)
>  		return ret;

[Severity: Medium]
Does this early return bypass software-level cleanup of IRQ wake and
disable states? If pm_runtime_force_resume() fails, the function returns
early, bypassing enable_irq(host->irq) and sdhci_disable_irq_wakeups(host).

Because the PM core does not abort system resume on a device resume failure,
a subsequent system suspend will call the suspend callback again.
This unconditionally calls disable_irq() and sdhci_enable_irq_wakeups().

Can this cause the IRQ disable depth and wake depth to accumulate unbounded
on subsequent suspends?

[Severity: Critical]
This isn't a bug introduced by this patch, but does the early return leave
the unclocked device exposed to child MMC accesses?

Returning an error from a parent's system resume callback does not abort
the system resume process for children. The PM core continues and invokes
the resume callback for the child mmc_card device, which unconditionally
triggers sdhci_set_ios() on the host controller and accesses hardware
registers.

Because the SDHCI_DEVICE_DEAD flag is not set before returning early,
sdhci_set_ios() does not abort and accesses the unclocked hardware
registers, potentially triggering the same kernel panic this patch was
intended to fix. Would setting host->flags |= SDHCI_DEVICE_DEAD; before
returning prevent this?

[ ... ]

This isn't a bug introduced by this patch, but looking at
sdhci_esdhc_imx_remove() in the same file:

    pm_runtime_get_sync(&pdev->dev);
    dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);

[Severity: High]
Does this code ignore the return value of pm_runtime_get_sync()?

If resuming the device fails, the clocks remain disabled. The immediate
next instruction accesses SDHCI_INT_STATUS. Can this trigger a kernel panic
due to unclocked hardware access, similar to the one being fixed in the
resume path?

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