Message ID | 20240321-sdhci-mmc-suspend-v1-1-fbc555a64400@8devices.com |
---|---|
State | Accepted |
Commit | f8def10f73a516b771051a2f70f2f0446902cb4f |
Headers | show |
Series | mmc: sdhci-msm: pervent access to suspended controller | expand |
On 27/03/24 17:17, Ulf Hansson wrote: > On Tue, 26 Mar 2024 at 11:25, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 21/03/24 16:30, Mantas Pucka wrote: >>> Generic sdhci code registers LED device and uses host->runtime_suspended >>> flag to protect access to it. The sdhci-msm driver doesn't set this flag, >>> which causes a crash when LED is accessed while controller is runtime >>> suspended. Fix this by setting the flag correctly. >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support") >>> Signed-off-by: Mantas Pucka <mantas@8devices.com> >> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > > Looks like this problem may exist for other sdhci drivers too. In > particular for those that enables runtime PM, don't set > SDHCI_QUIRK_NO_LED and don't use sdhci_runtime|suspend_resume_host(). > > Don't know if there is a better way to address this, if not on a case > by case basis. Do you have any thoughts about this? Yes probably case by case, but I will look at it. > > Kind regards > Uffe > >> >>> --- >>> drivers/mmc/host/sdhci-msm.c | 16 +++++++++++++++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c >>> index 668e0aceeeba..e113b99a3eab 100644 >>> --- a/drivers/mmc/host/sdhci-msm.c >>> +++ b/drivers/mmc/host/sdhci-msm.c >>> @@ -2694,6 +2694,11 @@ static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev) >>> struct sdhci_host *host = dev_get_drvdata(dev); >>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&host->lock, flags); >>> + host->runtime_suspended = true; >>> + spin_unlock_irqrestore(&host->lock, flags); >>> >>> /* Drop the performance vote */ >>> dev_pm_opp_set_rate(dev, 0); >>> @@ -2708,6 +2713,7 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) >>> struct sdhci_host *host = dev_get_drvdata(dev); >>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); >>> + unsigned long flags; >>> int ret; >>> >>> ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), >>> @@ -2726,7 +2732,15 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) >>> >>> dev_pm_opp_set_rate(dev, msm_host->clk_rate); >>> >>> - return sdhci_msm_ice_resume(msm_host); >>> + ret = sdhci_msm_ice_resume(msm_host); >>> + if (ret) >>> + return ret; >>> + >>> + spin_lock_irqsave(&host->lock, flags); >>> + host->runtime_suspended = false; >>> + spin_unlock_irqrestore(&host->lock, flags); >>> + >>> + return ret; >>> } >>> >>> static const struct dev_pm_ops sdhci_msm_pm_ops = { >>> >>> --- >>> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba >>> change-id: 20240321-sdhci-mmc-suspend-34f4af1d0286 >>> >>> Best regards, >>
On Thu, 4 Apr 2024 at 20:42, Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 28/03/24 16:20, Adrian Hunter wrote: > > On 27/03/24 17:17, Ulf Hansson wrote: > >> On Tue, 26 Mar 2024 at 11:25, Adrian Hunter <adrian.hunter@intel.com> wrote: > >>> > >>> On 21/03/24 16:30, Mantas Pucka wrote: > >>>> Generic sdhci code registers LED device and uses host->runtime_suspended > >>>> flag to protect access to it. The sdhci-msm driver doesn't set this flag, > >>>> which causes a crash when LED is accessed while controller is runtime > >>>> suspended. Fix this by setting the flag correctly. > >>>> > >>>> Cc: stable@vger.kernel.org > >>>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support") > >>>> Signed-off-by: Mantas Pucka <mantas@8devices.com> > >>> > >>> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > >> > >> Looks like this problem may exist for other sdhci drivers too. In > >> particular for those that enables runtime PM, don't set > >> SDHCI_QUIRK_NO_LED and don't use sdhci_runtime|suspend_resume_host(). > >> > >> Don't know if there is a better way to address this, if not on a case > >> by case basis. Do you have any thoughts about this? > > > > Yes probably case by case, but I will look at it. > > There seem to be 3 that use runtime pm but not > sdhci_runtime_suspend_host(): > > 1. dwcmshc_runtime_suspend() : only turns off the card clock > via SDHCI_CLOCK_CONTROL register, so registers are presumably > still accessible > > 2. gl9763e_runtime_suspend() : ditto > > 3. sdhci_tegra_runtime_suspend() : disables the functional > clock via clk_disable_unprepare(), so registers are presumably > still accessible > > sdhci_msm_runtime_suspend() is different because it also turns > off the interface clock. > > But it looks like there are no similar cases. Not sure we should care, but it still looks a bit fragile to me. We may also have a power-domain hooked up to the device, that could get power gated too, in which case it's likely affecting the access to registers. Kind regards Uffe
On 5/04/24 00:13, Ulf Hansson wrote: > On Thu, 4 Apr 2024 at 20:42, Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 28/03/24 16:20, Adrian Hunter wrote: >>> On 27/03/24 17:17, Ulf Hansson wrote: >>>> On Tue, 26 Mar 2024 at 11:25, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> >>>>> On 21/03/24 16:30, Mantas Pucka wrote: >>>>>> Generic sdhci code registers LED device and uses host->runtime_suspended >>>>>> flag to protect access to it. The sdhci-msm driver doesn't set this flag, >>>>>> which causes a crash when LED is accessed while controller is runtime >>>>>> suspended. Fix this by setting the flag correctly. >>>>>> >>>>>> Cc: stable@vger.kernel.org >>>>>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support") >>>>>> Signed-off-by: Mantas Pucka <mantas@8devices.com> >>>>> >>>>> Acked-by: Adrian Hunter <adrian.hunter@intel.com> >>>> >>>> Looks like this problem may exist for other sdhci drivers too. In >>>> particular for those that enables runtime PM, don't set >>>> SDHCI_QUIRK_NO_LED and don't use sdhci_runtime|suspend_resume_host(). >>>> >>>> Don't know if there is a better way to address this, if not on a case >>>> by case basis. Do you have any thoughts about this? >>> >>> Yes probably case by case, but I will look at it. >> >> There seem to be 3 that use runtime pm but not >> sdhci_runtime_suspend_host(): >> >> 1. dwcmshc_runtime_suspend() : only turns off the card clock >> via SDHCI_CLOCK_CONTROL register, so registers are presumably >> still accessible >> >> 2. gl9763e_runtime_suspend() : ditto >> >> 3. sdhci_tegra_runtime_suspend() : disables the functional >> clock via clk_disable_unprepare(), so registers are presumably >> still accessible >> >> sdhci_msm_runtime_suspend() is different because it also turns >> off the interface clock. >> >> But it looks like there are no similar cases. > > Not sure we should care, but it still looks a bit fragile to me. We > may also have a power-domain hooked up to the device, that could get > power gated too, in which case it's likely affecting the access to > registers. Thought some more about this, but there isn't an easy way to know if drivers are catering for SDIO card interrupt or SD card detect interrupt.
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index 668e0aceeeba..e113b99a3eab 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -2694,6 +2694,11 @@ static __maybe_unused int sdhci_msm_runtime_suspend(struct device *dev) struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + unsigned long flags; + + spin_lock_irqsave(&host->lock, flags); + host->runtime_suspended = true; + spin_unlock_irqrestore(&host->lock, flags); /* Drop the performance vote */ dev_pm_opp_set_rate(dev, 0); @@ -2708,6 +2713,7 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) struct sdhci_host *host = dev_get_drvdata(dev); struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host); + unsigned long flags; int ret; ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks), @@ -2726,7 +2732,15 @@ static __maybe_unused int sdhci_msm_runtime_resume(struct device *dev) dev_pm_opp_set_rate(dev, msm_host->clk_rate); - return sdhci_msm_ice_resume(msm_host); + ret = sdhci_msm_ice_resume(msm_host); + if (ret) + return ret; + + spin_lock_irqsave(&host->lock, flags); + host->runtime_suspended = false; + spin_unlock_irqrestore(&host->lock, flags); + + return ret; } static const struct dev_pm_ops sdhci_msm_pm_ops = {
Generic sdhci code registers LED device and uses host->runtime_suspended flag to protect access to it. The sdhci-msm driver doesn't set this flag, which causes a crash when LED is accessed while controller is runtime suspended. Fix this by setting the flag correctly. Cc: stable@vger.kernel.org Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support") Signed-off-by: Mantas Pucka <mantas@8devices.com> --- drivers/mmc/host/sdhci-msm.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240321-sdhci-mmc-suspend-34f4af1d0286 Best regards,