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

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

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