Re: [PATCH 2/4] ACPI: button: Cancel hibernation if button is pressed during hibernation

["Rafael J. Wysocki" <rafael@kernel.org>]

On Fri, Nov 7, 2025 at 7:45 PM Muhammad Usama Anjum
<usama.anjum@collabora.com> wrote:
>
> acpi_pm_wakeup_event() is called from acpi_button_notify() which is
> called when power button is pressed. The system is worken up from s2idle
> in this case by setting hard parameter to pm_wakeup_dev_event().

Right, so presumably you want to set it for hibernation too.

> Call acpi_pm_wakeup_event() if power button is pressed and hibernation
> is in progress.

Well, this is not what the code does after the change.

> Set the hard parameter such that pm_system_wakeup()
> gets called which increments pm_abort_suspend counter. The explicit call
> to acpi_pm_wakeup_event() is necessary as ACPI button device has the
> wakeup source. Hence call to input_report_key() with input device
> doesn't call pm_system_wakeup() as it doesn't have wakeup source
> registered.
>
> Hence hibernation would be cancelled as in hibernation path, this counter
> is checked if it should be aborted.
>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> Changes since RFC:
> - Use pm_sleep_transition_in_progress()
> - Update descriptin why explicit call to acpi_pm_wakeup_event() is
>   necessary
> ---
>  drivers/acpi/button.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 3c6dd9b4ba0ad..e4be5c763edaf 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -20,6 +20,7 @@
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
>  #include <acpi/button.h>
> +#include <linux/suspend.h>
>
>  #define ACPI_BUTTON_CLASS              "button"
>  #define ACPI_BUTTON_FILE_STATE         "state"
> @@ -458,11 +459,16 @@ static void acpi_button_notify(acpi_handle handle, u32 event, void *data)
>         acpi_pm_wakeup_event(&device->dev);

The above is what you want to change, as this already reports the
event.  Reporting it twice is unnecessary and potentially confusing.

>         button = acpi_driver_data(device);
> -       if (button->suspended || event == ACPI_BUTTON_NOTIFY_WAKE)
> -               return;
> -
>         input = button->input;
>         keycode = test_bit(KEY_SLEEP, input->keybit) ? KEY_SLEEP : KEY_POWER;
> +       if (event == ACPI_BUTTON_NOTIFY_STATUS && keycode == KEY_POWER &&
> +           pm_sleep_transition_in_progress()) {
> +               pm_wakeup_dev_event(&device->dev, 0, true);
> +               return;
> +       }

First, this will affect suspend too.

Second, this reports an already reported wakeup event.

Next, why KEY_POWER only?  Is KEY_SLEEP not expected to wake up?

And why event == ACPI_BUTTON_NOTIFY_STATUS?  Isn't this what
ACPI_BUTTON_NOTIFY_WAKE is for?

> +
> +       if (button->suspended || event == ACPI_BUTTON_NOTIFY_WAKE)
> +               return;
>
>         input_report_key(input, keycode, 1);
>         input_sync(input);
> --