linux-kernel - Re: [RFC PATCH] PM: wakeup: set device_set_wakeup

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0iEoTapZP1-QHxe8UDc1oq3y12Tph0-mU=1NhPjNpYLQw@mail.gmail.com>
Date: Wed, 29 Jan 2025 17:18:24 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Dhruva Gole <d-gole@...com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Markus Schneider-Pargmann <msp@...libre.com>, 
	Kevin Hilman <khilman@...libre.com>, Joe Hattori <joe@...is.s.u-tokyo.ac.jp>
Subject: Re: [RFC PATCH] PM: wakeup: set device_set_wakeup_capable to false in
 case of error

On Wed, Jan 29, 2025 at 12:21 PM Dhruva Gole <d-gole@...com> wrote:
>
> In device_init_wakeup enable, we first set device_set_wakeup_capable to
> true. However in case the device_wakeup_enable fails for whatever reason,
> there was no error handling being done.
> Consequenty, there was no cleanup being done to device_set_wakeup_capable.

s/Consequenty/Consequently/

> If a certain API is enabling something, it should take care of disabling it
> in error scenarios. In this case device_init_wakeup should on it's own
> check for errors and clean up accordingly.

Why do you think that failing device_wakeup_enable() will always mean
that the device is not in fact wakeup capable?

> Cc: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>
> Signed-off-by: Dhruva Gole <d-gole@...com>
> ---
>
> This patch was briefly proposed in a related thread [1], where this discussion
> was taking place.
> That probably got missed due to some confusion around the device_init_wakeup
> return value.
>
> There is infact error returning being done in drivers/base/power/wakeup.c, and
> ideally we should be using that info as done in this patch.
>
> If this patch get accepted, it might even bring forth few hidden bugs
> due to missing error handling, and it will also change the patch for
> devm_device_init_wakeup() helper slightly[2].
>
> [1] https://lore.kernel.org/linux-pm/20241218064335.c72gmw56ogtp36a2@lcpd911/
> [2] https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/
>
> ---
>  include/linux/pm_wakeup.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index d501c09c60cd..ed62a7055a54 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -231,9 +231,13 @@ static inline void pm_wakeup_hard_event(struct device *dev)
>   */
>  static inline int device_init_wakeup(struct device *dev, bool enable)
>  {
> +       int err;
>         if (enable) {
>                 device_set_wakeup_capable(dev, true);
> -               return device_wakeup_enable(dev);
> +               err = device_wakeup_enable(dev);
> +               if (err)
> +                       device_set_wakeup_capable(dev, false);
> +               return err;
>         }
>         device_wakeup_disable(dev);
>         device_set_wakeup_capable(dev, false);
> --
> 2.34.1
>