Re: [PATCH v5 6/8] mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The exact same unchecked pm_runtime_get_sync() bug that this patch fixes in sdhci_esdhc_suspend() is present in sdhci_esdhc_imx_remove().
- [High] Ignoring pm_runtime_force_resume() failure in sdhci_esdhc_resume() can lead to unclocked register access and a kernel panic.
--

--- Patch [6]: [PATCH v5 6/8] mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend ---
commit 96d8d6ad1f0e2f2a06d51f1a48050880d936f2ca
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend

Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() to
simplify error handling. pm_runtime_resume_and_get() automatically
drops the usage counter on failure, avoiding the need for a separate
pm_runtime_put_noidle() call. If it fails, the device is unclocked and
accessing hardware registers would cause a kernel panic, so return the
error immediately.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index d980dea9cad1f..7818f351a58ae 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2061,7 +2061,9 @@ static int sdhci_esdhc_suspend(struct device *dev)
>  	 * 2, make sure the pm_runtime_force_resume() in sdhci_esdhc_resume() really
>  	 *    invoke its ->runtime_resume callback (needs_force_resume = 1).
>  	 */
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
>  
>  	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
>  		(host->tuning_mode != SDHCI_TUNING_MODE_1)) {

[Severity: High]
This is a pre-existing issue, but I noticed similar unchecked runtime PM calls
in this driver that could lead to the same unclocked register access panics
mentioned in the commit message.

In sdhci_esdhc_imx_remove(), the return value of pm_runtime_get_sync() is
ignored immediately before performing a hardware register read:

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 pm_runtime_get_sync() fails to resume the device (for example, due to
clk_prepare_enable() failing) during driver unbind, it returns an error. Does
ignoring this error and immediately reading SDHCI_INT_STATUS cause a
synchronous external abort that crashes the system?

[Severity: High]
This isn't a bug introduced by this patch, but a similar issue exists in the
system resume path.

In sdhci_esdhc_resume(), pm_runtime_force_resume() is called and its error is
ignored immediately before hardware register initialization:

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

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

	/* re-initialize hw state in case it's lost in low power mode */
	sdhci_esdhc_imx_hwinit(host);
	...
}

If pm_runtime_force_resume() fails and returns an error, the driver
proceeds to call sdhci_esdhc_imx_hwinit(), which writes to hardware
registers like ESDHC_WTMK_LVL. Could accessing these registers while
clocks are gated off cause a synchronous external abort on i.MX platforms?

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