[PATCH v1 0/9] PM: Check power.needs_force_resume in pm_runtime_force_suspend()

Posted by Rafael J. Wysocki 7 months, 2 weeks ago

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

Add a power.needs_force_resume check to pm_runtime_force_suspend() so
it need not rely on the runtime PM status of the device when deciding
whether or not to return early.

With the new check in place, pm_runtime_force_suspend() will also skip
devices with the runtime PM status equal to RPM_ACTIVE if they have
power.needs_force_resume set, so it won't need to change the RPM
status of the device to RPM_SUSPENDED in addition to setting
power.needs_force_resume in the case when pm_runtime_need_not_resume()
return false.

This allows the runtime PM status update to be removed from
pm_runtime_force_resume(), so the runtime PM status remains unchanged
between the pm_runtime_force_suspend() and pm_runtime_force_resume()
calls.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1975,7 +1975,7 @@
 	int ret;
 
 	pm_runtime_disable(dev);
-	if (pm_runtime_status_suspended(dev))
+	if (pm_runtime_status_suspended(dev) || dev->power.needs_force_resume)
 		return 0;
 
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
@@ -1990,15 +1990,16 @@
 	/*
 	 * If the device can stay in suspend after the system-wide transition
 	 * to the working state that will follow, drop the children counter of
-	 * its parent, but set its status to RPM_SUSPENDED anyway in case this
-	 * function will be called again for it in the meantime.
+	 * its parent and the usage counters of its suppliers.  Otherwise, set
+	 * power.needs_force_resume to let pm_runtime_force_resume() know that
+	 * the device needs to be taken care of and to prevent this function
+	 * from handling the device again in case the device is passed to it
+	 * once more subsequently.
 	 */
-	if (pm_runtime_need_not_resume(dev)) {
+	if (pm_runtime_need_not_resume(dev))
 		pm_runtime_set_suspended(dev);
-	} else {
-		__update_runtime_status(dev, RPM_SUSPENDED);
+	else
 		dev->power.needs_force_resume = true;
-	}
 
 	return 0;
 
@@ -2029,12 +2030,6 @@
 	if (!dev->power.needs_force_resume)
 		goto out;
 
-	/*
-	 * The value of the parent's children counter is correct already, so
-	 * just update the status of the device.
-	 */
-	__update_runtime_status(dev, RPM_ACTIVE);
-
 	callback = RPM_GET_CALLBACK(dev, runtime_resume);
 
 	dev_pm_disable_wake_irq_check(dev, false);

Re: [PATCH v1 0/9] PM: Check power.needs_force_resume in pm_runtime_force_suspend()

Posted by Ulf Hansson 7 months, 2 weeks ago

On Wed, 25 Jun 2025 at 21:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add a power.needs_force_resume check to pm_runtime_force_suspend() so
> it need not rely on the runtime PM status of the device when deciding
> whether or not to return early.
>
> With the new check in place, pm_runtime_force_suspend() will also skip
> devices with the runtime PM status equal to RPM_ACTIVE if they have
> power.needs_force_resume set, so it won't need to change the RPM
> status of the device to RPM_SUSPENDED in addition to setting
> power.needs_force_resume in the case when pm_runtime_need_not_resume()
> return false.
>
> This allows the runtime PM status update to be removed from
> pm_runtime_force_resume(), so the runtime PM status remains unchanged
> between the pm_runtime_force_suspend() and pm_runtime_force_resume()
> calls.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |   21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1975,7 +1975,7 @@
>         int ret;
>
>         pm_runtime_disable(dev);
> -       if (pm_runtime_status_suspended(dev))
> +       if (pm_runtime_status_suspended(dev) || dev->power.needs_force_resume)
>                 return 0;
>
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> @@ -1990,15 +1990,16 @@
>         /*
>          * If the device can stay in suspend after the system-wide transition
>          * to the working state that will follow, drop the children counter of
> -        * its parent, but set its status to RPM_SUSPENDED anyway in case this
> -        * function will be called again for it in the meantime.
> +        * its parent and the usage counters of its suppliers.  Otherwise, set
> +        * power.needs_force_resume to let pm_runtime_force_resume() know that
> +        * the device needs to be taken care of and to prevent this function
> +        * from handling the device again in case the device is passed to it
> +        * once more subsequently.
>          */
> -       if (pm_runtime_need_not_resume(dev)) {
> +       if (pm_runtime_need_not_resume(dev))
>                 pm_runtime_set_suspended(dev);
> -       } else {
> -               __update_runtime_status(dev, RPM_SUSPENDED);
> +       else
>                 dev->power.needs_force_resume = true;
> -       }
>
>         return 0;
>
> @@ -2029,12 +2030,6 @@
>         if (!dev->power.needs_force_resume)
>                 goto out;
>
> -       /*
> -        * The value of the parent's children counter is correct already, so
> -        * just update the status of the device.
> -        */
> -       __update_runtime_status(dev, RPM_ACTIVE);
> -
>         callback = RPM_GET_CALLBACK(dev, runtime_resume);
>
>         dev_pm_disable_wake_irq_check(dev, false);
>

As I mentioned for patch4, pm_runtime_force_suspend() is being used
from driver's ->remove() callback too.

If such a driver/device gets probed again, we need a fresh start. It
seems like we need to clear the needs_force_resume flag in
pm_runtime_reinit(). In fact, that looks like an existing bug, even
before $subject patch, right?

Kind regards
Uffe

Re: [PATCH v1 0/9] PM: Check power.needs_force_resume in pm_runtime_force_suspend()

Posted by Rafael J. Wysocki 7 months, 2 weeks ago

On Thu, Jun 26, 2025 at 12:10 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 25 Jun 2025 at 21:25, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Add a power.needs_force_resume check to pm_runtime_force_suspend() so
> > it need not rely on the runtime PM status of the device when deciding
> > whether or not to return early.
> >
> > With the new check in place, pm_runtime_force_suspend() will also skip
> > devices with the runtime PM status equal to RPM_ACTIVE if they have
> > power.needs_force_resume set, so it won't need to change the RPM
> > status of the device to RPM_SUSPENDED in addition to setting
> > power.needs_force_resume in the case when pm_runtime_need_not_resume()
> > return false.
> >
> > This allows the runtime PM status update to be removed from
> > pm_runtime_force_resume(), so the runtime PM status remains unchanged
> > between the pm_runtime_force_suspend() and pm_runtime_force_resume()
> > calls.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/runtime.c |   21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> >
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1975,7 +1975,7 @@
> >         int ret;
> >
> >         pm_runtime_disable(dev);
> > -       if (pm_runtime_status_suspended(dev))
> > +       if (pm_runtime_status_suspended(dev) || dev->power.needs_force_resume)
> >                 return 0;
> >
> >         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> > @@ -1990,15 +1990,16 @@
> >         /*
> >          * If the device can stay in suspend after the system-wide transition
> >          * to the working state that will follow, drop the children counter of
> > -        * its parent, but set its status to RPM_SUSPENDED anyway in case this
> > -        * function will be called again for it in the meantime.
> > +        * its parent and the usage counters of its suppliers.  Otherwise, set
> > +        * power.needs_force_resume to let pm_runtime_force_resume() know that
> > +        * the device needs to be taken care of and to prevent this function
> > +        * from handling the device again in case the device is passed to it
> > +        * once more subsequently.
> >          */
> > -       if (pm_runtime_need_not_resume(dev)) {
> > +       if (pm_runtime_need_not_resume(dev))
> >                 pm_runtime_set_suspended(dev);
> > -       } else {
> > -               __update_runtime_status(dev, RPM_SUSPENDED);
> > +       else
> >                 dev->power.needs_force_resume = true;
> > -       }
> >
> >         return 0;
> >
> > @@ -2029,12 +2030,6 @@
> >         if (!dev->power.needs_force_resume)
> >                 goto out;
> >
> > -       /*
> > -        * The value of the parent's children counter is correct already, so
> > -        * just update the status of the device.
> > -        */
> > -       __update_runtime_status(dev, RPM_ACTIVE);
> > -
> >         callback = RPM_GET_CALLBACK(dev, runtime_resume);
> >
> >         dev_pm_disable_wake_irq_check(dev, false);
> >
>
> As I mentioned for patch4, pm_runtime_force_suspend() is being used
> from driver's ->remove() callback too.
>
> If such a driver/device gets probed again, we need a fresh start. It
> seems like we need to clear the needs_force_resume flag in
> pm_runtime_reinit(). In fact, that looks like an existing bug, even
> before $subject patch, right?

If it is used in ->remove(), then yes, it needs to be cleared in
_reinit(), at least in principle.

Re: [PATCH v1 0/9] PM: Check power.needs_force_resume in pm_runtime_force_suspend()

Posted by Rafael J. Wysocki 7 months, 2 weeks ago

This should be [PATCH v1 2/9], but it obviously doesn't apply without
the [1/9], so please regard it as the second one in the series.

Sorry about the confusion.

On Wed, Jun 25, 2025 at 9:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Add a power.needs_force_resume check to pm_runtime_force_suspend() so
> it need not rely on the runtime PM status of the device when deciding
> whether or not to return early.
>
> With the new check in place, pm_runtime_force_suspend() will also skip
> devices with the runtime PM status equal to RPM_ACTIVE if they have
> power.needs_force_resume set, so it won't need to change the RPM
> status of the device to RPM_SUSPENDED in addition to setting
> power.needs_force_resume in the case when pm_runtime_need_not_resume()
> return false.
>
> This allows the runtime PM status update to be removed from
> pm_runtime_force_resume(), so the runtime PM status remains unchanged
> between the pm_runtime_force_suspend() and pm_runtime_force_resume()
> calls.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |   21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1975,7 +1975,7 @@
>         int ret;
>
>         pm_runtime_disable(dev);
> -       if (pm_runtime_status_suspended(dev))
> +       if (pm_runtime_status_suspended(dev) || dev->power.needs_force_resume)
>                 return 0;
>
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> @@ -1990,15 +1990,16 @@
>         /*
>          * If the device can stay in suspend after the system-wide transition
>          * to the working state that will follow, drop the children counter of
> -        * its parent, but set its status to RPM_SUSPENDED anyway in case this
> -        * function will be called again for it in the meantime.
> +        * its parent and the usage counters of its suppliers.  Otherwise, set
> +        * power.needs_force_resume to let pm_runtime_force_resume() know that
> +        * the device needs to be taken care of and to prevent this function
> +        * from handling the device again in case the device is passed to it
> +        * once more subsequently.
>          */
> -       if (pm_runtime_need_not_resume(dev)) {
> +       if (pm_runtime_need_not_resume(dev))
>                 pm_runtime_set_suspended(dev);
> -       } else {
> -               __update_runtime_status(dev, RPM_SUSPENDED);
> +       else
>                 dev->power.needs_force_resume = true;
> -       }
>
>         return 0;
>
> @@ -2029,12 +2030,6 @@
>         if (!dev->power.needs_force_resume)
>                 goto out;
>
> -       /*
> -        * The value of the parent's children counter is correct already, so
> -        * just update the status of the device.
> -        */
> -       __update_runtime_status(dev, RPM_ACTIVE);
> -
>         callback = RPM_GET_CALLBACK(dev, runtime_resume);
>
>         dev_pm_disable_wake_irq_check(dev, false);
>
>
>
>