| Message ID | 20220622051215.34063-1-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init | expand |
On 22/06/22 08:12, Tony Lindgren wrote: > We need runtime PM enabled early in probe before sdhci_setup_host() for > sdhci_omap_set_capabilities(). But on the first runtime resume we must > not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been > called yet. Let's check for an initialized controller like we already do > for context restore to fix a lockdep warning. > > Fixes: f433e8aac6b9 ("mmc: sdhci-omap: Implement PM runtime functions") > Reported-by: Yegor Yefremov <yegorslists@googlemail.com> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Tony Lindgren <tony@atomide.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > > Changes since v1: > > - Add comments for why runtime PM is needed before sdhci_setup_host() > as suggested by Adrian > > --- > drivers/mmc/host/sdhci-omap.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -1298,8 +1298,9 @@ static int sdhci_omap_probe(struct platform_device *pdev) > /* > * omap_device_pm_domain has callbacks to enable the main > * functional clock, interface clock and also configure the > - * SYSCONFIG register of omap devices. The callback will be invoked > - * as part of pm_runtime_get_sync. > + * SYSCONFIG register to clear any boot loader set voltage > + * capabilities before calling sdhci_setup_host(). The > + * callback will be invoked as part of pm_runtime_get_sync. > */ > pm_runtime_use_autosuspend(dev); > pm_runtime_set_autosuspend_delay(dev, 50); > @@ -1441,7 +1442,8 @@ static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev) > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); > > - sdhci_runtime_suspend_host(host); > + if (omap_host->con != -EINVAL) > + sdhci_runtime_suspend_host(host); > > sdhci_omap_context_save(omap_host); > > @@ -1458,10 +1460,10 @@ static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev) > > pinctrl_pm_select_default_state(dev); > > - if (omap_host->con != -EINVAL) > + if (omap_host->con != -EINVAL) { > sdhci_omap_context_restore(omap_host); > - > - sdhci_runtime_resume_host(host, 0); > + sdhci_runtime_resume_host(host, 0); > + } > > return 0; > }
On Wed, 22 Jun 2022 at 07:12, Tony Lindgren <tony@atomide.com> wrote: > > We need runtime PM enabled early in probe before sdhci_setup_host() for > sdhci_omap_set_capabilities(). But on the first runtime resume we must > not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been > called yet. Let's check for an initialized controller like we already do > for context restore to fix a lockdep warning. Thanks for explaining the background to the problem. However, looking a bit closer I am worried that the error path in ->probe() is broken too. It seems in the error path, at the label "err_rpm_put", there is a call to pm_runtime_put_autosuspend(). This doesn't really guarantee that the ->runtime_suspend() callback will be invoked, which I guess is the assumption, don't you think? That said, I wonder if it would not be easier to convert the ->probe() function to make use of pm_runtime_get_noresume() and pm_runtime_set_active() instead. In this way the ->probe() function becomes responsible of turning on/off the resources "manually" that it requires to probe (and when it fails to probe), while we can keep the ->runtime_suspend|resume() callbacks simpler. Did that make sense to you? Kind regards Uffe > > Fixes: f433e8aac6b9 ("mmc: sdhci-omap: Implement PM runtime functions") > Reported-by: Yegor Yefremov <yegorslists@googlemail.com> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > Changes since v1: > > - Add comments for why runtime PM is needed before sdhci_setup_host() > as suggested by Adrian > > --- > drivers/mmc/host/sdhci-omap.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c > --- a/drivers/mmc/host/sdhci-omap.c > +++ b/drivers/mmc/host/sdhci-omap.c > @@ -1298,8 +1298,9 @@ static int sdhci_omap_probe(struct platform_device *pdev) > /* > * omap_device_pm_domain has callbacks to enable the main > * functional clock, interface clock and also configure the > - * SYSCONFIG register of omap devices. The callback will be invoked > - * as part of pm_runtime_get_sync. > + * SYSCONFIG register to clear any boot loader set voltage > + * capabilities before calling sdhci_setup_host(). The > + * callback will be invoked as part of pm_runtime_get_sync. > */ > pm_runtime_use_autosuspend(dev); > pm_runtime_set_autosuspend_delay(dev, 50); > @@ -1441,7 +1442,8 @@ static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev) > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); > > - sdhci_runtime_suspend_host(host); > + if (omap_host->con != -EINVAL) > + sdhci_runtime_suspend_host(host); > > sdhci_omap_context_save(omap_host); > > @@ -1458,10 +1460,10 @@ static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev) > > pinctrl_pm_select_default_state(dev); > > - if (omap_host->con != -EINVAL) > + if (omap_host->con != -EINVAL) { > sdhci_omap_context_restore(omap_host); > - > - sdhci_runtime_resume_host(host, 0); > + sdhci_runtime_resume_host(host, 0); > + } > > return 0; > } > -- > 2.36.1
* Ulf Hansson <ulf.hansson@linaro.org> [220623 12:55]: > On Wed, 22 Jun 2022 at 07:12, Tony Lindgren <tony@atomide.com> wrote: > > > > We need runtime PM enabled early in probe before sdhci_setup_host() for > > sdhci_omap_set_capabilities(). But on the first runtime resume we must > > not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been > > called yet. Let's check for an initialized controller like we already do > > for context restore to fix a lockdep warning. > > Thanks for explaining the background to the problem. However, looking > a bit closer I am worried that the error path in ->probe() is broken > too. > > It seems in the error path, at the label "err_rpm_put", there is a > call to pm_runtime_put_autosuspend(). This doesn't really guarantee > that the ->runtime_suspend() callback will be invoked, which I guess > is the assumption, don't you think? OK I'll check and send a separate patch for that. > That said, I wonder if it would not be easier to convert the ->probe() > function to make use of pm_runtime_get_noresume() and > pm_runtime_set_active() instead. In this way the ->probe() function > becomes responsible of turning on/off the resources "manually" that it > requires to probe (and when it fails to probe), while we can keep the > ->runtime_suspend|resume() callbacks simpler. > > Did that make sense to you? Simpler would be better :) We need to call pm_runtime_get_sync() at some point though to enable the parent device hierarchy. Just calling the sdhci_omap runtime functions is not enough. And we still need to check for the valid context too. Also I'm not convinced that calling pm_runtime_get_sync() on the parent device would do the right thing on old omap3 devices without bigger changes.. But maybe you have some better ideas that I'm not considering. Regards, Tony
On Mon, 27 Jun 2022 at 08:13, Tony Lindgren <tony@atomide.com> wrote: > > * Ulf Hansson <ulf.hansson@linaro.org> [220623 12:55]: > > On Wed, 22 Jun 2022 at 07:12, Tony Lindgren <tony@atomide.com> wrote: > > > > > > We need runtime PM enabled early in probe before sdhci_setup_host() for > > > sdhci_omap_set_capabilities(). But on the first runtime resume we must > > > not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been > > > called yet. Let's check for an initialized controller like we already do > > > for context restore to fix a lockdep warning. > > > > Thanks for explaining the background to the problem. However, looking > > a bit closer I am worried that the error path in ->probe() is broken > > too. > > > > It seems in the error path, at the label "err_rpm_put", there is a > > call to pm_runtime_put_autosuspend(). This doesn't really guarantee > > that the ->runtime_suspend() callback will be invoked, which I guess > > is the assumption, don't you think? > > OK I'll check and send a separate patch for that. > > > That said, I wonder if it would not be easier to convert the ->probe() > > function to make use of pm_runtime_get_noresume() and > > pm_runtime_set_active() instead. In this way the ->probe() function > > becomes responsible of turning on/off the resources "manually" that it > > requires to probe (and when it fails to probe), while we can keep the > > ->runtime_suspend|resume() callbacks simpler. > > > > Did that make sense to you? > > Simpler would be better :) We need to call pm_runtime_get_sync() at some > point though to enable the parent device hierarchy. Is there a parent device that has runtime PM enabled? In other cases, it should be fine to use pm_runtime_set_active() during ->probe(). > Just calling the > sdhci_omap runtime functions is not enough. And we still need to check > for the valid context too. Also I'm not convinced that calling > pm_runtime_get_sync() on the parent device would do the right thing on > old omap3 devices without bigger changes.. I certainly agree. The parent should not be managed directly by the sdhci driver. One thing that can be discussed though, is whether pm_runtime_set_active() actually should runtime resume the parent, which would make the behaviour consistent with how suppliers are being treated. > But maybe you have some better > ideas that I'm not considering. I can try to draft a patch, if that would help? But, let's finalize the discussion above first (apologize for the delay). Kind regards Uffe
* Ulf Hansson <ulf.hansson@linaro.org> [220712 09:52]: > On Mon, 27 Jun 2022 at 08:13, Tony Lindgren <tony@atomide.com> wrote: > > > > * Ulf Hansson <ulf.hansson@linaro.org> [220623 12:55]: > > > On Wed, 22 Jun 2022 at 07:12, Tony Lindgren <tony@atomide.com> wrote: > > > > > > > > We need runtime PM enabled early in probe before sdhci_setup_host() for > > > > sdhci_omap_set_capabilities(). But on the first runtime resume we must > > > > not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been > > > > called yet. Let's check for an initialized controller like we already do > > > > for context restore to fix a lockdep warning. > > > > > > Thanks for explaining the background to the problem. However, looking > > > a bit closer I am worried that the error path in ->probe() is broken > > > too. > > > > > > It seems in the error path, at the label "err_rpm_put", there is a > > > call to pm_runtime_put_autosuspend(). This doesn't really guarantee > > > that the ->runtime_suspend() callback will be invoked, which I guess > > > is the assumption, don't you think? > > > > OK I'll check and send a separate patch for that. > > > > > That said, I wonder if it would not be easier to convert the ->probe() > > > function to make use of pm_runtime_get_noresume() and > > > pm_runtime_set_active() instead. In this way the ->probe() function > > > becomes responsible of turning on/off the resources "manually" that it > > > requires to probe (and when it fails to probe), while we can keep the > > > ->runtime_suspend|resume() callbacks simpler. > > > > > > Did that make sense to you? > > > > Simpler would be better :) We need to call pm_runtime_get_sync() at some > > point though to enable the parent device hierarchy. > > Is there a parent device that has runtime PM enabled? Yes there is the interconnect target module device as the parent with runtime PM enabled. So the sdhci-omap driver needs the parent enabled. > In other cases, it should be fine to use pm_runtime_set_active() > during ->probe(). Yup, this can't be done here though AFAIK. Something needs to enable runtime PM for the parent device to have the sdhci registers accessible. > > Just calling the > > sdhci_omap runtime functions is not enough. And we still need to check > > for the valid context too. Also I'm not convinced that calling > > pm_runtime_get_sync() on the parent device would do the right thing on > > old omap3 devices without bigger changes.. > > I certainly agree. The parent should not be managed directly by the > sdhci driver. OK > One thing that can be discussed though, is whether > pm_runtime_set_active() actually should runtime resume the parent, > which would make the behaviour consistent with how suppliers are being > treated. Hmm yeah that's an interesting idea. > > But maybe you have some better > > ideas that I'm not considering. > > I can try to draft a patch, if that would help? But, let's finalize > the discussion above first (apologize for the delay). OK. Should we apply the $subject patch to fix the splat meanwhile though? Seems like what you're suggesting may take some more discussion on the mailing lists. Regrads, Tony
On Tue, 12 Jul 2022 at 14:18, Tony Lindgren <tony@atomide.com> wrote: > > * Ulf Hansson <ulf.hansson@linaro.org> [220712 09:52]: > > On Mon, 27 Jun 2022 at 08:13, Tony Lindgren <tony@atomide.com> wrote: > > > > > > * Ulf Hansson <ulf.hansson@linaro.org> [220623 12:55]: > > > > On Wed, 22 Jun 2022 at 07:12, Tony Lindgren <tony@atomide.com> wrote: > > > > > > > > > > We need runtime PM enabled early in probe before sdhci_setup_host() for > > > > > sdhci_omap_set_capabilities(). But on the first runtime resume we must > > > > > not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been > > > > > called yet. Let's check for an initialized controller like we already do > > > > > for context restore to fix a lockdep warning. > > > > > > > > Thanks for explaining the background to the problem. However, looking > > > > a bit closer I am worried that the error path in ->probe() is broken > > > > too. > > > > > > > > It seems in the error path, at the label "err_rpm_put", there is a > > > > call to pm_runtime_put_autosuspend(). This doesn't really guarantee > > > > that the ->runtime_suspend() callback will be invoked, which I guess > > > > is the assumption, don't you think? > > > > > > OK I'll check and send a separate patch for that. > > > > > > > That said, I wonder if it would not be easier to convert the ->probe() > > > > function to make use of pm_runtime_get_noresume() and > > > > pm_runtime_set_active() instead. In this way the ->probe() function > > > > becomes responsible of turning on/off the resources "manually" that it > > > > requires to probe (and when it fails to probe), while we can keep the > > > > ->runtime_suspend|resume() callbacks simpler. > > > > > > > > Did that make sense to you? > > > > > > Simpler would be better :) We need to call pm_runtime_get_sync() at some > > > point though to enable the parent device hierarchy. > > > > Is there a parent device that has runtime PM enabled? > > Yes there is the interconnect target module device as the parent with > runtime PM enabled. So the sdhci-omap driver needs the parent enabled. I see, thanks for clarifying! > > > In other cases, it should be fine to use pm_runtime_set_active() > > during ->probe(). > > Yup, this can't be done here though AFAIK. Something needs to enable > runtime PM for the parent device to have the sdhci registers accessible. > > > > Just calling the > > > sdhci_omap runtime functions is not enough. And we still need to check > > > for the valid context too. Also I'm not convinced that calling > > > pm_runtime_get_sync() on the parent device would do the right thing on > > > old omap3 devices without bigger changes.. > > > > I certainly agree. The parent should not be managed directly by the > > sdhci driver. > > OK > > > One thing that can be discussed though, is whether > > pm_runtime_set_active() actually should runtime resume the parent, > > which would make the behaviour consistent with how suppliers are being > > treated. > > Hmm yeah that's an interesting idea. > > > > But maybe you have some better > > > ideas that I'm not considering. > > > > I can try to draft a patch, if that would help? But, let's finalize > > the discussion above first (apologize for the delay). > > OK. Should we apply the $subject patch to fix the splat meanwhile > though? Seems like what you're suggesting may take some more discussion > on the mailing lists. Yep, that seems like a reasonable way forward. I keep you cced if/when I propose something that can replace it. So I applied the v2 for fixes and by adding a stable tag, thanks! Kind regards Uffe
diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c --- a/drivers/mmc/host/sdhci-omap.c +++ b/drivers/mmc/host/sdhci-omap.c @@ -1298,8 +1298,9 @@ static int sdhci_omap_probe(struct platform_device *pdev) /* * omap_device_pm_domain has callbacks to enable the main * functional clock, interface clock and also configure the - * SYSCONFIG register of omap devices. The callback will be invoked - * as part of pm_runtime_get_sync. + * SYSCONFIG register to clear any boot loader set voltage + * capabilities before calling sdhci_setup_host(). The + * callback will be invoked as part of pm_runtime_get_sync. */ pm_runtime_use_autosuspend(dev); pm_runtime_set_autosuspend_delay(dev, 50); @@ -1441,7 +1442,8 @@ static int __maybe_unused sdhci_omap_runtime_suspend(struct device *dev) struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host); - sdhci_runtime_suspend_host(host); + if (omap_host->con != -EINVAL) + sdhci_runtime_suspend_host(host); sdhci_omap_context_save(omap_host); @@ -1458,10 +1460,10 @@ static int __maybe_unused sdhci_omap_runtime_resume(struct device *dev) pinctrl_pm_select_default_state(dev); - if (omap_host->con != -EINVAL) + if (omap_host->con != -EINVAL) { sdhci_omap_context_restore(omap_host); - - sdhci_runtime_resume_host(host, 0); + sdhci_runtime_resume_host(host, 0); + } return 0; }
We need runtime PM enabled early in probe before sdhci_setup_host() for sdhci_omap_set_capabilities(). But on the first runtime resume we must not call sdhci_runtime_resume_host() as sdhci_setup_host() has not been called yet. Let's check for an initialized controller like we already do for context restore to fix a lockdep warning. Fixes: f433e8aac6b9 ("mmc: sdhci-omap: Implement PM runtime functions") Reported-by: Yegor Yefremov <yegorslists@googlemail.com> Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Tony Lindgren <tony@atomide.com> --- Changes since v1: - Add comments for why runtime PM is needed before sdhci_setup_host() as suggested by Adrian --- drivers/mmc/host/sdhci-omap.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)