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,
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 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():
- dwcmshc_runtime_suspend() : only turns off the card clock
via SDHCI_CLOCK_CONTROL register, so registers are presumably still accessible
gl9763e_runtime_suspend() : ditto
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():
- dwcmshc_runtime_suspend() : only turns off the card clock
via SDHCI_CLOCK_CONTROL register, so registers are presumably still accessible
gl9763e_runtime_suspend() : ditto
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.
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
linux-stable-mirror@lists.linaro.org
-
Adrian Hunter -
Mantas Pucka -
Ulf Hansson