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

New issues:
- [Medium] Spurious pinctrl warnings on every system resume for platforms without pinctrl configuration.

Pre-existing issues:
- [Critical] Unchecked return value of pm_runtime_force_resume() may lead to unclocked register access and system crash.
- [High] Error paths in system suspend and resume callbacks fail to balance the runtime PM usage counter.
- [High] esdhc_change_pinstate() uses IS_ERR() instead of IS_ERR_OR_NULL(), leading to a potential NULL pointer dereference on misconfigured DTs.
--

--- Patch [4]: [PATCH v6 4/8] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume ---
commit 8ca4820b7ac16444e14c14d08a8eaf0a8e80915f
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 and directly restores the host
controller's ios timing to match the card. This patch restores the pinctrl
state based on current timing mode before pm_runtime_force_resume() is
called.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 2da5c8ab3e622..bc0864cb22b49 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2115,6 +2115,12 @@ static int sdhci_esdhc_resume(struct device *dev)
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>  	int ret;
>  
> +	if (!device_may_wakeup(dev)) {
> +		ret = esdhc_change_pinstate(host, host->timing);

[Severity: High]
This is a pre-existing issue, but does esdhc_change_pinstate() fail to
catch a NULL pointer on misconfigured device trees?

In esdhc_change_pinstate(), it uses IS_ERR() instead of IS_ERR_OR_NULL():

	case MMC_TIMING_UHS_DDR50:
		if (IS_ERR(imx_data->pins_100mhz))
			return -EINVAL;
		pinctrl = imx_data->pins_100mhz;

If a misconfigured device tree forces UHS capabilities, the MMC core
enables UHS timings, but pins_100mhz may remain NULL. Since IS_ERR(NULL)
evaluates to false, it assigns NULL to pinctrl.

This NULL pointer is directly passed to the pinctrl subsystem:

	return pinctrl_select_state(imx_data->pinctrl, pinctrl);

This causes a panic when dereferenced by pinctrl_commit_state().

> +		if (ret)
> +			dev_warn(dev, "Failed to restore pinctrl state\n");

[Severity: Medium]
Is this warning going to trigger on every system resume for platforms
without pinctrl?

Platforms without pinctrl correctly initialize imx_data->pinctrl to an
ERR_PTR in probe(), treating it as non-fatal.

When esdhc_change_pinstate() is called, it returns -EINVAL if pinctrl
is an ERR_PTR:

	if (IS_ERR(imx_data->pinctrl))
		return -EINVAL;

This causes dev_warn() to spam the kernel log on unaffected hardware on
every system resume.

> +	}
> +
>  	pm_runtime_force_resume(dev);

[Severity: Critical]
This is a pre-existing issue, but does ignoring the return value of
pm_runtime_force_resume() risk an unclocked register access?

If the runtime resume callback fails to enable hardware clocks, the error
is returned but completely ignored here.

The driver then proceeds to access memory-mapped registers unconditionally
in sdhci_esdhc_imx_hwinit(). Accessing peripheral registers without clocks
enabled on i.MX platforms causes a Synchronous External Abort,
deterministically crashing the system.

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

[Severity: High]
This is a pre-existing issue, but do the error paths in the system suspend
and resume callbacks fail to balance the runtime PM usage counter?

In sdhci_esdhc_resume(), if mmc_gpio_set_cd_wake() fails, the function
returns early:

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

This bypasses pm_runtime_put_autosuspend(), leaking the usage counter
incremented during suspend.

Similarly, in sdhci_esdhc_suspend(), if pinctrl_pm_select_sleep_state()
fails, it returns early without calling pm_runtime_put() to balance
the preceding pm_runtime_get_sync().

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