linux-kernel - Re: [PATCH v2 4/9] PM: Check power.needs_force_resume in pm_runtime_force

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] [day] [month] [year] [list]

Message-ID: <CAJZ5v0h1F-N3-Ue5OqUPoORDtR=qwknX-yDNuqxbogEjidxooQ@mail.gmail.com>
Date: Fri, 27 Jun 2025 13:14:40 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Linux PM <linux-pm@...r.kernel.org>, Ulf Hansson <ulf.hansson@...aro.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux ACPI <linux-acpi@...r.kernel.org>, 
	Linux PCI <linux-pci@...r.kernel.org>, 
	Mika Westerberg <mika.westerberg@...ux.intel.com>, "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v2 4/9] PM: Check power.needs_force_resume in pm_runtime_force_suspend()

On Fri, Jun 27, 2025 at 12:52 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Thu, Jun 26, 2025 at 8:15 PM Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > Add a power.needs_force_resume check to pm_runtime_force_suspend() so
> > it need not rely on the runtime PM status of the device when deciding
> > whether or not to return early.
> >
> > With the new check in place, pm_runtime_force_suspend() will also skip
> > devices with the runtime PM status equal to RPM_ACTIVE if they have
> > power.needs_force_resume set, so it won't need to change the RPM
> > status of the device to RPM_SUSPENDED in addition to setting
> > power.needs_force_resume in the case when pm_runtime_need_not_resume()
> > return false.
> >
> > This allows the runtime PM status update to be removed from
> > pm_runtime_force_resume(), so the runtime PM status remains unchanged
> > between the pm_runtime_force_suspend() and pm_runtime_force_resume()
> > calls.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >
> > v1 -> v2: Corresponds to patch [2/9] (that was posted as [0/9] by mistake) in v1.
> >
> > ---
> >  drivers/base/power/runtime.c |   21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1973,7 +1973,7 @@
> >         int ret;
> >
> >         pm_runtime_disable(dev);
> > -       if (pm_runtime_status_suspended(dev))
> > +       if (pm_runtime_status_suspended(dev) || dev->power.needs_force_resume)
> >                 return 0;
> >
> >         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> > @@ -1988,15 +1988,16 @@
> >         /*
> >          * If the device can stay in suspend after the system-wide transition
> >          * to the working state that will follow, drop the children counter of
> > -        * its parent, but set its status to RPM_SUSPENDED anyway in case this
> > -        * function will be called again for it in the meantime.
> > +        * its parent and the usage counters of its suppliers.  Otherwise, set
> > +        * power.needs_force_resume to let pm_runtime_force_resume() know that
> > +        * the device needs to be taken care of and to prevent this function
> > +        * from handling the device again in case the device is passed to it
> > +        * once more subsequently.
> >          */
> > -       if (pm_runtime_need_not_resume(dev)) {
> > +       if (pm_runtime_need_not_resume(dev))
> >                 pm_runtime_set_suspended(dev);
> > -       } else {
> > -               __update_runtime_status(dev, RPM_SUSPENDED);
> > +       else
> >                 dev->power.needs_force_resume = true;
> > -       }
>
> I kind of see that this change may confuse other things looking at the
> PM runtime status to determine whether or not the device needs to be
> suspended that possibly run after pm_runtime_force_suspend().
>
> I'm also not quite sure why I thought that this patch would be
> necessary in this series because the [5/9] should just work without
> it.
>
> Please disregard it unless you see why it is needed here.

Well, not quite.

It is needed, but not at this point.  That is,patch [5/9] will work
without it, but then it is needed for the PCI and ACPI PM to work with
pm_runtime_force_suspend().

Namely, say DPM_FLAG_SMART_SUSPEND is set and
pm_runtime_force_suspend() is used as a driver callback in the
"suspend" phase.  The PCI PM sets strict_midlayer, so
pm_runtime_force_suspend() runs the driver runtime PM callback
directly, but power still needs to be removed from the device.
However, pci_pm_suspend_noirq() checks dev_pm_skip_suspend() and it
will bail out if the device is RPM_SUSPENDED.

I guess I should reorder the patches and add the above bit of
explanation to the changelog of the $subject one.

If you can review patches [2-3/9] from this series, I'll be able to go
ahead with the first half of it and the rest can be resent separately.

Sorry for the confusion!

> >
> >         return 0;
> >
> > @@ -2029,12 +2030,6 @@
> >         if (!dev->power.needs_force_resume)
> >                 goto out;
> >
> > -       /*
> > -        * The value of the parent's children counter is correct already, so
> > -        * just update the status of the device.
> > -        */
> > -       __update_runtime_status(dev, RPM_ACTIVE);
> > -
> >         callback = RPM_GET_CALLBACK(dev, runtime_resume);
> >
> >         dev_pm_disable_wake_irq_check(dev, false);
> >
> >
> >
> >