[PATCH] mmc: sdhci-msm: pervent access to suspended controller

[PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Mantas Pucka]

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

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>

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Adrian Hunter]

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,

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Ulf Hansson]

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

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Adrian Hunter]

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

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Adrian Hunter]

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.

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Ulf Hansson]

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

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Adrian Hunter]

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.

Re: [PATCH] mmc: sdhci-msm: pervent access to suspended controller

[Ulf Hansson]

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