diff mbox series

mmc: sdhci-msm: pervent access to suspended controller

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

Commit Message

Mantas Pucka March 21, 2024, 2:30 p.m. UTC 
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,

Comments

Adrian Hunter March 28, 2024, 2:20 p.m. UTC | #1 
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,
>>
Ulf Hansson April 4, 2024, 9:13 p.m. UTC | #2 
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
Adrian Hunter April 16, 2024, 2:27 p.m. UTC | #3 
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 mbox series

Patch

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