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

[Luke Wang (OSS)]

> -----Original Message-----
> From: Dan Carpenter <error27@xxxxxxxxx>
> Sent: Friday, July 17, 2026 6:11 PM
> To: Luke Wang (OSS) <ziniu.wang_1@xxxxxxxxxxx>
> Cc: adrian.hunter@xxxxxxxxx; ulfh@xxxxxxxxxx; Bough Chen
> <haibo.chen@xxxxxxx>; Frank Li <frank.li@xxxxxxx>;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; dl-S32 <S32@xxxxxxx>;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> harshit.m.mogalapalli@xxxxxxxxxx
> Subject: Re: [PATCH v7 8/8] mmc: sdhci-esdhc-imx: fix resume error handling
> 
> [You don't often get email from error27@xxxxxxxxx. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Wed, Jul 15, 2026 at 03:18:18PM +0800, ziniu.wang_1@xxxxxxxxxxx
> wrote:
> > From: Luke Wang <ziniu.wang_1@xxxxxxx>
> >
> > 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?

Accessing registers on an unclocked device does cause a panic. The specific case here 
is `pm_runtime_force_resume()` failing on clock enable, which I have not actually hit
in practice - this error handling was suggested by Sashiko during the v1 review, 
so it's defensive hardening rather than a fix for an observed crash.

> 
> 
> > 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.

Fair point. In the `on=false` path `disable_irq_wake()` always returns 0 today, 
so the check is a no-op. And even if the internal implementation changes and
returns an error, it wouldn't matter functionally here: the device is already resumed,
so a failure to clear CD wake doesn't break anything. 

> 
> >
> > 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 <adrian.hunter@xxxxxxxxx>
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > Signed-off-by: Luke Wang <ziniu.wang_1@xxxxxxx>
> > ---
> >  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..

`mmc_gpio_set_cd_wake()` doesn't touch any SDHCI registers or need the clock.
It only calls `enable/disable_irq_wake()` on the CD IRQ (see drivers/mmc/core/slot-gpio.c). 
So it is safe to call it before or after resume, which is why Tegra can call it first without a panic.

Regards,
Luke Wang

> 
>   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