linux-kernel - Re: [PATCH v1 4/5] PM: sleep: Start suspending parents and suppliers after subordinate suspend

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: <CAJZ5v0iDW0kkaGprMO84oR8rdvjWGeXAupcQ6iAnqqajYDdOAg@mail.gmail.com>
Date: Thu, 13 Mar 2025 14:53:21 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Saravana Kannan <saravanak@...gle.com>
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>, 
	Ulf Hansson <ulf.hansson@...aro.org>, Johan Hovold <johan@...nel.org>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Subject: Re: [PATCH v1 4/5] PM: sleep: Start suspending parents and suppliers
 after subordinate suspend

On Thu, Mar 13, 2025 at 2:47 AM Saravana Kannan <saravanak@...gle.com> wrote:
>
> On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > In analogy with the previous change affecting the resume path,
> > make device_suspend() start the async suspend of the device's parent
> > and suppliers after the device itself has been processed and make
> > dpm_suspend() start processing "async" leaf devices (that is, devices
> > without children or consumers) upfront because they don't need to wait
> > for any other devices.
> >
> > On the Dell XPS13 9360 in my office, this change reduces the total
> > duration of device suspend by approximately 100 ms (over 20%).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >  drivers/base/power/main.c |   73 +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 69 insertions(+), 4 deletions(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1237,6 +1237,49 @@
> >
> >  /*------------------------- Suspend routines -------------------------*/
> >
> > +static bool dpm_leaf_device(struct device *dev)
> > +{
> > +       struct device *child;
> > +
> > +       lockdep_assert_held(&dpm_list_mtx);
> > +
> > +       child = device_find_any_child(dev);
> > +       if (child) {
> > +               put_device(child);
> > +
> > +               return false;
> > +       }
> > +
> > +       /*
> > +        * Since this function is required to run under dpm_list_mtx, the
> > +        * list_empty() below will only return true if the device's list of
> > +        * consumers is actually empty before calling it.
> > +        */
> > +       return list_empty(&dev->links.consumers);
> > +}
> > +
> > +static void dpm_async_suspend_superior(struct device *dev, async_func_t func)
> > +{
> > +       struct device_link *link;
> > +
> > +       mutex_lock(&dpm_list_mtx);
> > +
> > +       /* Start processing the device's parent if it is "async". */
> > +       if (dev->parent)
> > +               dpm_async_unless_in_progress(dev->parent, func);
> > +
> > +       /*
> > +        * Start processing the device's "async" suppliers.
> > +        *
> > +        * The dpm_list_mtx locking is sufficient for this.
> > +        */
>
> Why is dpm_list_mtx sufficient? Is it because you are assuming no
> driver is trying to change the device links during suspend/resume? Or
> is there some other reason?

dpm_list_mtx is acquired in device_link_add(), so no new links can be
added while this code is running, and list_del_rcu() is safe with
respect to list_for_each_entry_rcu() according to its kerneldoc
comment.

Worst case it will start async processing for a device that is going
away which should be handled cleanly.

> That sounds a bit risky. Is it because if
> you do, you'll hit a AB-BA deadlock or at least a lockdep warning?
> Also, if we can use the device links read locks, we won't block the
> other readers -- so, less contention.

Readers are not blocked regardless, writers are.

> > +       list_for_each_entry_rcu(link, &dev->links.consumers, s_node)
> > +               if (READ_ONCE(link->status) != DL_STATE_DORMANT)
> > +                       dpm_async_unless_in_progress(link->consumer, func);

Oh, the above is actually broken.  It should be

list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
        if (READ_ONCE(link->status) != DL_STATE_DORMANT)
                dpm_async_unless_in_progress(link->supplier, func);

shouldn't it?

I need to fix this.

> This will still queue a lot of devices that can't suspend yet.

I'm not sure what you mean by "a lot"?  This is only going to queue
the suppliers of this particular device.  How many of those could be
there?

> Curious, how many devices do you have in the system where you are testing this?

Around 1500 device objects, and the majority of them have parents and
children, but there are only a few device links.

Thanks!