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().
Call acpi_pm_wakeup_event() if power button is pressed and hibernation
is in progress. 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);
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;
+ }
+
+ if (button->suspended || event == ACPI_BUTTON_NOTIFY_WAKE)
+ return;
input_report_key(input, keycode, 1);
input_sync(input);
--
2.47.3On 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);
> --
On 11/24/25 11:42 PM, Rafael J. Wysocki wrote:
> 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.
Thanks for mentioning. This will be fixed easily by adding newer
version of acpi_pm_wakeup_event() which takes hard parameter.
>
>> 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.
I'll fix terminologies in entire series.
>
> Second, this reports an already reported wakeup event.
As mentioned above, I'll update to only 1 call to pm_wakeup_dev_event().>
> Next, why KEY_POWER only? Is KEY_SLEEP not expected to wake up?
Yes, this is true. This will be fixed in next version.
>
> And why event == ACPI_BUTTON_NOTIFY_STATUS? Isn't this what
> ACPI_BUTTON_NOTIFY_WAKE is for?
While device is hibernating, at that if power button is pressed,
ACPI_BUTTON_NOTIFY_STATUS event is received.
ACPI_BUTTON_NOTIFY_WAKE is received when we turn on the device from sleeping state
(suspend) or turned off state (hibernated).
>
>> +
>> + if (button->suspended || event == ACPI_BUTTON_NOTIFY_WAKE)
>> + return;
>>
>> input_report_key(input, keycode, 1);
>> input_sync(input);
>> --
--
---
Thanks,
Usama
On Fri, Nov 07, 2025 at 11:44:29PM +0500, Muhammad Usama Anjum 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(). > > Call acpi_pm_wakeup_event() if power button is pressed and hibernation > is in progress. 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> This could be dangerous, as this is not what happens today, are you sure that people aren't just used to pressing the button multiple times until the system is hibernated? If so, that would now break with this change as it's hard to determine what is going on. And why does hibernate take so long? Why not fix that up instead? thanks, greg k-h
Hi Greg, Thank you for the review. On 11/24/25 10:03 PM, Greg Kroah-Hartman wrote: > On Fri, Nov 07, 2025 at 11:44:29PM +0500, Muhammad Usama Anjum 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(). >> >> Call acpi_pm_wakeup_event() if power button is pressed and hibernation >> is in progress. 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> > My thinking is that people don't press power button after triggering hibernation. They will only press power button if they want to cancel the hibernation or resume from hibernation a bit later when hibernation completes. > This could be dangerous, as this is not what happens today, are you sure > that people aren't just used to pressing the button multiple times until > the system is hibernated? If so, that would now break with this change > as it's hard to determine what is going on. Yes, its possible. Previously the device wouldn't cancel hibernation on power button press; while now it'll cancel. So should we put this cancellation under some config option to avoid breaking the default behavior? > > And why does hibernate take so long? Why not fix that up instead? Hibernation is inherently slow: it must freeze devices, copy and compress/encrypt memory, then resume storage devices to write the image to disk. While I've thought about increasing the speed, I've no concrete ideas yet. The main problem is that its sequential in nature. > > thanks, > > greg k-h -- --- Thanks, Usama
On Tue, Nov 25, 2025 at 04:12:54PM +0500, Muhammad Usama Anjum wrote: > Hi Greg, > > Thank you for the review. > > On 11/24/25 10:03 PM, Greg Kroah-Hartman wrote: > > On Fri, Nov 07, 2025 at 11:44:29PM +0500, Muhammad Usama Anjum 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(). > >> > >> Call acpi_pm_wakeup_event() if power button is pressed and hibernation > >> is in progress. 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> > > > My thinking is that people don't press power button after triggering > hibernation. They will only press power button if they want to cancel the > hibernation or resume from hibernation a bit later when hibernation completes. > > This could be dangerous, as this is not what happens today, are you sure > > that people aren't just used to pressing the button multiple times until > > the system is hibernated? If so, that would now break with this change > > as it's hard to determine what is going on. > Yes, its possible. Previously the device wouldn't cancel hibernation on power > button press; while now it'll cancel. > > So should we put this cancellation under some config option to avoid breaking > the default behavior? Do not add another config option, that way lies madness. As proof, what would your distro select for this, in order to preserve old behavior? :) > > And why does hibernate take so long? Why not fix that up instead? > Hibernation is inherently slow: it must freeze devices, copy and > compress/encrypt memory, then resume storage devices to write the image to > disk. > > While I've thought about increasing the speed, I've no concrete ideas yet. > The main problem is that its sequential in nature. Then fix that? thanks, greg k-h
On 11/25/25 4:47 PM, Greg Kroah-Hartman wrote: > On Tue, Nov 25, 2025 at 04:12:54PM +0500, Muhammad Usama Anjum wrote: >> Hi Greg, >> >> Thank you for the review. >> >> On 11/24/25 10:03 PM, Greg Kroah-Hartman wrote: >>> On Fri, Nov 07, 2025 at 11:44:29PM +0500, Muhammad Usama Anjum 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(). >>>> >>>> Call acpi_pm_wakeup_event() if power button is pressed and hibernation >>>> is in progress. 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> >>> >> My thinking is that people don't press power button after triggering >> hibernation. They will only press power button if they want to cancel the >> hibernation or resume from hibernation a bit later when hibernation completes. >>> This could be dangerous, as this is not what happens today, are you sure >>> that people aren't just used to pressing the button multiple times until >>> the system is hibernated? If so, that would now break with this change >>> as it's hard to determine what is going on. >> Yes, its possible. Previously the device wouldn't cancel hibernation on power >> button press; while now it'll cancel. >> >> So should we put this cancellation under some config option to avoid breaking >> the default behavior? > > Do not add another config option, that way lies madness. As proof, what > would your distro select for this, in order to preserve old behavior? :) I think, the new behavior would be desirable by most distros. They don't care about the old behavior. But its just my thinking. What do you think is the way forward? Even if there are users which used to pressing power button during hibernation, will not press it after a few tries if they really want the hibernation to complete. > >>> And why does hibernate take so long? Why not fix that up instead? >> Hibernation is inherently slow: it must freeze devices, copy and >> compress/encrypt memory, then resume storage devices to write the image to >> disk. >> >> While I've thought about increasing the speed, I've no concrete ideas yet. >> The main problem is that its sequential in nature. > > Then fix that? That's in the plan. But who knows when we get time to attempt that. First I need a board/machine with serial console access to view all logs in real time. :) --- Thanks, Usama
On Tue, Nov 25, 2025 at 09:41:22PM +0500, Muhammad Usama Anjum wrote: > >> While I've thought about increasing the speed, I've no concrete ideas yet. > >> The main problem is that its sequential in nature. > > > > Then fix that? > That's in the plan. But who knows when we get time to attempt that. Take the time to fix this properly first, don't paper over the issue by changing user/system interactions that will not be needed in the future when the real problem is resolved. > First I need a board/machine with serial console access to view all logs in real > time. :) usb debug cables might be your solution. good luck! greg k-h
On 11/26/25 12:38 PM, Greg Kroah-Hartman wrote: > On Tue, Nov 25, 2025 at 09:41:22PM +0500, Muhammad Usama Anjum wrote: >>>> While I've thought about increasing the speed, I've no concrete ideas yet. >>>> The main problem is that its sequential in nature. >>> >>> Then fix that? >> That's in the plan. But who knows when we get time to attempt that. > > Take the time to fix this properly first, don't paper over the issue by > changing user/system interactions that will not be needed in the future > when the real problem is resolved. You're absolutely right, and I share the same philosophy. However, I think the hibernation cancellation feature has standalone value regardless of how much we optimize hibernation time. Even if we achieve significant improvements (5-30% or more), there will still be scenarios where users want to abort an in-progress hibernation. I'm focusing on making this series more concise for next revision. > >> First I need a board/machine with serial console access to view all logs in real >> time. :) > > usb debug cables might be your solution. > > good luck! > > greg k-h -- --- Thanks, Usama
>> First I need a board/machine with serial console access to view all logs in real >> time. :) > > usb debug cables might be your solution. > Just a word of warning before you go too far down this path to get a console working from XHCI debug. This is probably a Hal changing a light bulb problem [1]. Last time I tried XHCI debug cables on some modern AMD systems I ran into a problem that the BAR is too big for early_ioremap(). Looking through LKML it's come up a few times in the past too [2] [3]. Link: https://youtu.be/5W4NFcamRhM?si=qOFrCTzvK6-H-4AX [1] Link: https://lore.kernel.org/linux-usb/ZCOq3PUBCtHkwdnw@mail-itl/ [2] Link: https://lore.kernel.org/linux-usb/CAAcb1K_bezseTM8DrOrzVUi_W+nZoE2N0CO4k3AQWPw7=7pyjw@mail.gmail.com/ [3] The other obvious idea is to use netconsole, but you need a PCIe Ethernet controller, but I think you'll have more success in development using KVM as these are generic architectural problems. To help you get started with this I may point out something that was shared to me for another hibernate bug [4]. Link: https://lore.kernel.org/linux-pm/20251105180506.137448-1-safinaskar@gmail.com/ [4] Askar Safin (CC'ed) produced a script that does a very minimal kernel build, sets up a VM with the right sizes of disks/swap/etc. It's trivial to make kernel changes and re-run the script, and you can also attach a debugger to the KVM instance. Maybe you can adapt something like that. You can wrap it with 'time' calls to actually measure performance for any ideas and prove them out too.
On 11/26/25 5:55 PM, Mario Limonciello wrote: >>> First I need a board/machine with serial console access to view all logs in real >>> time. :) >> >> usb debug cables might be your solution. >> > Just a word of warning before you go too far down this path to get a console working from XHCI debug. > > This is probably a Hal changing a light bulb problem [1]. Last time I tried XHCI debug cables on some modern AMD systems I ran into a problem that the BAR is too big for early_ioremap(). Looking through LKML it's come up a few times in the past too [2] [3]. > > Link: https://youtu.be/5W4NFcamRhM?si=qOFrCTzvK6-H-4AX [1] > Link: https://lore.kernel.org/linux-usb/ZCOq3PUBCtHkwdnw@mail-itl/ [2] > Link: https://lore.kernel.org/linux-usb/CAAcb1K_bezseTM8DrOrzVUi_W+nZoE2N0CO4k3AQWPw7=7pyjw@mail.gmail.com/ [3] > > The other obvious idea is to use netconsole, but you need a PCIe Ethernet controller, but I think you'll have more success in development using KVM as these are generic architectural problems. > > To help you get started with this I may point out something that was shared to me for another hibernate bug [4]. > > Link: https://lore.kernel.org/linux-pm/20251105180506.137448-1-safinaskar@gmail.com/ [4] > > Askar Safin (CC'ed) produced a script that does a very minimal kernel build, sets up a VM with the right sizes of disks/swap/etc. It's trivial to make kernel changes and re-run the script, and you can also attach a debugger to the KVM instance. Maybe you can adapt something like that. You can wrap it with 'time' calls to actually measure performance for any ideas and prove them out too. I was just going to try it. Thank you so much for double confirming. I'll test and see. -- --- Thanks, Usama
© 2016 - 2025 Red Hat, Inc.