linux-kernel - Re: [PATCH v1 5/5] PM: sleep: Make late and noirq suspend of devices more asynchronous

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: <CAJZ5v0hghyWwd_kZXS1+9pD+DkWiP_4MYORryvkhmFoqayQVaw@mail.gmail.com>
Date: Thu, 13 Mar 2025 14:32:52 +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 5/5] PM: sleep: Make late and noirq suspend of devices
 more asynchronous

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 previous changes, make device_suspend_late() and
> > device_suspend_noirq() start the async suspend of the device's parent
> > and suppliers after the device itself has been processed and make
> > dpm_suspend_late() and dpm_noirq_suspend_devices() start processing
> > "async" leaf devices (that is, devices without children or consumers)
> > upfront because they don't need to wait for any other devices.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >  drivers/base/power/main.c |   60 +++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 52 insertions(+), 8 deletions(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1317,6 +1317,8 @@
> >         device_links_read_unlock(idx);
> >  }
> >
> > +static void async_suspend_noirq(void *data, async_cookie_t cookie);
> > +
> >  /**
> >   * device_suspend_noirq - Execute a "noirq suspend" callback for given device.
> >   * @dev: Device to handle.
> > @@ -1396,7 +1398,13 @@
> >  Complete:
> >         complete_all(&dev->power.completion);
> >         TRACE_SUSPEND(error);
> > -       return error;
> > +
> > +       if (error || async_error)
> > +               return error;
> > +
> > +       dpm_async_suspend_superior(dev, async_suspend_noirq);
> > +
> > +       return 0;
> >  }
> >
> >  static void async_suspend_noirq(void *data, async_cookie_t cookie)
> > @@ -1410,6 +1418,7 @@
> >  static int dpm_noirq_suspend_devices(pm_message_t state)
> >  {
> >         ktime_t starttime = ktime_get();
> > +       struct device *dev;
> >         int error = 0;
> >
> >         trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
> > @@ -1419,15 +1428,28 @@
> >
> >         mutex_lock(&dpm_list_mtx);
> >
> > +       /*
> > +        * Start processing "async" leaf devices upfront because they don't need
> > +        * to wait.
> > +        */
> > +       list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
> > +               dpm_clear_async_state(dev);
> > +               if (dpm_leaf_device(dev))
> > +                       dpm_async_fn(dev, async_suspend_noirq);
> > +       }
> > +
> >         while (!list_empty(&dpm_late_early_list)) {
> > -               struct device *dev = to_device(dpm_late_early_list.prev);
> > +               dev = to_device(dpm_late_early_list.prev);
> >
> >                 list_move(&dev->power.entry, &dpm_noirq_list);
>
> This issue is present in the previous patch too, but it's easier for
> me to point it out here. Suspend abort will break now.
>
> For example, say the devices are suspended in the order A -> B -> C ->
> D -> E if everything was sync.
>
> Case 1: Fully sync devices
> If C aborts, only A and B will be in the dpm_noirq_list. When we try
> to undo the suspend, we just resume devices in dpm_noirq_list and that
> just resumes A and B.
>
> Case 2: Only C is sync.
> When C aborts, A, B, D and E could have finished suspending. But only
> A and B will be in the dpm_noirq_list. When we try to undo the
> suspend, we just resume devices in dpm_noirq_list and that just
> resumes A and B. D and E never get resumed.
>
> My fix for this is to move all devices to dpm_noirq_list if a suspend
> aborts and then using the existing
> is_suspended/is_noirq_suspended/is_late_suspended flags to skip over
> devices that haven't been suspended. That works nicely except in
> is_suspended and I tried to fix it in [2]. But you had an issue with
> [2] that I didn't fully understand and I meant to dig deeper and fix.
> But I didn't get around to it as I got swamped with other work.
>
> [2] - https://lore.kernel.org/linux-pm/20241114220921.2529905-2-saravanak@google.com/

I hope that my last message in this thread clarifies this a bit.

Anyway, yes, this is a bug and yes, it can be addressed by just
continuing to move devices to the target list on errors until all of
them get moved and then look at
is_suspended/is_noirq_suspended/is_late_suspended.

I'll post a fix for the direct-complete handling in device_suspend()
and an update of the last three patches in this series with this issue
addressed.