linux-kernel - Re: [PATCH v1] PM: sleep: Fix handling devices with direct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGETcx-sydVJcigqin-zO=ANc65kuz5F3X51a8-SnM0X2zKhMw@mail.gmail.com>
Date: Thu, 13 Mar 2025 14:25:29 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc: Linux PM <linux-pm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Alan Stern <stern@...land.harvard.edu>, Ulf Hansson <ulf.hansson@...aro.org>, 
	Johan Hovold <johan@...nel.org>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Jon Hunter <jonathanh@...dia.com>
Subject: Re: [PATCH v1] PM: sleep: Fix handling devices with direct_complete
 set on errors

On Thu, Mar 13, 2025 at 9:00 AM Rafael J. Wysocki <rjw@...ysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...gle.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

Reviewed-by: Saravana Kannan <saravanak@...gle.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;
>                         }
>
>
>
>