Re: [PATCH v4 7/7] mmc: sdhci-esdhc-imx: fix resume error handling

[sashiko-bot@kernel.org]

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

New issues:
- [High] The early return on `pm_runtime_force_resume()` failure intentionally skips software teardowns, leading to permanent resource leaks and IRQ depth nesting.
- [High] The patch's error handling fails to prevent a kernel panic, as the panic is simply deferred to the MMC child device resume.

Pre-existing issues:
- [High] `sdhci_esdhc_imx_remove()` ignores the return value of `pm_runtime_get_sync()`, leading to a kernel panic on device removal if clocks fail to enable.
--

commit 2ca3586c0fc9237c4716073c7d74f73e6e520449
Author: Luke Wang <ziniu.wang_1@nxp.com>

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

This commit checks the return value of pm_runtime_force_resume() in
sdhci_esdhc_resume(). If it fails, the code returns immediately,
intentionally skipping some software teardown steps to prevent accessing
unclocked hardware registers which would cause a kernel panic.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6920f0425508d..586a1debf0981 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2124,12 +2124,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: High]
If pm_runtime_force_resume() fails and we return early here, doesn't this
skip re-enabling the IRQ?

Since sdhci_esdhc_suspend() calls disable_irq(), skipping enable_irq()
leaves the interrupt permanently disabled and results in kernel warnings
due to an unbalanced wake depth on subsequent suspend cycles.


[Severity: High]
Does this early return actually prevent the kernel panic?

The PM core does not stop resuming child devices when a parent fails.
When the MMC class device resumes, it unblocks the block queue and calls
down into mmc_power_up(), which will access the unclocked
SDHCI_HOST_CONTROL register because __mmc_claim_host() ignores the
pm_runtime_get_sync() failure.

Should the code set host->flags |= SDHCI_DEVICE_DEAD before returning
the error so the MMC core is prevented from attempting to access the
unclocked hardware?

> +
> +	mmc_gpio_set_cd_wake(host->mmc, false);
> +
>  	/* re-initialize hw state in case it's lost in low power mode */
>  	sdhci_esdhc_imx_hwinit(host);
>  

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar issue in
sdhci_esdhc_imx_remove():

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_imx_remove() {
    ...
    pm_runtime_get_sync(&pdev->dev);
    dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
    ...
}

Since the return value of pm_runtime_get_sync() is ignored before accessing
hardware registers, could this also cause a synchronous external abort
on device removal if the clocks fail to enable?

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