Re: [PATCH v1] PM: runtime: Avoid device usage count underflows

[Ulf Hansson]

On Mon, 11 Apr 2022 at 13:29, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Mon, Apr 11, 2022 at 12:36 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >
> > On Fri, 8 Apr 2022 at 19:05, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Apr 8, 2022 at 4:05 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, 6 Apr 2022 at 21:03, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > > > >
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > >
> > > > > A PM-runtime device usage count underflow is potentially critical,
> > > > > because it may cause a device to be suspended when it is expected to
> > > > > be operational.
> > > >
> > > > I get the point. Although, perhaps we should also state that it's a
> > > > programming problem that we would like to catch and warn about?
> > >
> > > OK, I can add that to the changelog.
> > >
> > > > >
> > > > > For this reason, (1) make rpm_check_suspend_allowed() return an error
> > > > > when the device usage count is negative to prevent devices from being
> > > > > suspended in that case, (2) introduce rpm_drop_usage_count() that will
> > > > > detect device usage count underflows, warn about them and fix them up,
> > > > > and (3) use it to drop the usage count in a few places instead of
> > > > > atomic_dec_and_test().
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/base/power/runtime.c |   44 ++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 37 insertions(+), 7 deletions(-)
> > > > >
> > > > > Index: linux-pm/drivers/base/power/runtime.c
> > > > > ===================================================================
> > > > > --- linux-pm.orig/drivers/base/power/runtime.c
> > > > > +++ linux-pm/drivers/base/power/runtime.c
> > > > > @@ -263,7 +263,7 @@ static int rpm_check_suspend_allowed(str
> > > > >                 retval = -EINVAL;
> > > > >         else if (dev->power.disable_depth > 0)
> > > > >                 retval = -EACCES;
> > > > > -       else if (atomic_read(&dev->power.usage_count) > 0)
> > > > > +       else if (atomic_read(&dev->power.usage_count))
> > > > >                 retval = -EAGAIN;
> > > > >         else if (!dev->power.ignore_children &&
> > > > >                         atomic_read(&dev->power.child_count))
> > > > > @@ -1039,13 +1039,33 @@ int pm_schedule_suspend(struct device *d
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(pm_schedule_suspend);
> > > > >
> > > > > +static int rpm_drop_usage_count(struct device *dev)
> > > > > +{
> > > > > +       int ret;
> > > > > +
> > > > > +       ret = atomic_sub_return(1, &dev->power.usage_count);
> > > > > +       if (ret >= 0)
> > > > > +               return ret;
> > > > > +
> > > > > +       /*
> > > > > +        * Because rpm_resume() does not check the usage counter, it will resume
> > > > > +        * the device even if the usage counter is 0 or negative, so it is
> > > > > +        * sufficient to increment the usage counter here to reverse the change
> > > > > +        * made above.
> > > > > +        */
> > > > > +       atomic_inc(&dev->power.usage_count);
> > > >
> > > > Rather than this two-step process, couldn't we just do an
> > > > "atomic_add_unless(&dev->power.usage_count, -1, 0)" - and check the
> > > > return value?
> > >
> > > No, we couldn't, because atomic_add_unless() returns a bool and we
> > > need to know the new counter value (and in particular whether or not
> > > it is 0).
> >
> > atomic_add_unless(&dev->power.usage_count, -1, 0) would return true as
> > long as the counter value is greater than 0.
>
> Yes, and it in particular, when the current value of the counter is 1
> before the operation IIUC.
>
> So after the operation it is 0 and true will be returned, won't it?
> But that's exactly the case we want to catch.
>
> > If the counter has become 0, atomic_add_unless() would return false
> > and not continue to decrease the value below zero. Isn't this exactly
> > what we want?
>
> Not really.
>
> We want to detect transitions from 0 to 1 which is the case when the
> device can be suspended.

I assume you mean from 1 to 0. In any case, I see what you mean by
now, sorry for the noise.

Then feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

Kind regards
Uffe