Re: [PATCH 3/4] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added early return on `pinctrl_pm_select_default_state()` failure skips critical PM state restoration, IRQ re-enablement, and leaks the runtime PM usage counter.
- [Medium] Unconditional transition to default pinctrl state on resume overrides the active pin state retained for wakeup-enabled SDIO devices.

Pre-existing issues:
- [High] Preexisting early returns in both the suspend and resume paths leak the runtime PM usage counter and leave the device's IRQ permanently disabled.
--

commit 36afd1c0e24c2aa82ce799711cc1629a34958ef4
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

SDIO devices such as WiFi may keep power during suspend, so the MMC core skips
full card re-initialization on resume. When DDR_EN is set during
pm_runtime_force_resume() without the default pinctrl state, the loopback
clock is invalid and causes read CRC errors. This patch restores the default
pinctrl state before pm_runtime_force_resume() to fix this issue.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index a944351dbcdff..a3688c94cf580 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2114,6 +2114,10 @@ static int sdhci_esdhc_resume(struct device *dev)
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>  	int ret;
>  
> +	ret = pinctrl_pm_select_default_state(dev);

[Severity: Medium]
Does this unconditional transition to the default pinctrl state override the
active pin state retained for wakeup-enabled SDIO devices?

During suspend, if a device is a wakeup source, it intentionally avoids changing
the pinctrl state to sleep, retaining its active high-speed state.

If we unconditionally select the default state here, could it glitch the SD
bus pins for powered SDIO cards by forcing them out of the retained state
before switching back to high-speed later?

> +	if (ret)
> +		return ret;

[Severity: High]
If pinctrl_pm_select_default_state() fails and we return early, does this skip
important PM state restoration?

It looks like returning here bypasses pm_runtime_force_resume(), enable_irq(),
and the final pm_runtime_put_autosuspend(). Could this permanently leave the
device with its hardware unresumed, its runtime PM usage count elevated, and
its interrupt disabled?

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

[Severity: High]
This is a pre-existing issue, but does this early return leak the PM usage
counter and leave the IRQ disabled?

If mmc_gpio_set_cd_wake() fails, we return early without calling enable_irq()
or pm_runtime_put_autosuspend(), potentially rendering the device unusable.

There appears to be a similar pre-existing early return on pinctrl failure in
sdhci_esdhc_suspend() that skips balancing pm_runtime_get_sync() and
disable_irq().

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