[PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

For all practical purposes, there is no difference between the situation
in which a given device is not ignoring children and its active child
count is nonzero and the situation in which its runtime PM usage counter
is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
device's usage counter and return 1 in the latter case.

For consistency, make it do so in the former case either by adjusting
pm_runtime_get_conditional() and update the related kerneldoc comments
accordingly.

Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1203,10 +1203,12 @@
  *
  * Return -EINVAL if runtime PM is disabled for @dev.
  *
- * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
- * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
- * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
- * without changing the usage counter.
+ * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
+ * is set, or (2) @dev is not ignoring children and its active child count is
+ * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
+ * the usage counter of @dev and return 1.
+ *
+ * Otherwise, return 0 without changing the usage counter.
  *
  * If @ign_usage_count is %true, this function can be used to prevent suspending
  * the device when its runtime PM status is %RPM_ACTIVE.
@@ -1228,7 +1230,8 @@
 		retval = -EINVAL;
 	} else if (dev->power.runtime_status != RPM_ACTIVE) {
 		retval = 0;
-	} else if (ign_usage_count) {
+	} else if (ign_usage_count || (!dev->power.ignore_children &&
+		   atomic_read(&dev->power.child_count) > 0)) {
 		retval = 1;
 		atomic_inc(&dev->power.usage_count);
 	} else {
@@ -1261,10 +1264,16 @@
  * @dev: Target device.
  *
  * Increment the runtime PM usage counter of @dev if its runtime PM status is
- * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
- * it returns 1. If the device is in a different state or its usage_count is 0,
- * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
- * in which case also the usage_count will remain unmodified.
+ * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
+ * ignoring children and its active child count is nonzero.  1 is returned in
+ * this case.
+ *
+ * If @dev is in a different state or it is not in use (that is, its usage
+ * counter is 0, or it is ignoring children, or its active child count is 0),
+ * 0 is returned.
+ *
+ * -EINVAL is returned if runtime PM is disabled for the device, in which case
+ * also the usage counter of @dev is not updated.
  */
 int pm_runtime_get_if_in_use(struct device *dev)
 {

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Ulf Hansson]

On Wed, 9 Jul 2025 at 12:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> For all practical purposes, there is no difference between the situation
> in which a given device is not ignoring children and its active child
> count is nonzero and the situation in which its runtime PM usage counter
> is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> device's usage counter and return 1 in the latter case.
>
> For consistency, make it do so in the former case either by adjusting
> pm_runtime_get_conditional() and update the related kerneldoc comments
> accordingly.
>
> Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1203,10 +1203,12 @@
>   *
>   * Return -EINVAL if runtime PM is disabled for @dev.
>   *
> - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> - * without changing the usage counter.
> + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> + * is set, or (2) @dev is not ignoring children and its active child count is
> + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> + * the usage counter of @dev and return 1.
> + *
> + * Otherwise, return 0 without changing the usage counter.
>   *
>   * If @ign_usage_count is %true, this function can be used to prevent suspending
>   * the device when its runtime PM status is %RPM_ACTIVE.
> @@ -1228,7 +1230,8 @@
>                 retval = -EINVAL;
>         } else if (dev->power.runtime_status != RPM_ACTIVE) {
>                 retval = 0;
> -       } else if (ign_usage_count) {
> +       } else if (ign_usage_count || (!dev->power.ignore_children &&
> +                  atomic_read(&dev->power.child_count) > 0)) {

I am not sure I understand why this is needed, sorry.

If someone and somehow we have "dev->power.runtime_status ==
RPM_ACTIVE", then the dev's parents/childrens and suppliers/consumers
should have been reference counted correctly already. Otherwise it
should not have been possible to set the runtime_status to RPM_ACTIVE
in the first place, right?

>                 retval = 1;
>                 atomic_inc(&dev->power.usage_count);
>         } else {
> @@ -1261,10 +1264,16 @@
>   * @dev: Target device.
>   *
>   * Increment the runtime PM usage counter of @dev if its runtime PM status is
> - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> - * it returns 1. If the device is in a different state or its usage_count is 0,
> - * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
> - * in which case also the usage_count will remain unmodified.
> + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
> + * ignoring children and its active child count is nonzero.  1 is returned in
> + * this case.
> + *
> + * If @dev is in a different state or it is not in use (that is, its usage
> + * counter is 0, or it is ignoring children, or its active child count is 0),
> + * 0 is returned.
> + *
> + * -EINVAL is returned if runtime PM is disabled for the device, in which case
> + * also the usage counter of @dev is not updated.
>   */
>  int pm_runtime_get_if_in_use(struct device *dev)
>  {
>
>
>

Kind regards
Uffe

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Rafael J. Wysocki]

On Wed, Jul 9, 2025 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 9 Jul 2025 at 12:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > For all practical purposes, there is no difference between the situation
> > in which a given device is not ignoring children and its active child
> > count is nonzero and the situation in which its runtime PM usage counter
> > is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> > device's usage counter and return 1 in the latter case.
> >
> > For consistency, make it do so in the former case either by adjusting
> > pm_runtime_get_conditional() and update the related kerneldoc comments
> > accordingly.
> >
> > Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> >
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1203,10 +1203,12 @@
> >   *
> >   * Return -EINVAL if runtime PM is disabled for @dev.
> >   *
> > - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> > - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> > - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> > - * without changing the usage counter.
> > + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> > + * is set, or (2) @dev is not ignoring children and its active child count is
> > + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> > + * the usage counter of @dev and return 1.
> > + *
> > + * Otherwise, return 0 without changing the usage counter.
> >   *
> >   * If @ign_usage_count is %true, this function can be used to prevent suspending
> >   * the device when its runtime PM status is %RPM_ACTIVE.
> > @@ -1228,7 +1230,8 @@
> >                 retval = -EINVAL;
> >         } else if (dev->power.runtime_status != RPM_ACTIVE) {
> >                 retval = 0;
> > -       } else if (ign_usage_count) {
> > +       } else if (ign_usage_count || (!dev->power.ignore_children &&
> > +                  atomic_read(&dev->power.child_count) > 0)) {
>
> I am not sure I understand why this is needed, sorry.
>
> If someone and somehow we have "dev->power.runtime_status ==
> RPM_ACTIVE", then the dev's parents/childrens and suppliers/consumers
> should have been reference counted correctly already.

Sure.

> Otherwise it should not have been possible to set the runtime_status to RPM_ACTIVE
> in the first place, right?

Right.

runtime_status must be RPM_ACTIVE, but pm_runtime_get_if_in_use() only
wants to bump it up if the device is in use in addition to that.

So far it's been checking the usage counter only though.

Thanks!

>
> >                 retval = 1;
> >                 atomic_inc(&dev->power.usage_count);
> >         } else {
> > @@ -1261,10 +1264,16 @@
> >   * @dev: Target device.
> >   *
> >   * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> > - * it returns 1. If the device is in a different state or its usage_count is 0,
> > - * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
> > - * in which case also the usage_count will remain unmodified.
> > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
> > + * ignoring children and its active child count is nonzero.  1 is returned in
> > + * this case.
> > + *
> > + * If @dev is in a different state or it is not in use (that is, its usage
> > + * counter is 0, or it is ignoring children, or its active child count is 0),
> > + * 0 is returned.
> > + *
> > + * -EINVAL is returned if runtime PM is disabled for the device, in which case
> > + * also the usage counter of @dev is not updated.
> >   */
> >  int pm_runtime_get_if_in_use(struct device *dev)
> >  {
> >
> >
> >
>
> Kind regards
> Uffe
>

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Rafael J. Wysocki]

On Wed, Jul 9, 2025 at 2:06 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 9, 2025 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 9 Jul 2025 at 12:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > For all practical purposes, there is no difference between the situation
> > > in which a given device is not ignoring children and its active child
> > > count is nonzero and the situation in which its runtime PM usage counter
> > > is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> > > device's usage counter and return 1 in the latter case.
> > >
> > > For consistency, make it do so in the former case either by adjusting
> > > pm_runtime_get_conditional() and update the related kerneldoc comments
> > > accordingly.
> > >
> > > Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > >
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -1203,10 +1203,12 @@
> > >   *
> > >   * Return -EINVAL if runtime PM is disabled for @dev.
> > >   *
> > > - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> > > - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> > > - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> > > - * without changing the usage counter.
> > > + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> > > + * is set, or (2) @dev is not ignoring children and its active child count is
> > > + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> > > + * the usage counter of @dev and return 1.
> > > + *
> > > + * Otherwise, return 0 without changing the usage counter.
> > >   *
> > >   * If @ign_usage_count is %true, this function can be used to prevent suspending
> > >   * the device when its runtime PM status is %RPM_ACTIVE.
> > > @@ -1228,7 +1230,8 @@
> > >                 retval = -EINVAL;
> > >         } else if (dev->power.runtime_status != RPM_ACTIVE) {
> > >                 retval = 0;
> > > -       } else if (ign_usage_count) {
> > > +       } else if (ign_usage_count || (!dev->power.ignore_children &&
> > > +                  atomic_read(&dev->power.child_count) > 0)) {
> >
> > I am not sure I understand why this is needed, sorry.
> >
> > If someone and somehow we have "dev->power.runtime_status ==
> > RPM_ACTIVE", then the dev's parents/childrens and suppliers/consumers
> > should have been reference counted correctly already.
>
> Sure.
>
> > Otherwise it should not have been possible to set the runtime_status to RPM_ACTIVE
> > in the first place, right?
>
> Right.
>
> runtime_status must be RPM_ACTIVE, but pm_runtime_get_if_in_use() only
> wants to bump it up if the device is in use in addition to that.

I mean pm_runtime_get_if_in_use() only wants to bump up the device's
usage counter if it is in use already.

> So far it's been checking the usage counter only though.

And the above is correct.

> >
> > >                 retval = 1;
> > >                 atomic_inc(&dev->power.usage_count);
> > >         } else {
> > > @@ -1261,10 +1264,16 @@
> > >   * @dev: Target device.
> > >   *
> > >   * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > > - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> > > - * it returns 1. If the device is in a different state or its usage_count is 0,
> > > - * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
> > > - * in which case also the usage_count will remain unmodified.
> > > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
> > > + * ignoring children and its active child count is nonzero.  1 is returned in
> > > + * this case.
> > > + *
> > > + * If @dev is in a different state or it is not in use (that is, its usage
> > > + * counter is 0, or it is ignoring children, or its active child count is 0),
> > > + * 0 is returned.
> > > + *
> > > + * -EINVAL is returned if runtime PM is disabled for the device, in which case
> > > + * also the usage counter of @dev is not updated.
> > >   */
> > >  int pm_runtime_get_if_in_use(struct device *dev)
> > >  {
> > >
> > >
> > >
> >
> > Kind regards
> > Uffe
> >

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Ulf Hansson]

On Wed, 9 Jul 2025 at 14:48, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 9, 2025 at 2:06 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Jul 9, 2025 at 1:47 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 9 Jul 2025 at 12:41, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > >
> > > > For all practical purposes, there is no difference between the situation
> > > > in which a given device is not ignoring children and its active child
> > > > count is nonzero and the situation in which its runtime PM usage counter
> > > > is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> > > > device's usage counter and return 1 in the latter case.
> > > >
> > > > For consistency, make it do so in the former case either by adjusting
> > > > pm_runtime_get_conditional() and update the related kerneldoc comments
> > > > accordingly.
> > > >
> > > > Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > ---
> > > >  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
> > > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > >
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -1203,10 +1203,12 @@
> > > >   *
> > > >   * Return -EINVAL if runtime PM is disabled for @dev.
> > > >   *
> > > > - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> > > > - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> > > > - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> > > > - * without changing the usage counter.
> > > > + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> > > > + * is set, or (2) @dev is not ignoring children and its active child count is
> > > > + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> > > > + * the usage counter of @dev and return 1.
> > > > + *
> > > > + * Otherwise, return 0 without changing the usage counter.
> > > >   *
> > > >   * If @ign_usage_count is %true, this function can be used to prevent suspending
> > > >   * the device when its runtime PM status is %RPM_ACTIVE.
> > > > @@ -1228,7 +1230,8 @@
> > > >                 retval = -EINVAL;
> > > >         } else if (dev->power.runtime_status != RPM_ACTIVE) {
> > > >                 retval = 0;
> > > > -       } else if (ign_usage_count) {
> > > > +       } else if (ign_usage_count || (!dev->power.ignore_children &&
> > > > +                  atomic_read(&dev->power.child_count) > 0)) {
> > >
> > > I am not sure I understand why this is needed, sorry.
> > >
> > > If someone and somehow we have "dev->power.runtime_status ==
> > > RPM_ACTIVE", then the dev's parents/childrens and suppliers/consumers
> > > should have been reference counted correctly already.
> >
> > Sure.
> >
> > > Otherwise it should not have been possible to set the runtime_status to RPM_ACTIVE
> > > in the first place, right?
> >
> > Right.
> >
> > runtime_status must be RPM_ACTIVE, but pm_runtime_get_if_in_use() only
> > wants to bump it up if the device is in use in addition to that.
>
> I mean pm_runtime_get_if_in_use() only wants to bump up the device's
> usage counter if it is in use already.
>
> > So far it's been checking the usage counter only though.
>
> And the above is correct.

Aha, I understand your point now.

Comparing how a runtime resume of consumer-device-link works (it bumps
the provider's usage count), your change makes perfect sense as it
aligns the behaviour for child devices.

[...]

That said, please add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Sakari Ailus]

Hi Rafael,

Thanks for the patch.

On Wed, Jul 09, 2025 at 12:41:45PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> For all practical purposes, there is no difference between the situation
> in which a given device is not ignoring children and its active child
> count is nonzero and the situation in which its runtime PM usage counter
> is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> device's usage counter and return 1 in the latter case.
> 
> For consistency, make it do so in the former case either by adjusting
> pm_runtime_get_conditional() and update the related kerneldoc comments
> accordingly.
> 
> Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")

I guess this should be:

Fixes: c111566bea7c ("PM: runtime: Add pm_runtime_get_if_active()")

Should this also be cc'd to stable?

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1203,10 +1203,12 @@
>   *
>   * Return -EINVAL if runtime PM is disabled for @dev.
>   *
> - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> - * without changing the usage counter.
> + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> + * is set, or (2) @dev is not ignoring children and its active child count is
> + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> + * the usage counter of @dev and return 1.
> + *
> + * Otherwise, return 0 without changing the usage counter.
>   *
>   * If @ign_usage_count is %true, this function can be used to prevent suspending
>   * the device when its runtime PM status is %RPM_ACTIVE.
> @@ -1228,7 +1230,8 @@
>  		retval = -EINVAL;
>  	} else if (dev->power.runtime_status != RPM_ACTIVE) {
>  		retval = 0;
> -	} else if (ign_usage_count) {
> +	} else if (ign_usage_count || (!dev->power.ignore_children &&
> +		   atomic_read(&dev->power.child_count) > 0)) {
>  		retval = 1;
>  		atomic_inc(&dev->power.usage_count);
>  	} else {
> @@ -1261,10 +1264,16 @@
>   * @dev: Target device.
>   *
>   * Increment the runtime PM usage counter of @dev if its runtime PM status is
> - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> - * it returns 1. If the device is in a different state or its usage_count is 0,
> - * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
> - * in which case also the usage_count will remain unmodified.
> + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
> + * ignoring children and its active child count is nonzero.  1 is returned in
> + * this case.
> + *
> + * If @dev is in a different state or it is not in use (that is, its usage
> + * counter is 0, or it is ignoring children, or its active child count is 0),
> + * 0 is returned.
> + *
> + * -EINVAL is returned if runtime PM is disabled for the device, in which case
> + * also the usage counter of @dev is not updated.
>   */
>  int pm_runtime_get_if_in_use(struct device *dev)
>  {
> 
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Rafael J. Wysocki]

Hi Sakari,

On Tue, Jul 15, 2025 at 9:28 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rafael,
>
> Thanks for the patch.
>
> On Wed, Jul 09, 2025 at 12:41:45PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > For all practical purposes, there is no difference between the situation
> > in which a given device is not ignoring children and its active child
> > count is nonzero and the situation in which its runtime PM usage counter
> > is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> > device's usage counter and return 1 in the latter case.
> >
> > For consistency, make it do so in the former case either by adjusting
> > pm_runtime_get_conditional() and update the related kerneldoc comments
> > accordingly.
> >
> > Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
>
> I guess this should be:
>
> Fixes: c111566bea7c ("PM: runtime: Add pm_runtime_get_if_active()")

Technically yes, but that would require specific backport changes for
older "stable" series.

> Should this also be cc'd to stable?

Possibly.

> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Thank you!

> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> >
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1203,10 +1203,12 @@
> >   *
> >   * Return -EINVAL if runtime PM is disabled for @dev.
> >   *
> > - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> > - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> > - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> > - * without changing the usage counter.
> > + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> > + * is set, or (2) @dev is not ignoring children and its active child count is
> > + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> > + * the usage counter of @dev and return 1.
> > + *
> > + * Otherwise, return 0 without changing the usage counter.
> >   *
> >   * If @ign_usage_count is %true, this function can be used to prevent suspending
> >   * the device when its runtime PM status is %RPM_ACTIVE.
> > @@ -1228,7 +1230,8 @@
> >               retval = -EINVAL;
> >       } else if (dev->power.runtime_status != RPM_ACTIVE) {
> >               retval = 0;
> > -     } else if (ign_usage_count) {
> > +     } else if (ign_usage_count || (!dev->power.ignore_children &&
> > +                atomic_read(&dev->power.child_count) > 0)) {
> >               retval = 1;
> >               atomic_inc(&dev->power.usage_count);
> >       } else {
> > @@ -1261,10 +1264,16 @@
> >   * @dev: Target device.
> >   *
> >   * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> > - * it returns 1. If the device is in a different state or its usage_count is 0,
> > - * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
> > - * in which case also the usage_count will remain unmodified.
> > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
> > + * ignoring children and its active child count is nonzero.  1 is returned in
> > + * this case.
> > + *
> > + * If @dev is in a different state or it is not in use (that is, its usage
> > + * counter is 0, or it is ignoring children, or its active child count is 0),
> > + * 0 is returned.
> > + *
> > + * -EINVAL is returned if runtime PM is disabled for the device, in which case
> > + * also the usage counter of @dev is not updated.
> >   */
> >  int pm_runtime_get_if_in_use(struct device *dev)
> >  {
> >
> >
>
> --
> Kind regards,
>
> Sakari Ailus
>

Re: [PATCH v1] PM: runtime: Take active children into account in pm_runtime_get_if_in_use()

[Sakari Ailus]

Hi Rafael,

On Tue, Jul 15, 2025 at 02:46:21PM +0200, Rafael J. Wysocki wrote:
> Hi Sakari,
> 
> On Tue, Jul 15, 2025 at 9:28 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Rafael,
> >
> > Thanks for the patch.
> >
> > On Wed, Jul 09, 2025 at 12:41:45PM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > For all practical purposes, there is no difference between the situation
> > > in which a given device is not ignoring children and its active child
> > > count is nonzero and the situation in which its runtime PM usage counter
> > > is nonzero.  However, pm_runtime_get_if_in_use() will only increment the
> > > device's usage counter and return 1 in the latter case.
> > >
> > > For consistency, make it do so in the former case either by adjusting
> > > pm_runtime_get_conditional() and update the related kerneldoc comments
> > > accordingly.
> > >
> > > Fixes: c0ef3df8dbae ("PM: runtime: Simplify pm_runtime_get_if_active() usage")
> >
> > I guess this should be:
> >
> > Fixes: c111566bea7c ("PM: runtime: Add pm_runtime_get_if_active()")
> 
> Technically yes, but that would require specific backport changes for
> older "stable" series.

The lines in pm_runtime_get_conditional() were added by c111566bea7cc, it
looks like the actual fix would be backportable without changes whereas the
documentation would require them. So splitting the patch should allow
backporting the fix only while leaving the documentation as-is. I don't
really have an opinion either way. It's perhaps unlikely the issue causes
actual problems but you never know...

> 
> > Should this also be cc'd to stable?
> 
> Possibly.
> 
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Thank you!
> 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/power/runtime.c |   27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > >
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -1203,10 +1203,12 @@
> > >   *
> > >   * Return -EINVAL if runtime PM is disabled for @dev.
> > >   *
> > > - * Otherwise, if the runtime PM status of @dev is %RPM_ACTIVE and either
> > > - * @ign_usage_count is %true or the runtime PM usage counter of @dev is not
> > > - * zero, increment the usage counter of @dev and return 1. Otherwise, return 0
> > > - * without changing the usage counter.
> > > + * Otherwise, if its runtime PM status is %RPM_ACTIVE and (1) @ign_usage_count
> > > + * is set, or (2) @dev is not ignoring children and its active child count is
> > > + * nonero, or (3) the runtime PM usage counter of @dev is not zero, increment
> > > + * the usage counter of @dev and return 1.
> > > + *
> > > + * Otherwise, return 0 without changing the usage counter.
> > >   *
> > >   * If @ign_usage_count is %true, this function can be used to prevent suspending
> > >   * the device when its runtime PM status is %RPM_ACTIVE.
> > > @@ -1228,7 +1230,8 @@
> > >               retval = -EINVAL;
> > >       } else if (dev->power.runtime_status != RPM_ACTIVE) {
> > >               retval = 0;
> > > -     } else if (ign_usage_count) {
> > > +     } else if (ign_usage_count || (!dev->power.ignore_children &&
> > > +                atomic_read(&dev->power.child_count) > 0)) {
> > >               retval = 1;
> > >               atomic_inc(&dev->power.usage_count);
> > >       } else {
> > > @@ -1261,10 +1264,16 @@
> > >   * @dev: Target device.
> > >   *
> > >   * Increment the runtime PM usage counter of @dev if its runtime PM status is
> > > - * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which case
> > > - * it returns 1. If the device is in a different state or its usage_count is 0,
> > > - * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device,
> > > - * in which case also the usage_count will remain unmodified.
> > > + * %RPM_ACTIVE and its runtime PM usage counter is greater than 0 or it is not
> > > + * ignoring children and its active child count is nonzero.  1 is returned in
> > > + * this case.
> > > + *
> > > + * If @dev is in a different state or it is not in use (that is, its usage
> > > + * counter is 0, or it is ignoring children, or its active child count is 0),
> > > + * 0 is returned.
> > > + *
> > > + * -EINVAL is returned if runtime PM is disabled for the device, in which case
> > > + * also the usage counter of @dev is not updated.
> > >   */
> > >  int pm_runtime_get_if_in_use(struct device *dev)
> > >  {
> > >
> > >
> >

-- 
Kind regards,

Sakari Ailus