[PATCH v1] PM: sleep: Fix handling devices with direct_complete set on errors

Posted by Rafael J. Wysocki 9 months, 1 week ago

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

When dpm_suspend() fails, some devices with power.direct_complete set
may not have been handled by device_suspend() yet, so runtime PM has
not been disabled for them yet even though power.direct_complete is set.

Since device_resume() expects that runtime PM has been disabled for all
devices with power.direct_complete set, it will attempt to reenable
runtime PM for the devices that have not been processed by device_suspend()
which does not make sense.  Had those devices had runtime PM disabled
before device_suspend() had run, device_resume() would have inadvertently
enable runtime PM for them, but this is not expected to happen because
it would require ->prepare() callbacks to return positive values for
devices with runtime PM disabled, which would be invalid.

In practice, this issue is most likely benign because pm_runtime_enable()
will not allow the "disable depth" counter to underflow, but it causes a
warning message to be printed for each affected device.

To allow device_resume() to distinguish the "direct complete" devices
that have been processed by device_suspend() from those which have not
been handled by it, make device_suspend() set power.is_suspended for
"direct complete" devices.

Next, move the power.is_suspended check in device_resume() before the
power.direct_complete check in it to make it skip the "direct complete"
devices that have not been handled by device_suspend().

This change is based on a preliminary patch from Saravana Kannan.

Fixes: aae4518b3124 ("PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily")
Link: https://lore.kernel.org/linux-pm/20241114220921.2529905-2-saravanak@google.com/
Reported-by: Saravana Kannan <saravanak@google.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -938,6 +938,9 @@
 	if (dev->power.syscore)
 		goto Complete;
 
+	if (!dev->power.is_suspended)
+		goto Complete;
+
 	if (dev->power.direct_complete) {
 		/*
 		 * Allow new children to be added under the device after this
@@ -963,9 +966,6 @@
 	 */
 	dev->power.is_prepared = false;
 
-	if (!dev->power.is_suspended)
-		goto Unlock;
-
 	if (dev->pm_domain) {
 		info = "power domain ";
 		callback = pm_op(&dev->pm_domain->ops, state);
@@ -1005,7 +1005,6 @@
 	error = dpm_run_callback(callback, dev, state, info);
 	dev->power.is_suspended = false;
 
- Unlock:
 	device_unlock(dev);
 	dpm_watchdog_clear(&wd);
 
@@ -1669,6 +1668,7 @@
 			pm_runtime_disable(dev);
 			if (pm_runtime_status_suspended(dev)) {
 				pm_dev_dbg(dev, state, "direct-complete ");
+				dev->power.is_suspended = true;
 				goto Complete;
 			}

Re: [PATCH v1] PM: sleep: Fix handling devices with direct_complete set on errors

Posted by Saravana Kannan 9 months, 1 week ago

On Thu, Mar 13, 2025 at 9:00 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> When dpm_suspend() fails, some devices with power.direct_complete set
> may not have been handled by device_suspend() yet, so runtime PM has
> not been disabled for them yet even though power.direct_complete is set.
>
> Since device_resume() expects that runtime PM has been disabled for all
> devices with power.direct_complete set, it will attempt to reenable
> runtime PM for the devices that have not been processed by device_suspend()
> which does not make sense.  Had those devices had runtime PM disabled
> before device_suspend() had run, device_resume() would have inadvertently
> enable runtime PM for them, but this is not expected to happen because
> it would require ->prepare() callbacks to return positive values for
> devices with runtime PM disabled, which would be invalid.
>
> In practice, this issue is most likely benign because pm_runtime_enable()
> will not allow the "disable depth" counter to underflow, but it causes a
> warning message to be printed for each affected device.
>
> To allow device_resume() to distinguish the "direct complete" devices
> that have been processed by device_suspend() from those which have not
> been handled by it, make device_suspend() set power.is_suspended for
> "direct complete" devices.
>
> Next, move the power.is_suspended check in device_resume() before the
> power.direct_complete check in it to make it skip the "direct complete"
> devices that have not been handled by device_suspend().
>
> This change is based on a preliminary patch from Saravana Kannan.
>
> Fixes: aae4518b3124 ("PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily")
> Link: https://lore.kernel.org/linux-pm/20241114220921.2529905-2-saravanak@google.com/
> Reported-by: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Saravana Kannan <saravanak@google.com>

> ---
>  drivers/base/power/main.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -938,6 +938,9 @@
>         if (dev->power.syscore)
>                 goto Complete;
>
> +       if (!dev->power.is_suspended)
> +               goto Complete;
> +
>         if (dev->power.direct_complete) {
>                 /*
>                  * Allow new children to be added under the device after this
> @@ -963,9 +966,6 @@
>          */
>         dev->power.is_prepared = false;
>
> -       if (!dev->power.is_suspended)
> -               goto Unlock;
> -
>         if (dev->pm_domain) {
>                 info = "power domain ";
>                 callback = pm_op(&dev->pm_domain->ops, state);
> @@ -1005,7 +1005,6 @@
>         error = dpm_run_callback(callback, dev, state, info);
>         dev->power.is_suspended = false;
>
> - Unlock:
>         device_unlock(dev);
>         dpm_watchdog_clear(&wd);
>
> @@ -1669,6 +1668,7 @@
>                         pm_runtime_disable(dev);
>                         if (pm_runtime_status_suspended(dev)) {
>                                 pm_dev_dbg(dev, state, "direct-complete ");
> +                               dev->power.is_suspended = true;
>                                 goto Complete;
>                         }
>
>
>
>