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

Pre-existing issues:
- [High] Missing cleanup of the PM usage counter and IRQ state on error paths in `sdhci_esdhc_suspend()` and `sdhci_esdhc_resume()`.
--

--- Patch [6]: [PATCH v6 6/8] mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend ---
commit a5ad87bca26adcf9d30b3e6b6dd0a1a1287863a6
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.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6e792bee25bfa..ec531a9451c4e 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;

[Severity: High]
This is a pre-existing issue, but since we are modifying the PM reference
handling here, does the rest of sdhci_esdhc_suspend() properly clean up the
PM usage counter and IRQ state on its error paths?

For example, further down in sdhci_esdhc_suspend(), if
pinctrl_pm_select_sleep_state() fails:

		ret = pinctrl_pm_select_sleep_state(dev);
		if (ret)
			return ret;

Does this early return exit the function without dropping the PM reference
acquired above or re-enabling IRQs?

Similarly, if mmc_gpio_set_cd_wake() fails:

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

	pm_runtime_force_suspend(dev);

	return ret;

If this fails, the function returns an error, which causes the PM core to
abort system suspend and skip the device's resume callback.

Does this leak the PM reference and leave the device with a permanently
disabled IRQ?

Also, in sdhci_esdhc_resume(), if mmc_gpio_set_cd_wake() fails:

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

Could this early return skip enable_irq() and pm_runtime_put_autosuspend()
at the end of the function, permanently breaking the device state?

[ ... ]

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