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: [email protected]
Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
Signed-off-by: Mantas Pucka <[email protected]>
---
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 <[email protected]>
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: [email protected]
> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
> Signed-off-by: Mantas Pucka <[email protected]>
Acked-by: Adrian Hunter <[email protected]>
> ---
> 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 <[email protected]> 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: [email protected]
> > Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
> > Signed-off-by: Mantas Pucka <[email protected]>
>
> Acked-by: Adrian Hunter <[email protected]>
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 <[email protected]> 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: [email protected]
>>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
>>> Signed-off-by: Mantas Pucka <[email protected]>
>>
>> Acked-by: Adrian Hunter <[email protected]>
>
> 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 <[email protected]> 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: [email protected]
> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
> Signed-off-by: Mantas Pucka <[email protected]>
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 <[email protected]>
>
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 <[email protected]> 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: [email protected]
>>>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
>>>> Signed-off-by: Mantas Pucka <[email protected]>
>>>
>>> Acked-by: Adrian Hunter <[email protected]>
>>
>> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> >>>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
> >>>> Signed-off-by: Mantas Pucka <[email protected]>
> >>>
> >>> Acked-by: Adrian Hunter <[email protected]>
> >>
> >> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
>>>>>> Fixes: 67e6db113c90 ("mmc: sdhci-msm: Add pm_runtime and system PM support")
>>>>>> Signed-off-by: Mantas Pucka <[email protected]>
>>>>>
>>>>> Acked-by: Adrian Hunter <[email protected]>
>>>>
>>>> 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.