linux-kernel - Re: [PATCH v1] PM: sleep: core: Synchronize runtime PM status of parents and children

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gvQjp_P-5Ww7iN1cGiiMJ6tvLLnPpkTQNk++KhoRe=GA@mail.gmail.com>
Date: Wed, 29 Jan 2025 16:55:03 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, 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 Wed, Jan 29, 2025 at 12:53 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> 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.

It is needed because pm_runtime_set_active() doesn't cause the setting
to propagate to the parent's/suppliers of the suppliers AFAICS.

> 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?

The parent is not runtime-PM enabled when this happens.