| Message ID | 20240321-sdhci-mmc-suspend-v1-1-fbc555a64400@8devices.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-msm: pervent access to suspended controller | expand |
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> > --- > 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 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? 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 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, 21 Mar 2024 at 15:30, Mantas Pucka <mantas@8devices.com> 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> Applied for fixes, thanks! 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, > -- > Mantas Pucka <mantas@8devices.com> >
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.
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,