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

On Wed, Jul 15, 2026 at 03:18:18PM +0800, [email protected] wrote:
> From: Luke Wang <[email protected]>
> 
> Check pm_runtime_force_resume() return value in resume. If it fails
> (clock enable failure), return immediately since accessing hardware
> registers on an unclocked device would cause a kernel panic.
> 

The commit message says "would" which implies maybe...  Is this something
that happens in real life?


> The early return intentionally skips enable_irq() and
> sdhci_disable_irq_wakeups() because the IRQ handler reads
> SDHCI_INT_STATUS, which would also fault without clocks. The PM runtime
> usage counter leak only affects this already-broken device instance and
> is an acceptable tradeoff to preserve system stability.
> 
> Remove the return value check for mmc_gpio_set_cd_wake(host->mmc, false)
> since disable_irq_wake() called internally always returns 0.

I kind of agree with Sashiko here that keeping the check is better future
proofing.

> 
> Also return 0 explicitly on the success path instead of propagating
> stale return values.

Unrelated cleans...  :/

> 
> Fixes: 676a83855614 ("mmc: host: sdhci-esdhc-imx: refactor the system PM logic")
> Acked-by: Adrian Hunter <[email protected]>
> Reviewed-by: Frank Li <[email protected]>
> Signed-off-by: Luke Wang <[email protected]>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 290a3172931b..18f4905c15b9 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2123,12 +2123,12 @@ static int sdhci_esdhc_resume(struct device *dev)
>  			dev_warn(dev, "Failed to restore pinctrl state\n");
>  	}
>  
> -	pm_runtime_force_resume(dev);
> -
> -	ret = mmc_gpio_set_cd_wake(host->mmc, false);
> +	ret = pm_runtime_force_resume(dev);
>  	if (ret)
>  		return ret;
>  
> +	mmc_gpio_set_cd_wake(host->mmc, false);
> +

copy-paste-checker says that similar code exists in:

drivers/mmc/host/sdhci-tegra.c
  1879  static int sdhci_tegra_resume(struct device *dev)
  1880  {
  1881          struct sdhci_host *host = dev_get_drvdata(dev);
  1882          int ret;
  1883  
  1884          ret = mmc_gpio_set_cd_wake(host->mmc, false);
  1885          if (ret)
  1886                  return ret;
  1887  
  1888          ret = pm_runtime_force_resume(dev);
  1889          if (ret)
  1890                  return ret;

In this case we are calling mmc_gpio_set_cd_wake() before calling
resume so it's surprising that that doesn't cause a panic..

  1891  
  1892          sdhci_tegra_program_stream_id(host);
  1893  
  1894          ret = sdhci_resume_host(host);
  1895          if (ret)
  1896                  goto disable_clk;
  1897  
  1898          if (host->mmc->caps2 & MMC_CAP2_CQE) {
  1899                  ret = cqhci_resume(host->mmc);
  1900                  if (ret)
  1901                          goto suspend_host;
  1902          }
  1903  
  1904          return 0;
  1905  
  1906  suspend_host:
  1907          sdhci_suspend_host(host);
  1908  disable_clk:
  1909          pm_runtime_force_suspend(dev);
  1910          return ret;
  1911  }

regards,
dan carpenter