MIME-Version: 1.0
References: <4723000.31r3eYUQgx@kreacher> <Yaphb0hcqTQ3S78n@rowland.harvard.edu>
In-Reply-To: <Yaphb0hcqTQ3S78n@rowland.harvard.edu>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 3 Dec 2021 20:40:59 +0100
Message-ID: <CAJZ5v0gfvrki8CjB=Fytqqnk_8T9kJofjTETAYn_9X+0rWNN3w@mail.gmail.com>
Subject: Re: [PATCH] PM: runtime: Capture device status before disabling
 runtime PM
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        Kevin Hilman <khilman@kernel.org>,
        Maulik Shah <mkshah@codeaurora.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 3, 2021 at 7:27 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Dec 03, 2021 at 05:24:45PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@linaro.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.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.