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

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

[Rafael J. Wysocki]

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.

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);
+	dev_warn(dev, "Runtime PM usage count underflow!\n");
+	return -EINVAL;
+}
+
 /**
  * __pm_runtime_idle - Entry point for runtime idle operations.
  * @dev: Device to send idle notification for.
  * @rpmflags: Flag bits.
  *
  * If the RPM_GET_PUT flag is set, decrement the device's usage count and
- * return immediately if it is larger than zero.  Then carry out an idle
+ * return immediately if it is larger than zero (if it becomes negative, log a
+ * warning, increment it, and return an error).  Then carry out an idle
  * notification, either synchronous or asynchronous.
  *
  * This routine may be called in atomic context if the RPM_ASYNC flag is set,
@@ -1057,9 +1077,12 @@ int __pm_runtime_idle(struct device *dev
 	int retval;
 
 	if (rpmflags & RPM_GET_PUT) {
-		if (!atomic_dec_and_test(&dev->power.usage_count)) {
+		retval = rpm_drop_usage_count(dev);
+		if (retval > 0) {
 			trace_rpm_usage_rcuidle(dev, rpmflags);
 			return 0;
+		} else if (retval < 0) {
+			return retval;
 		}
 	}
 
@@ -1079,7 +1102,8 @@ EXPORT_SYMBOL_GPL(__pm_runtime_idle);
  * @rpmflags: Flag bits.
  *
  * If the RPM_GET_PUT flag is set, decrement the device's usage count and
- * return immediately if it is larger than zero.  Then carry out a suspend,
+ * return immediately if it is larger than zero (if it becomes negative, log a
+ * warning, increment it, and return an error).  Then carry out a suspend,
  * either synchronous or asynchronous.
  *
  * This routine may be called in atomic context if the RPM_ASYNC flag is set,
@@ -1091,9 +1115,12 @@ int __pm_runtime_suspend(struct device *
 	int retval;
 
 	if (rpmflags & RPM_GET_PUT) {
-		if (!atomic_dec_and_test(&dev->power.usage_count)) {
+		retval = rpm_drop_usage_count(dev);
+		if (retval > 0) {
 			trace_rpm_usage_rcuidle(dev, rpmflags);
 			return 0;
+		} else if (retval < 0) {
+			return retval;
 		}
 	}
 
@@ -1527,14 +1554,17 @@ EXPORT_SYMBOL_GPL(pm_runtime_forbid);
  */
 void pm_runtime_allow(struct device *dev)
 {
+	int ret;
+
 	spin_lock_irq(&dev->power.lock);
 	if (dev->power.runtime_auto)
 		goto out;
 
 	dev->power.runtime_auto = true;
-	if (atomic_dec_and_test(&dev->power.usage_count))
+	ret = rpm_drop_usage_count(dev);
+	if (ret == 0)
 		rpm_idle(dev, RPM_AUTO | RPM_ASYNC);
-	else
+	else if (ret > 0)
 		trace_rpm_usage_rcuidle(dev, RPM_AUTO | RPM_ASYNC);
 
  out:

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

[Ulf Hansson]

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?

>
> 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?

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

[...]

Kind regards
Uffe

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

[Rafael J. Wysocki]

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;
> > +}
> > +
>
> [...]

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

[Ulf Hansson]

On Fri, 8 Apr 2022 at 19:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> 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).

atomic_add_unless(&dev->power.usage_count, -1, 0) would return true as
long as the counter value is greater than 0.

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?

>
> 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;
> > > +}
> > > +
> >
> > [...]

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Mon, Apr 11, 2022 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 8 Apr 2022 at 19:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > 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).
>
> 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.

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

[Ulf Hansson]

On Mon, 11 Apr 2022 at 13:29, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Apr 11, 2022 at 12:36 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 8 Apr 2022 at 19:05, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > 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).
> >
> > 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@linaro.org>

Kind regards
Uffe

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

[Andy Shevchenko]

On Wed, Apr 6, 2022 at 11:49 PM 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.
>
> 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().

...

> +               retval = rpm_drop_usage_count(dev);
> +               if (retval > 0) {
>                         trace_rpm_usage_rcuidle(dev, rpmflags);
>                         return 0;
> +               } else if (retval < 0) {
> +                       return retval;
>                 }

Can be written in a form

               if (retval < 0)
                       return retval;
               if (retval > 0) {
                       trace_rpm_usage_rcuidle(dev, rpmflags);
                       return 0;
               }

...

> +               if (retval > 0) {
>                         trace_rpm_usage_rcuidle(dev, rpmflags);
>                         return 0;
> +               } else if (retval < 0) {
> +                       return retval;
>                 }

Ditto.

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

On Mon, Apr 11, 2022 at 5:09 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Apr 6, 2022 at 11:49 PM 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.
> >
> > 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().
>
> ...
>
> > +               retval = rpm_drop_usage_count(dev);
> > +               if (retval > 0) {
> >                         trace_rpm_usage_rcuidle(dev, rpmflags);
> >                         return 0;
> > +               } else if (retval < 0) {
> > +                       return retval;
> >                 }
>
> Can be written in a form
>
>                if (retval < 0)
>                        return retval;
>                if (retval > 0) {
>                        trace_rpm_usage_rcuidle(dev, rpmflags);
>                        return 0;
>                }
>

I know.

And why would it be better?

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

[Andy Shevchenko]

On Mon, Apr 11, 2022 at 6:17 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Apr 11, 2022 at 5:09 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Apr 6, 2022 at 11:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

...

> > > +               retval = rpm_drop_usage_count(dev);
> > > +               if (retval > 0) {
> > >                         trace_rpm_usage_rcuidle(dev, rpmflags);
> > >                         return 0;
> > > +               } else if (retval < 0) {
> > > +                       return retval;
> > >                 }
> >
> > Can be written in a form
> >
> >                if (retval < 0)
> >                        return retval;
> >                if (retval > 0) {
> >                        trace_rpm_usage_rcuidle(dev, rpmflags);
> >                        return 0;
> >                }
> >
>
> I know.
>
> And why would it be better?

Depends on the perception:
a) less characters to parse (no 'else');
b) checking for errors first, which seems more or less standard pattern.

-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

On Mon, Apr 11, 2022 at 6:53 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Apr 11, 2022 at 6:17 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Mon, Apr 11, 2022 at 5:09 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Wed, Apr 6, 2022 at 11:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> ...
>
> > > > +               retval = rpm_drop_usage_count(dev);
> > > > +               if (retval > 0) {
> > > >                         trace_rpm_usage_rcuidle(dev, rpmflags);
> > > >                         return 0;
> > > > +               } else if (retval < 0) {
> > > > +                       return retval;
> > > >                 }
> > >
> > > Can be written in a form
> > >
> > >                if (retval < 0)
> > >                        return retval;
> > >                if (retval > 0) {
> > >                        trace_rpm_usage_rcuidle(dev, rpmflags);
> > >                        return 0;
> > >                }
> > >
> >
> > I know.
> >
> > And why would it be better?
>
> Depends on the perception:

Well, exactly.

> a) less characters to parse (no 'else');

But to me, with the "else" it is clear that the conditionals are
related to each other which is not so clear otherwise at first sight.
YMMV

> b) checking for errors first, which seems more or less standard pattern.

So the checks can be reversed no problem, but this is such a minor point ,,,