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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 22 Jun 2022 at 07:12, Tony Lindgren <tony@xxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Suggested-by: Arnd Bergmann <arnd@xxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>
> 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



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux