[PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Tony Lindgren]

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

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

Re: [PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Adrian Hunter]

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

Re: [PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Ulf Hansson]

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

Re: [PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Tony Lindgren]

* 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

Re: [PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Ulf Hansson]

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

Re: [PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Tony Lindgren]

* 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

Re: [PATCH v2 1/1] mmc: sdhci-omap: Fix a lockdep warning for PM runtime init

[Ulf Hansson]

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