Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Fri, 8 Apr 2022 19:05:16 +0200 | Subject | Re: [PATCH v1] PM: runtime: Avoid device usage count underflows |
| |
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; > > +} > > + > > [...]
| |