Re: [PATCH v5 8/8] 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 delays a kernel panic because the MMC core is not notified that the device is dead, leading to subsequent unclocked hardware accesses.
- [Medium] The card detect GPIO IRQ is incorrectly left configured as a system wake source on the error path.

Pre-existing issues:
- [High] The driver ignores the return value of `pm_runtime_get_sync()` in the unbind/remove path, leading to a kernel panic.
--

--- Patch [8]: [PATCH v5 8/8] mmc: sdhci-esdhc-imx: fix resume error handling ---
commit eb59f1a91caafc261d97666e5d0b0450cef19279
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(). If it fails, it returns early to prevent unclocked
hardware accesses. It also simplifies the return value handling on success.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6cb84690fde4e..f4bfb14423e4d 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2126,12 +2126,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 we return early here without setting host->flags |= SDHCI_DEVICE_DEAD,
will the MMC core still attempt to access unclocked hardware?

The PM core will continue system resume and eventually invoke
mmc_resume_host() on the child MMC device. The MMC core would then call host
operations like sdhci_set_ios(), which attempt to read and write SDHCI
hardware registers on an unclocked controller. Could this trigger a
synchronous external abort and result in a kernel panic?

[Severity: Medium]
Does this early return skip the GPIO CD wake IRQ cleanup, leaving the
wake source configured?

During sdhci_esdhc_suspend(), the card detect IRQ is configured as a wake
source via mmc_gpio_set_cd_wake(..., true). Because the card detect
configuration interacts with the GPIO controller and has no dependency on
SDHCI clocks, should it be executed on the error path to avoid leaking the
wake source configuration?

>  
> +	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 does ignoring the return
value of pm_runtime_get_sync() in sdhci_esdhc_imx_remove() lead to the same
kernel panic on unclocked hardware access?

In 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);

If the device is in an error state and clocks fail to enable,
pm_runtime_get_sync() returns a negative error. By immediately executing
readl(host->ioaddr + SDHCI_INT_STATUS) on an unclocked device, could this
trigger a synchronous external abort when the device is unbound?

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