Re: [PATCH v6 8/8] mmc: sdhci-esdhc-imx: fix resume error handling

[sashiko-bot@kernel.org]

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

New issues:
- [High] Returning early on resume failure leaks the PM usage counter and desyncs the IRQ depth, causing a permanent device lockup.

Pre-existing issues:
- [High] Missing synchronize_irq() in runtime suspend causes a race condition with the threaded IRQ handler, leading to a kernel panic.
--

--- Patch [8]: [PATCH v6 8/8] mmc: sdhci-esdhc-imx: fix resume error handling ---
commit 656597a370f7029432d32936bfe95d2095dee2e6
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 the
resume path. It returns immediately on failure to prevent accessing hardware
registers on an unclocked device, while skipping enable_irq() and IRQ wakeup
teardown.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> --- 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)
[ ... ]
> -	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]
Will returning early here leak the PM usage counter and leave the IRQ
permanently disabled?

sdhci_esdhc_suspend() increments the PM usage counter via
pm_runtime_resume_and_get() and the IRQ disable depth via disable_irq().

If pm_runtime_force_resume() fails and we return early, we skip the calls
to enable_irq() and pm_runtime_put_autosuspend() at the end of the function.

On subsequent suspend/resume cycles, disable_irq() and
pm_runtime_resume_and_get() would be called again, stacking the counters.
Even if the hardware failure is transient, a subsequent successful resume
would only decrement the counters once, potentially leaving the device IRQ
disabled until reboot.

Is there a way to balance the suspend operations even when resume fails?

[ ... ]
> @@ -2158,7 +2158,7 @@ static int sdhci_esdhc_resume(struct device *dev)
>  
>  	pm_runtime_put_autosuspend(dev);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int sdhci_esdhc_runtime_suspend(struct device *dev)
>  {
[ ... ]
>  	sdhci_runtime_suspend_host(host);
>  
>  	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>  		mmc_retune_needed(host->mmc);
>  
>  	imx_data->actual_clock = host->mmc->actual_clock;
>  	esdhc_pltfm_set_clock(host, 0);
>  	clk_disable_unprepare(imx_data->clk_per);

[Severity: High]
This is a pre-existing issue, but does this runtime suspend path need to
wait for threaded IRQ handlers to finish before disabling hardware clocks?

The core requests the interrupt with IRQF_SHARED and a threaded handler
(sdhci_thread_irq). sdhci_runtime_suspend_host() masks interrupts at the
controller and calls synchronize_hardirq(), but it does not wait for the
threaded handler to complete.

If a card event triggers the threaded handler just before suspend, it
can run concurrently and read hardware registers (for example via
sdhci_card_event() calling sdhci_get_cd()). 

Because the clocks are disabled shortly after, a concurrent register read
by the threaded handler could cause a kernel panic.

Should synchronize_irq() be called here before powering down the clocks?

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