linux-kernel - Re: [PATCH] PM: runtime: Capture device status before disabling runtime PM

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: <CAJZ5v0gfvrki8CjB=Fytqqnk_8T9kJofjTETAYn_9X+0rWNN3w@mail.gmail.com>
Date:   Fri, 3 Dec 2021 20:40:59 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux PM <linux-pm@...r.kernel.org>,
        Kevin Hilman <khilman@...nel.org>,
        Maulik Shah <mkshah@...eaurora.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PM: runtime: Capture device status before disabling
 runtime PM

On Fri, Dec 3, 2021 at 7:27 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> On Fri, Dec 03, 2021 at 05:24:45PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > In some cases (for example, during system-wide suspend and resume of
> > devices) it is useful to know whether or not runtime PM has ever been
> > enabled for a given device and, if so, what the runtime PM status of
> > it had been right before runtime PM was disabled for it last time.
> >
> > For this reason, introduce a new struct dev_pm_info field called
> > last_status that will be used for capturing the runtime PM status of
> > the device when its power.disable_depth counter changes from 0 to 1.
> >
> > The new field will be set to RPM_INVALID to start with and whenever
> > power.disable_depth changes from 1 to 0, so it will be valid only
> > when runtime PM of the device is currently disabled, but it has been
> > enabled at least once.
> >
> > Immediately use power.last_status in rpm_resume() to make it handle
> > the case when PM runtime is disabled for the device, but its runtime
> > PM status is RPM_ACTIVE more consistently.  Namely, make it return 1
> > if power.last_status is also equal to RPM_ACTIVE in that case (the
> > idea being that if the status was RPM_ACTIVE last time when
> > power.disable_depth was changing from 0 to 1 and it is still
> > RPM_ACTIVE, it can be assumed to reflect what happened to the device
> > last time when it was using runtime PM) and -EACCES otherwise.
> >
> > Update the documentation to provide a description of last_status and
> > change the description of pm_runtime_resume() in it to reflect the
> > new behavior of rpm_active().
> >
> > While at it, rearrange the code in pm_runtime_enable() to be more
> > straightforward and replace the WARN() macro in it with a pr_warn()
> > invocation which is less disruptive.
> >
> > Link: https://lore.kernel.org/linux-pm/20211026222626.39222-1-ulf.hansson@linaro.org/t/#u
> > Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >  Documentation/power/runtime_pm.rst |   14 +++++++++----
> >  drivers/base/power/runtime.c       |   38 +++++++++++++++++++------------------
> >  include/linux/pm.h                 |    2 +
> >  3 files changed, 32 insertions(+), 22 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -744,11 +744,10 @@ static int rpm_resume(struct device *dev
> >   repeat:
> >       if (dev->power.runtime_error)
> >               retval = -EINVAL;
> > -     else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > -         && dev->power.runtime_status == RPM_ACTIVE)
> > -             retval = 1;
> >       else if (dev->power.disable_depth > 0)
> > -             retval = -EACCES;
> > +             retval = dev->power.runtime_status == RPM_ACTIVE &&
> > +                      dev->power.last_status == RPM_ACTIVE ? 1 : -EACCES;
>
> Suggestion for a small improvement in readability: The way this
> statement is broken between two lines, it looks as if the ?: operator
> has higher precedence than the && operator, which is very confusing.
> Adding parentheses would help.  Even better would be to rewrite this as
> an "if" statement:
>
>                 if (dev->power.runtime_status == RPM_ACTIVE &&
>                     dev->power.last_status == RPM_ACTIVE)
>                         retval = 1;
>                 else
>                         retval = -EACCES;

I would need to add braces around this then as per the coding style,
which I wanted to avoid, but of course that can be done.