Message-ID: <CAPDyKFpc5p3sXZ6LfdVgt8jR5ZbsQExTgeyMNA-PzcWs5A9U0A@mail.gmail.com>
Date: Wed, 29 Jan 2025 12:52:50 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Linux PM <linux-pm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Alan Stern <stern@...land.harvard.edu>, Bjorn Helgaas <helgaas@...nel.org>, 
	Linux PCI <linux-pci@...r.kernel.org>, Johan Hovold <johan@...nel.org>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Kevin Xie <kevin.xie@...rfivetech.com>
Subject: Re: [PATCH v1] PM: sleep: core: Synchronize runtime PM status of
 parents and children

On Tue, 28 Jan 2025 at 20:24, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Commit 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the
> resume phase") overlooked the case in which the parent of a device with
> DPM_FLAG_SMART_SUSPEND set did not use that flag and could be runtime-
> suspended before a transition into a system-wide sleep state.  In that
> case, if the child is resumed during the subsequent transition from
> that state into the working state, its runtime PM status will be set to
> RPM_ACTIVE, but the runtime PM status of the parent will not be updated
> accordingly, even though the parent will be resumed too, because of the
> dev_pm_skip_suspend() check in device_resume_noirq().
>
> Address this problem by tracking the need to set the runtime PM status
> to RPM_ACTIVE during system-wide resume transitions for devices with
> DPM_FLAG_SMART_SUSPEND set and all of the devices depended on by them.
>
> Fixes: 6e176bf8d461 ("PM: sleep: core: Do not skip callbacks in the resume phase")
> Closes: https://lore.kernel.org/linux-pm/Z30p2Etwf3F2AUvD@hovoldconsulting.com/
> Reported-by: Johan Hovold <johan@...nel.org>
> Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  drivers/base/power/main.c |   29 ++++++++++++++++++++---------
>  include/linux/pm.h        |    1 +
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -656,13 +656,15 @@
>          * so change its status accordingly.
>          *
>          * Otherwise, the device is going to be resumed, so set its PM-runtime
> -        * status to "active", but do that only if DPM_FLAG_SMART_SUSPEND is set
> -        * to avoid confusing drivers that don't use it.
> +        * status to "active" unless its power.set_active flag is clear, in
> +        * which case it is not necessary to update its PM-runtime status.
>          */
> -       if (skip_resume)
> +       if (skip_resume) {
>                 pm_runtime_set_suspended(dev);
> -       else if (dev_pm_skip_suspend(dev))
> +       } else if (dev->power.set_active) {
>                 pm_runtime_set_active(dev);
> +               dev->power.set_active = false;
> +       }
>
>         if (dev->pm_domain) {
>                 info = "noirq power domain ";
> @@ -1189,18 +1191,24 @@
>         return PMSG_ON;
>  }
>
> -static void dpm_superior_set_must_resume(struct device *dev)
> +static void dpm_superior_set_must_resume(struct device *dev, bool set_active)
>  {
>         struct device_link *link;
>         int idx;
>
> -       if (dev->parent)
> +       if (dev->parent) {
>                 dev->parent->power.must_resume = true;
> +               if (set_active)
> +                       dev->parent->power.set_active = true;
> +       }
>
>         idx = device_links_read_lock();
>
> -       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
> +       list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node) {
>                 link->supplier->power.must_resume = true;
> +               if (set_active)
> +                       link->supplier->power.set_active = true;

If I understand correctly, the suppliers are already handled when the
pm_runtime_set_active() is called for consumers, so the above should
not be needed.

That said, maybe we instead allow parent/child to work in the similar
way as for consumer/suppliers, when pm_runtime_set_active() is called
for the child. In other words, when pm_runtime_set_active() is called
for a child and the parent is runtime PM enabled, let's runtime resume
it too, as we do for suppliers. Would that work, you think?

> +       }
>
>         device_links_read_unlock(idx);
>  }
> @@ -1278,8 +1286,11 @@
>               dev->power.may_skip_resume))
>                 dev->power.must_resume = true;
>
> -       if (dev->power.must_resume)
> -               dpm_superior_set_must_resume(dev);
> +       if (dev->power.must_resume) {
> +               dev->power.set_active = dev->power.set_active ||
> +                       dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND);
> +               dpm_superior_set_must_resume(dev, dev->power.set_active);
> +       }
>
>  Complete:
>         complete_all(&dev->power.completion);
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -683,6 +683,7 @@
>         bool                    no_pm_callbacks:1;      /* Owned by the PM core */
>         bool                    async_in_progress:1;    /* Owned by the PM core */
>         bool                    must_resume:1;          /* Owned by the PM core */
> +       bool                    set_active:1;           /* Owned by the PM core */
>         bool                    may_skip_resume:1;      /* Set by subsystems */
>  #else
>         bool                    should_wakeup:1;
>
>
>

Kind regards
Uffe

Powered by blists - more mailing lists