Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1836398.tdWV9SEqCh@kreacher> <CAPDyKFoop_mtWV0i+fH8GSmaxbPCyCPmOeNi6GuK5Qrj7ZVo1g@mail.gmail.com>
In-Reply-To: <CAPDyKFoop_mtWV0i+fH8GSmaxbPCyCPmOeNi6GuK5Qrj7ZVo1g@mail.gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 8 Apr 2022 19:05:16 +0200
Message-ID: <CAJZ5v0geG77Npdy_Ae7XnpWukPPSJ1nE0DFF-vbJViKj6GtCWA@mail.gmail.com>
Subject: Re: [PATCH v1] PM: runtime: Avoid device usage count underflows
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Alan Stern <stern@rowland.harvard.edu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 8, 2022 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 6 Apr 2022 at 21:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >  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).

I thought that it would be better to do the extra access in the
failing case only.

> > +       dev_warn(dev, "Runtime PM usage count underflow!\n");
> > +       return -EINVAL;
> > +}
> > +
>
> [...]