[PATCH v3 0/4] Fix soc-button-array debounce

[PATCH v3 0/4] Fix soc-button-array debounce

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

I have some hardware in front of me that uses the soc-button-array
driver but the power button doesn't work.

Digging into it, it's because the ASL prescribes a debounce of 0 for
the power button, but the soc-button-array driver hardcodes 50ms.

Hardcoding it to what the ASL expects the power button works.

I looked at the callpath into the GPIO core and I believe it's
because the debounce value from _CRS is never programmed to the
hardware the way that the GPIO gets setup.

This series add that programming path and then sets the hardcoded
value on on some quirked systems.  Hopefully Hans can confirm this
continues to work on the hardware that he originally developed the
hardcoding for.

---
v3:
 * Use Hans suggestion to force software debounce instead of a quirk
 * Fix a resume issue identified from testing this series

Mario Limonciello (4):
  gpiolib: acpi: Add a helper for programming debounce
  gpiolib: acpi: Program debounce when finding GPIO
  Input: Don't program hw debounce for soc_button_array devices
  Input: Don't send fake button presses to wake system

 drivers/gpio/gpiolib-acpi-core.c      | 25 ++++++++++++++-----------
 drivers/input/keyboard/gpio_keys.c    | 14 ++++++--------
 drivers/input/misc/soc_button_array.c |  1 +
 include/linux/gpio_keys.h             |  2 ++
 4 files changed, 23 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] gpiolib: acpi: Add a helper for programming debounce

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Debounce is programmed in two places and considered non-fatal in one of
them. Introduce a helper for programming debounce and show a warning
when failing to program.  This is a difference in behavior for the call
in acpi_dev_gpio_irq_wake_get_by().

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpio/gpiolib-acpi-core.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index 12b24a717e43f..1895e45bd9f16 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -291,6 +291,17 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio, int polarity)
 	return GPIOD_ASIS;
 }
 
+static void acpi_set_debounce_timeout(struct gpio_desc *desc, unsigned int timeout)
+{
+	int ret;
+
+	/* ACPI uses hundredths of milliseconds units */
+	ret = gpio_set_debounce_timeout(desc, timeout * 10);
+	if (ret)
+		dev_warn(&desc->gdev->dev,
+			 "Failed to set debounce-timeout: %d\n", ret);
+}
+
 static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 						struct acpi_resource_gpio *agpio,
 						unsigned int index,
@@ -300,18 +311,12 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 	enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
 	unsigned int pin = agpio->pin_table[index];
 	struct gpio_desc *desc;
-	int ret;
 
 	desc = gpiochip_request_own_desc(chip, pin, label, polarity, flags);
 	if (IS_ERR(desc))
 		return desc;
 
-	/* ACPI uses hundredths of milliseconds units */
-	ret = gpio_set_debounce_timeout(desc, agpio->debounce_timeout * 10);
-	if (ret)
-		dev_warn(chip->parent,
-			 "Failed to set debounce-timeout for pin 0x%04X, err %d\n",
-			 pin, ret);
+	acpi_set_debounce_timeout(desc, agpio->debounce_timeout);
 
 	return desc;
 }
@@ -1025,10 +1030,7 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
 			if (ret < 0)
 				return ret;
 
-			/* ACPI uses hundredths of milliseconds units */
-			ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
-			if (ret)
-				return ret;
+			acpi_set_debounce_timeout(desc, info.debounce);
 
 			irq_flags = acpi_dev_get_irq_type(info.triggering,
 							  info.polarity);
-- 
2.43.0

Re: [PATCH v3 1/4] gpiolib: acpi: Add a helper for programming debounce

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 04:58:10PM -0500, Mario Limonciello wrote:
> 
> Debounce is programmed in two places and considered non-fatal in one of
> them. Introduce a helper for programming debounce and show a warning
> when failing to program.

> This is a difference in behavior for the call
> in acpi_dev_gpio_irq_wake_get_by().

When I meant "both", I was thinking of the _single_ existing case and new one
which you are about to add. In principle, I think changing behaviour here is
undesired. We provoke BIOS writers to make mistakes with debounce settings in
GpioInt() resources.

I agree on the patch...

> -			/* ACPI uses hundredths of milliseconds units */
> -			ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
> -			if (ret)
> -				return ret;
> +			acpi_set_debounce_timeout(desc, info.debounce);

...except this hunk.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/4] gpiolib: acpi: Add a helper for programming debounce

[Mario Limonciello]

On 6/26/2025 9:29 AM, Andy Shevchenko wrote:
> On Wed, Jun 25, 2025 at 04:58:10PM -0500, Mario Limonciello wrote:
>>
>> Debounce is programmed in two places and considered non-fatal in one of
>> them. Introduce a helper for programming debounce and show a warning
>> when failing to program.
> 
>> This is a difference in behavior for the call
>> in acpi_dev_gpio_irq_wake_get_by().
> 
> When I meant "both", I was thinking of the _single_ existing case and new one
> which you are about to add. In principle, I think changing behaviour here is
> undesired. We provoke BIOS writers to make mistakes with debounce settings in
> GpioInt() resources.
> 
> I agree on the patch...
> 
>> -			/* ACPI uses hundredths of milliseconds units */
>> -			ret = gpio_set_debounce_timeout(desc, info.debounce * 10);
>> -			if (ret)
>> -				return ret;
>> +			acpi_set_debounce_timeout(desc, info.debounce);
> 
> ...except this hunk.
> 

OK in that case I'll just squash patches 1 and 2 together, pick up Hans' 
tag and drop this hunk.

[PATCH v3 2/4] gpiolib: acpi: Program debounce when finding GPIO

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When soc-button-array looks up the GPIO to use it calls acpi_find_gpio()
which will parse _CRS.

acpi_find_gpio.cold (drivers/gpio/gpiolib-acpi-core.c:953)
gpiod_find_and_request (drivers/gpio/gpiolib.c:4598 drivers/gpio/gpiolib.c:4625)
gpiod_get_index (drivers/gpio/gpiolib.c:4877)

The GPIO is setup basically, but the debounce information is discarded.
The platform will assert what debounce should be in _CRS, so program it
at the time it's available.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Make non fatal by using helper from patch 1 (Andy)
---
 drivers/gpio/gpiolib-acpi-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index 1895e45bd9f16..15222bfc25bb2 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -962,6 +962,7 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 
 	acpi_gpio_update_gpiod_flags(dflags, &info);
 	acpi_gpio_update_gpiod_lookup_flags(lookupflags, &info);
+	acpi_set_debounce_timeout(desc, info.debounce);
 	return desc;
 }
 
-- 
2.43.0

Re: [PATCH v3 2/4] gpiolib: acpi: Program debounce when finding GPIO

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 04:58:11PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> When soc-button-array looks up the GPIO to use it calls acpi_find_gpio()
> which will parse _CRS.
> 
> acpi_find_gpio.cold (drivers/gpio/gpiolib-acpi-core.c:953)
> gpiod_find_and_request (drivers/gpio/gpiolib.c:4598 drivers/gpio/gpiolib.c:4625)
> gpiod_get_index (drivers/gpio/gpiolib.c:4877)
> 
> The GPIO is setup basically, but the debounce information is discarded.
> The platform will assert what debounce should be in _CRS, so program it
> at the time it's available.

No objections, just a style nit-pick.

...

>  	acpi_gpio_update_gpiod_flags(dflags, &info);
>  	acpi_gpio_update_gpiod_lookup_flags(lookupflags, &info);

+ blank line

> +	acpi_set_debounce_timeout(desc, info.debounce);

+ blank line (this one I'm not sure, need to check the other function style).

>  	return desc;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/4] Input: Don't program hw debounce for soc_button_array devices

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Programming a hardware debounce of 50ms causes problems where a button
doesn't work properly anymore on some systems.  This debounce is intended
for compatibility with systems with a mechanical switch so soc_button_array
devices should only be using a sofware debounce.

Add support for indicating that a driver is only requesting gpio_keys
to use software debounce support and mark that in soc_button_array
accordingly.

Suggested-by: Hans de Goede <hansg@kernel.org>
Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/input/keyboard/gpio_keys.c    | 7 +++++--
 drivers/input/misc/soc_button_array.c | 1 +
 include/linux/gpio_keys.h             | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index f9db86da0818b..773aa5294d269 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -552,8 +552,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 		bool active_low = gpiod_is_active_low(bdata->gpiod);
 
 		if (button->debounce_interval) {
-			error = gpiod_set_debounce(bdata->gpiod,
-					button->debounce_interval * 1000);
+			if (ddata->pdata->no_hw_debounce)
+				error = -EINVAL;
+			else
+				error = gpiod_set_debounce(bdata->gpiod,
+							   button->debounce_interval * 1000);
 			/* use timer if gpiolib doesn't provide debounce */
 			if (error < 0)
 				bdata->software_debounce =
diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index b8cad415c62ca..dac940455bea8 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -232,6 +232,7 @@ soc_button_device_create(struct platform_device *pdev,
 	gpio_keys_pdata->buttons = gpio_keys;
 	gpio_keys_pdata->nbuttons = n_buttons;
 	gpio_keys_pdata->rep = autorepeat;
+	gpio_keys_pdata->no_hw_debounce = TRUE;
 
 	pd = platform_device_register_resndata(&pdev->dev, "gpio-keys",
 					       PLATFORM_DEVID_AUTO, NULL, 0,
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index 80fa930b04c67..c99f74467fda6 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -48,6 +48,7 @@ struct gpio_keys_button {
  * @enable:		platform hook for enabling the device
  * @disable:		platform hook for disabling the device
  * @name:		input device name
+ * @no_hw_debounce:	avoid programming hardware debounce
  */
 struct gpio_keys_platform_data {
 	const struct gpio_keys_button *buttons;
@@ -57,6 +58,7 @@ struct gpio_keys_platform_data {
 	int (*enable)(struct device *dev);
 	void (*disable)(struct device *dev);
 	const char *name;
+	bool no_hw_debounce;
 };
 
 #endif
-- 
2.43.0

Re: [PATCH v3 3/4] Input: Don't program hw debounce for soc_button_array devices

[Hans de Goede]

Hi,

On 25-Jun-25 23:58, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Programming a hardware debounce of 50ms causes problems where a button
> doesn't work properly anymore on some systems.  This debounce is intended
> for compatibility with systems with a mechanical switch so soc_button_array
> devices should only be using a sofware debounce.
> 
> Add support for indicating that a driver is only requesting gpio_keys
> to use software debounce support and mark that in soc_button_array
> accordingly.
> 
> Suggested-by: Hans de Goede <hansg@kernel.org>
> Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans



> ---
>  drivers/input/keyboard/gpio_keys.c    | 7 +++++--
>  drivers/input/misc/soc_button_array.c | 1 +
>  include/linux/gpio_keys.h             | 2 ++
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index f9db86da0818b..773aa5294d269 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -552,8 +552,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
>  		bool active_low = gpiod_is_active_low(bdata->gpiod);
>  
>  		if (button->debounce_interval) {
> -			error = gpiod_set_debounce(bdata->gpiod,
> -					button->debounce_interval * 1000);
> +			if (ddata->pdata->no_hw_debounce)
> +				error = -EINVAL;
> +			else
> +				error = gpiod_set_debounce(bdata->gpiod,
> +							   button->debounce_interval * 1000);
>  			/* use timer if gpiolib doesn't provide debounce */
>  			if (error < 0)
>  				bdata->software_debounce =
> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
> index b8cad415c62ca..dac940455bea8 100644
> --- a/drivers/input/misc/soc_button_array.c
> +++ b/drivers/input/misc/soc_button_array.c
> @@ -232,6 +232,7 @@ soc_button_device_create(struct platform_device *pdev,
>  	gpio_keys_pdata->buttons = gpio_keys;
>  	gpio_keys_pdata->nbuttons = n_buttons;
>  	gpio_keys_pdata->rep = autorepeat;
> +	gpio_keys_pdata->no_hw_debounce = TRUE;
>  
>  	pd = platform_device_register_resndata(&pdev->dev, "gpio-keys",
>  					       PLATFORM_DEVID_AUTO, NULL, 0,
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> index 80fa930b04c67..c99f74467fda6 100644
> --- a/include/linux/gpio_keys.h
> +++ b/include/linux/gpio_keys.h
> @@ -48,6 +48,7 @@ struct gpio_keys_button {
>   * @enable:		platform hook for enabling the device
>   * @disable:		platform hook for disabling the device
>   * @name:		input device name
> + * @no_hw_debounce:	avoid programming hardware debounce
>   */
>  struct gpio_keys_platform_data {
>  	const struct gpio_keys_button *buttons;
> @@ -57,6 +58,7 @@ struct gpio_keys_platform_data {
>  	int (*enable)(struct device *dev);
>  	void (*disable)(struct device *dev);
>  	const char *name;
> +	bool no_hw_debounce;
>  };
>  
>  #endif

Re: [PATCH v3 3/4] Input: Don't program hw debounce for soc_button_array devices

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 04:58:12PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Programming a hardware debounce of 50ms causes problems where a button
> doesn't work properly anymore on some systems.  This debounce is intended
> for compatibility with systems with a mechanical switch so soc_button_array
> devices should only be using a sofware debounce.
> 
> Add support for indicating that a driver is only requesting gpio_keys
> to use software debounce support and mark that in soc_button_array
> accordingly.

...

> +	gpio_keys_pdata->no_hw_debounce = TRUE;

true

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Sending an input event to wake a system does wake it, but userspace picks
up the keypress and processes it.  This isn't the intended behavior as it
causes a suspended system to wake up and then potentially turn off if
userspace is configured to turn off on power button presses.

Instead send a PM wakeup event for the PM core to handle waking the system.

Cc: Hans de Goede <hansg@kernel.org>
Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/input/keyboard/gpio_keys.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 773aa5294d269..4c6876b099c43 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 		pm_stay_awake(bdata->input->dev.parent);
 		if (bdata->suspended  &&
 		    (button->type == 0 || button->type == EV_KEY)) {
-			/*
-			 * Simulate wakeup key press in case the key has
-			 * already released by the time we got interrupt
-			 * handler to run.
-			 */
-			input_report_key(bdata->input, button->code, 1);
+			pm_wakeup_event(bdata->input->dev.parent, 0);
 		}
 	}
 
-- 
2.43.0

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi Mario,

On 25-Jun-25 23:58, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> Sending an input event to wake a system does wake it, but userspace picks
> up the keypress and processes it.  This isn't the intended behavior as it
> causes a suspended system to wake up and then potentially turn off if
> userspace is configured to turn off on power button presses.
> 
> Instead send a PM wakeup event for the PM core to handle waking the system.
> 
> Cc: Hans de Goede <hansg@kernel.org>
> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/input/keyboard/gpio_keys.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 773aa5294d269..4c6876b099c43 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>  		pm_stay_awake(bdata->input->dev.parent);
>  		if (bdata->suspended  &&
>  		    (button->type == 0 || button->type == EV_KEY)) {
> -			/*
> -			 * Simulate wakeup key press in case the key has
> -			 * already released by the time we got interrupt
> -			 * handler to run.
> -			 */
> -			input_report_key(bdata->input, button->code, 1);
> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>  		}
>  	}
>  

Hmm, we have the same problem on many Bay Trail / Cherry Trail
windows 8 / win10 tablets, so  this has been discussed before and e.g.
Android userspace actually needs the button-press (evdev) event to not
immediately go back to sleep, so a similar patch has been nacked in
the past.

At least for GNOME this has been fixed in userspace by ignoring
power-button events the first few seconds after a resume from suspend.

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/25 3:35 AM, Hans de Goede wrote:
> Hi Mario,
> 
> On 25-Jun-25 23:58, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> Sending an input event to wake a system does wake it, but userspace picks
>> up the keypress and processes it.  This isn't the intended behavior as it
>> causes a suspended system to wake up and then potentially turn off if
>> userspace is configured to turn off on power button presses.
>>
>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>
>> Cc: Hans de Goede <hansg@kernel.org>
>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/input/keyboard/gpio_keys.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>> index 773aa5294d269..4c6876b099c43 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>   		pm_stay_awake(bdata->input->dev.parent);
>>   		if (bdata->suspended  &&
>>   		    (button->type == 0 || button->type == EV_KEY)) {
>> -			/*
>> -			 * Simulate wakeup key press in case the key has
>> -			 * already released by the time we got interrupt
>> -			 * handler to run.
>> -			 */
>> -			input_report_key(bdata->input, button->code, 1);
>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>>   		}
>>   	}
>>   
> 
> Hmm, we have the same problem on many Bay Trail / Cherry Trail
> windows 8 / win10 tablets, so  this has been discussed before and e.g.
> Android userspace actually needs the button-press (evdev) event to not
> immediately go back to sleep, so a similar patch has been nacked in
> the past.
> 
> At least for GNOME this has been fixed in userspace by ignoring
> power-button events the first few seconds after a resume from suspend.
> 

The default behavior for logind is:

HandlePowerKey=poweroff

Can you share more about what version of GNOME has a workaround?
This was actually GNOME (on Ubuntu 24.04) that I found this issue.

Nonetheless if this is dependent on an Android userspace problem could 
we perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?

Most people not using Android would be compiling with that enabled in 
their kernel I'd expect.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

Hi Mario,

On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> 
> 
> On 6/26/25 3:35 AM, Hans de Goede wrote:
> > Hi Mario,
> > 
> > On 25-Jun-25 23:58, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > Sending an input event to wake a system does wake it, but userspace picks
> > > up the keypress and processes it.  This isn't the intended behavior as it
> > > causes a suspended system to wake up and then potentially turn off if
> > > userspace is configured to turn off on power button presses.
> > > 
> > > Instead send a PM wakeup event for the PM core to handle waking the system.
> > > 
> > > Cc: Hans de Goede <hansg@kernel.org>
> > > Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/input/keyboard/gpio_keys.c | 7 +------
> > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > index 773aa5294d269..4c6876b099c43 100644
> > > --- a/drivers/input/keyboard/gpio_keys.c
> > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> > >   		pm_stay_awake(bdata->input->dev.parent);
> > >   		if (bdata->suspended  &&
> > >   		    (button->type == 0 || button->type == EV_KEY)) {
> > > -			/*
> > > -			 * Simulate wakeup key press in case the key has
> > > -			 * already released by the time we got interrupt
> > > -			 * handler to run.
> > > -			 */
> > > -			input_report_key(bdata->input, button->code, 1);
> > > +			pm_wakeup_event(bdata->input->dev.parent, 0);

There is already pm_stay_awake() above.

> > >   		}
> > >   	}
> > 
> > Hmm, we have the same problem on many Bay Trail / Cherry Trail
> > windows 8 / win10 tablets, so  this has been discussed before and e.g.
> > Android userspace actually needs the button-press (evdev) event to not
> > immediately go back to sleep, so a similar patch has been nacked in
> > the past.
> > 
> > At least for GNOME this has been fixed in userspace by ignoring
> > power-button events the first few seconds after a resume from suspend.
> > 
> 
> The default behavior for logind is:
> 
> HandlePowerKey=poweroff
> 
> Can you share more about what version of GNOME has a workaround?
> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> 
> Nonetheless if this is dependent on an Android userspace problem could we
> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?

No it is not only Android, other userspace may want to distinguish
between normal and "dark" resume based on keyboard or other user
activity.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> Hi Mario,
> 
> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>
>>
>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>> Hi Mario,
>>>
>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>> causes a suspended system to wake up and then potentially turn off if
>>>> userspace is configured to turn off on power button presses.
>>>>
>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>
>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>    1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>> index 773aa5294d269..4c6876b099c43 100644
>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>    		pm_stay_awake(bdata->input->dev.parent);
>>>>    		if (bdata->suspended  &&
>>>>    		    (button->type == 0 || button->type == EV_KEY)) {
>>>> -			/*
>>>> -			 * Simulate wakeup key press in case the key has
>>>> -			 * already released by the time we got interrupt
>>>> -			 * handler to run.
>>>> -			 */
>>>> -			input_report_key(bdata->input, button->code, 1);
>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
> 
> There is already pm_stay_awake() above.

But that doesn't help with the fact that userspace gets KEY_POWER from 
this and reacts to it.

> 
>>>>    		}
>>>>    	}
>>>
>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>> Android userspace actually needs the button-press (evdev) event to not
>>> immediately go back to sleep, so a similar patch has been nacked in
>>> the past.
>>>
>>> At least for GNOME this has been fixed in userspace by ignoring
>>> power-button events the first few seconds after a resume from suspend.
>>>
>>
>> The default behavior for logind is:
>>
>> HandlePowerKey=poweroff
>>
>> Can you share more about what version of GNOME has a workaround?
>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>
>> Nonetheless if this is dependent on an Android userspace problem could we
>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> 
> No it is not only Android, other userspace may want to distinguish
> between normal and "dark" resume based on keyboard or other user
> activity.
> 
> Thanks.
> 
In this specific case does the key passed up to satisfy this userspace 
requirement and keep it awake need to specifically be a fabricated 
KEY_POWER?

Or could we find a key that doesn't require some userspace to ignore 
KEY_POWER?

Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> 
> 
> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> > Hi Mario,
> > 
> > On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> > > 
> > > 
> > > On 6/26/25 3:35 AM, Hans de Goede wrote:
> > > > Hi Mario,
> > > > 
> > > > On 25-Jun-25 23:58, Mario Limonciello wrote:
> > > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > > 
> > > > > Sending an input event to wake a system does wake it, but userspace picks
> > > > > up the keypress and processes it.  This isn't the intended behavior as it
> > > > > causes a suspended system to wake up and then potentially turn off if
> > > > > userspace is configured to turn off on power button presses.
> > > > > 
> > > > > Instead send a PM wakeup event for the PM core to handle waking the system.
> > > > > 
> > > > > Cc: Hans de Goede <hansg@kernel.org>
> > > > > Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > ---
> > > > >    drivers/input/keyboard/gpio_keys.c | 7 +------
> > > > >    1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > > > index 773aa5294d269..4c6876b099c43 100644
> > > > > --- a/drivers/input/keyboard/gpio_keys.c
> > > > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > > > @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> > > > >    		pm_stay_awake(bdata->input->dev.parent);
> > > > >    		if (bdata->suspended  &&
> > > > >    		    (button->type == 0 || button->type == EV_KEY)) {
> > > > > -			/*
> > > > > -			 * Simulate wakeup key press in case the key has
> > > > > -			 * already released by the time we got interrupt
> > > > > -			 * handler to run.
> > > > > -			 */
> > > > > -			input_report_key(bdata->input, button->code, 1);
> > > > > +			pm_wakeup_event(bdata->input->dev.parent, 0);
> > 
> > There is already pm_stay_awake() above.
> 
> But that doesn't help with the fact that userspace gets KEY_POWER from this
> and reacts to it.
> 
> > 
> > > > >    		}
> > > > >    	}
> > > > 
> > > > Hmm, we have the same problem on many Bay Trail / Cherry Trail
> > > > windows 8 / win10 tablets, so  this has been discussed before and e.g.
> > > > Android userspace actually needs the button-press (evdev) event to not
> > > > immediately go back to sleep, so a similar patch has been nacked in
> > > > the past.
> > > > 
> > > > At least for GNOME this has been fixed in userspace by ignoring
> > > > power-button events the first few seconds after a resume from suspend.
> > > > 
> > > 
> > > The default behavior for logind is:
> > > 
> > > HandlePowerKey=poweroff
> > > 
> > > Can you share more about what version of GNOME has a workaround?
> > > This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> > > 
> > > Nonetheless if this is dependent on an Android userspace problem could we
> > > perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> > 
> > No it is not only Android, other userspace may want to distinguish
> > between normal and "dark" resume based on keyboard or other user
> > activity.
> > 
> > Thanks.
> > 
> In this specific case does the key passed up to satisfy this userspace
> requirement and keep it awake need to specifically be a fabricated
> KEY_POWER?
> 
> Or could we find a key that doesn't require some userspace to ignore
> KEY_POWER?
> 
> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?

The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
It simply passes event to userspace for processing.

You need to fix your userspace. Even with your tweak it is possible for
userspace to get a normal key event "too early" and turn off the screen
again, so you still need to handle this situation.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
>>
>>
>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
>>> Hi Mario,
>>>
>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>>>
>>>>
>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>>>> Hi Mario,
>>>>>
>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>
>>>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>>>> causes a suspended system to wake up and then potentially turn off if
>>>>>> userspace is configured to turn off on power button presses.
>>>>>>
>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>>>
>>>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>>> index 773aa5294d269..4c6876b099c43 100644
>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>>>     		pm_stay_awake(bdata->input->dev.parent);
>>>>>>     		if (bdata->suspended  &&
>>>>>>     		    (button->type == 0 || button->type == EV_KEY)) {
>>>>>> -			/*
>>>>>> -			 * Simulate wakeup key press in case the key has
>>>>>> -			 * already released by the time we got interrupt
>>>>>> -			 * handler to run.
>>>>>> -			 */
>>>>>> -			input_report_key(bdata->input, button->code, 1);
>>>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>>>
>>> There is already pm_stay_awake() above.
>>
>> But that doesn't help with the fact that userspace gets KEY_POWER from this
>> and reacts to it.
>>
>>>
>>>>>>     		}
>>>>>>     	}
>>>>>
>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>>>> Android userspace actually needs the button-press (evdev) event to not
>>>>> immediately go back to sleep, so a similar patch has been nacked in
>>>>> the past.
>>>>>
>>>>> At least for GNOME this has been fixed in userspace by ignoring
>>>>> power-button events the first few seconds after a resume from suspend.
>>>>>
>>>>
>>>> The default behavior for logind is:
>>>>
>>>> HandlePowerKey=poweroff
>>>>
>>>> Can you share more about what version of GNOME has a workaround?
>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>>>
>>>> Nonetheless if this is dependent on an Android userspace problem could we
>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
>>>
>>> No it is not only Android, other userspace may want to distinguish
>>> between normal and "dark" resume based on keyboard or other user
>>> activity.
>>>
>>> Thanks.
>>>
>> In this specific case does the key passed up to satisfy this userspace
>> requirement and keep it awake need to specifically be a fabricated
>> KEY_POWER?
>>
>> Or could we find a key that doesn't require some userspace to ignore
>> KEY_POWER?
>>
>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> 
> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> It simply passes event to userspace for processing.

Right.  I don't expect a problem with most keys, but my proposal is to 
special case KEY_POWER while suspended.  If a key press event must be 
sent to keep Android and other userspace happy I suggest sending 
something different just for that situation.

Like this:

diff --git a/drivers/input/keyboard/gpio_keys.c 
b/drivers/input/keyboard/gpio_keys.c
index 773aa5294d269..66e788d381956 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void 
*dev_id)
                          * already released by the time we got interrupt
                          * handler to run.
                          */
-                       input_report_key(bdata->input, button->code, 1);
+                       if (button->code == KEY_POWER)
+                               input_report_key(bdata->input, 
KEY_WAKEUP, 1);
+                       else
+                               input_report_key(bdata->input, 
button->code, 1);
                 }
         }



> 
> You need to fix your userspace. Even with your tweak it is possible for
> userspace to get a normal key event "too early" and turn off the screen
> again, so you still need to handle this situation.
> 
> Thanks.
> 

I want to note this driver works quite differently than how ACPI power 
button does.

You can see in acpi_button_notify() that the "keypress" is only 
forwarded when not suspended [1].  Otherwise it's just wakeup event 
(which is what my patch was modeling).

https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461 
[1]

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> > > 
> > > 
> > > On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> > > > Hi Mario,
> > > > 
> > > > On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> > > > > 
> > > > > 
> > > > > On 6/26/25 3:35 AM, Hans de Goede wrote:
> > > > > > Hi Mario,
> > > > > > 
> > > > > > On 25-Jun-25 23:58, Mario Limonciello wrote:
> > > > > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > > 
> > > > > > > Sending an input event to wake a system does wake it, but userspace picks
> > > > > > > up the keypress and processes it.  This isn't the intended behavior as it
> > > > > > > causes a suspended system to wake up and then potentially turn off if
> > > > > > > userspace is configured to turn off on power button presses.
> > > > > > > 
> > > > > > > Instead send a PM wakeup event for the PM core to handle waking the system.
> > > > > > > 
> > > > > > > Cc: Hans de Goede <hansg@kernel.org>
> > > > > > > Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > > ---
> > > > > > >     drivers/input/keyboard/gpio_keys.c | 7 +------
> > > > > > >     1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > > > > > index 773aa5294d269..4c6876b099c43 100644
> > > > > > > --- a/drivers/input/keyboard/gpio_keys.c
> > > > > > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > > > > > @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> > > > > > >     		pm_stay_awake(bdata->input->dev.parent);
> > > > > > >     		if (bdata->suspended  &&
> > > > > > >     		    (button->type == 0 || button->type == EV_KEY)) {
> > > > > > > -			/*
> > > > > > > -			 * Simulate wakeup key press in case the key has
> > > > > > > -			 * already released by the time we got interrupt
> > > > > > > -			 * handler to run.
> > > > > > > -			 */
> > > > > > > -			input_report_key(bdata->input, button->code, 1);
> > > > > > > +			pm_wakeup_event(bdata->input->dev.parent, 0);
> > > > 
> > > > There is already pm_stay_awake() above.
> > > 
> > > But that doesn't help with the fact that userspace gets KEY_POWER from this
> > > and reacts to it.
> > > 
> > > > 
> > > > > > >     		}
> > > > > > >     	}
> > > > > > 
> > > > > > Hmm, we have the same problem on many Bay Trail / Cherry Trail
> > > > > > windows 8 / win10 tablets, so  this has been discussed before and e.g.
> > > > > > Android userspace actually needs the button-press (evdev) event to not
> > > > > > immediately go back to sleep, so a similar patch has been nacked in
> > > > > > the past.
> > > > > > 
> > > > > > At least for GNOME this has been fixed in userspace by ignoring
> > > > > > power-button events the first few seconds after a resume from suspend.
> > > > > > 
> > > > > 
> > > > > The default behavior for logind is:
> > > > > 
> > > > > HandlePowerKey=poweroff
> > > > > 
> > > > > Can you share more about what version of GNOME has a workaround?
> > > > > This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> > > > > 
> > > > > Nonetheless if this is dependent on an Android userspace problem could we
> > > > > perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> > > > 
> > > > No it is not only Android, other userspace may want to distinguish
> > > > between normal and "dark" resume based on keyboard or other user
> > > > activity.
> > > > 
> > > > Thanks.
> > > > 
> > > In this specific case does the key passed up to satisfy this userspace
> > > requirement and keep it awake need to specifically be a fabricated
> > > KEY_POWER?
> > > 
> > > Or could we find a key that doesn't require some userspace to ignore
> > > KEY_POWER?
> > > 
> > > Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> > 
> > The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> > It simply passes event to userspace for processing.
> 
> Right.  I don't expect a problem with most keys, but my proposal is to
> special case KEY_POWER while suspended.  If a key press event must be sent
> to keep Android and other userspace happy I suggest sending something
> different just for that situation.

I do not know if userspace specifically looks for KEY_POWER or if it
looks for user input in general, and I'd rather be on safe side and not
mangle user input.

As Hans mentioned, at least some userspace already prepared to deal with
this issue. And again, this only works if by the time ISR/debounce
runs the key is already released. What if it is still pressed? You still
going to observe KEY_POWER and need to suppress turning off the screen.

> 
> Like this:
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c
> b/drivers/input/keyboard/gpio_keys.c
> index 773aa5294d269..66e788d381956 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
> *dev_id)
>                          * already released by the time we got interrupt
>                          * handler to run.
>                          */
> -                       input_report_key(bdata->input, button->code, 1);
> +                       if (button->code == KEY_POWER)
> +                               input_report_key(bdata->input, KEY_WAKEUP,
> 1);

Just FYI: Here your KEY_WAKEUP is stuck forever.

> +                       else
> +                               input_report_key(bdata->input, button->code,
> 1);
>                 }
>         }
> 
> 
> 
> > 
> > You need to fix your userspace. Even with your tweak it is possible for
> > userspace to get a normal key event "too early" and turn off the screen
> > again, so you still need to handle this situation.
> > 
> > Thanks.
> > 
> 
> I want to note this driver works quite differently than how ACPI power
> button does.
> 
> You can see in acpi_button_notify() that the "keypress" is only forwarded
> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> patch was modeling).
> 
> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> [1]

If you check acpi_button_resume() you will see that the events are sent
from there. Except that for some reason they chose to use KEY_WAKEUP and
not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
multiple other platforms.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/2025 1:48 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
>>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
>>>>
>>>>
>>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
>>>>> Hi Mario,
>>>>>
>>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>>>>>
>>>>>>
>>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>>>
>>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>>>>>> causes a suspended system to wake up and then potentially turn off if
>>>>>>>> userspace is configured to turn off on power button presses.
>>>>>>>>
>>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>>>>>
>>>>>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>>> ---
>>>>>>>>      drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>>>>>      1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>>>>> index 773aa5294d269..4c6876b099c43 100644
>>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>>>>>      		pm_stay_awake(bdata->input->dev.parent);
>>>>>>>>      		if (bdata->suspended  &&
>>>>>>>>      		    (button->type == 0 || button->type == EV_KEY)) {
>>>>>>>> -			/*
>>>>>>>> -			 * Simulate wakeup key press in case the key has
>>>>>>>> -			 * already released by the time we got interrupt
>>>>>>>> -			 * handler to run.
>>>>>>>> -			 */
>>>>>>>> -			input_report_key(bdata->input, button->code, 1);
>>>>>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>>>>>
>>>>> There is already pm_stay_awake() above.
>>>>
>>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
>>>> and reacts to it.
>>>>
>>>>>
>>>>>>>>      		}
>>>>>>>>      	}
>>>>>>>
>>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>>>>>> Android userspace actually needs the button-press (evdev) event to not
>>>>>>> immediately go back to sleep, so a similar patch has been nacked in
>>>>>>> the past.
>>>>>>>
>>>>>>> At least for GNOME this has been fixed in userspace by ignoring
>>>>>>> power-button events the first few seconds after a resume from suspend.
>>>>>>>
>>>>>>
>>>>>> The default behavior for logind is:
>>>>>>
>>>>>> HandlePowerKey=poweroff
>>>>>>
>>>>>> Can you share more about what version of GNOME has a workaround?
>>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>>>>>
>>>>>> Nonetheless if this is dependent on an Android userspace problem could we
>>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
>>>>>
>>>>> No it is not only Android, other userspace may want to distinguish
>>>>> between normal and "dark" resume based on keyboard or other user
>>>>> activity.
>>>>>
>>>>> Thanks.
>>>>>
>>>> In this specific case does the key passed up to satisfy this userspace
>>>> requirement and keep it awake need to specifically be a fabricated
>>>> KEY_POWER?
>>>>
>>>> Or could we find a key that doesn't require some userspace to ignore
>>>> KEY_POWER?
>>>>
>>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
>>>
>>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
>>> It simply passes event to userspace for processing.
>>
>> Right.  I don't expect a problem with most keys, but my proposal is to
>> special case KEY_POWER while suspended.  If a key press event must be sent
>> to keep Android and other userspace happy I suggest sending something
>> different just for that situation.
> 
> I do not know if userspace specifically looks for KEY_POWER or if it
> looks for user input in general, and I'd rather be on safe side and not
> mangle user input.
> 
> As Hans mentioned, at least some userspace already prepared to deal with
> this issue. And again, this only works if by the time ISR/debounce
> runs the key is already released. What if it is still pressed? You still
> going to observe KEY_POWER and need to suppress turning off the screen.
> 
>>
>> Like this:
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c
>> b/drivers/input/keyboard/gpio_keys.c
>> index 773aa5294d269..66e788d381956 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>> *dev_id)
>>                           * already released by the time we got interrupt
>>                           * handler to run.
>>                           */
>> -                       input_report_key(bdata->input, button->code, 1);
>> +                       if (button->code == KEY_POWER)
>> +                               input_report_key(bdata->input, KEY_WAKEUP,
>> 1);
> 
> Just FYI: Here your KEY_WAKEUP is stuck forever.

Thanks.

> 
>> +                       else
>> +                               input_report_key(bdata->input, button->code,
>> 1);
>>                  }
>>          }
>>
>>
>>
>>>
>>> You need to fix your userspace. Even with your tweak it is possible for
>>> userspace to get a normal key event "too early" and turn off the screen
>>> again, so you still need to handle this situation.
>>>
>>> Thanks.
>>>
>>
>> I want to note this driver works quite differently than how ACPI power
>> button does.
>>
>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>> patch was modeling).
>>
>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>> [1]
> 
> If you check acpi_button_resume() you will see that the events are sent
> from there. Except that for some reason they chose to use KEY_WAKEUP and
> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> multiple other platforms.
> 
> Thanks.
> 

Well that would explain the difference, and git blame gives the history [1].

It's from enablement for Android with ACPI power button.  That commit 
also mentions that Android can handle both POWER and WAKEUP from input 
device to wakeup the system.  Non-Android userspace doesn't do anything 
with KEY_WAKEUP today.

So this has me thinking the proposal I had above to special case 
KEY_POWER and translate to KEY_WAKEUP is the right way forward, just 
making sure to release the key as you rightfully pointed out.

https://github.com/torvalds/linux/commit/16f70feaabe9fde0af703f2991d44a7589f0b6e3 
[1]

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 01:55:29PM -0500, Mario Limonciello wrote:
> On 6/26/2025 1:48 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> > > On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> > > > On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> > > > > 
> > > > > 
> > > > > On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> > > > > > Hi Mario,
> > > > > > 
> > > > > > On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 6/26/25 3:35 AM, Hans de Goede wrote:
> > > > > > > > Hi Mario,
> > > > > > > > 
> > > > > > > > On 25-Jun-25 23:58, Mario Limonciello wrote:
> > > > > > > > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > > > > 
> > > > > > > > > Sending an input event to wake a system does wake it, but userspace picks
> > > > > > > > > up the keypress and processes it.  This isn't the intended behavior as it
> > > > > > > > > causes a suspended system to wake up and then potentially turn off if
> > > > > > > > > userspace is configured to turn off on power button presses.
> > > > > > > > > 
> > > > > > > > > Instead send a PM wakeup event for the PM core to handle waking the system.
> > > > > > > > > 
> > > > > > > > > Cc: Hans de Goede <hansg@kernel.org>
> > > > > > > > > Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > > > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > > > > > > > ---
> > > > > > > > >      drivers/input/keyboard/gpio_keys.c | 7 +------
> > > > > > > > >      1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > > > > > > > index 773aa5294d269..4c6876b099c43 100644
> > > > > > > > > --- a/drivers/input/keyboard/gpio_keys.c
> > > > > > > > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > > > > > > > @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> > > > > > > > >      		pm_stay_awake(bdata->input->dev.parent);
> > > > > > > > >      		if (bdata->suspended  &&
> > > > > > > > >      		    (button->type == 0 || button->type == EV_KEY)) {
> > > > > > > > > -			/*
> > > > > > > > > -			 * Simulate wakeup key press in case the key has
> > > > > > > > > -			 * already released by the time we got interrupt
> > > > > > > > > -			 * handler to run.
> > > > > > > > > -			 */
> > > > > > > > > -			input_report_key(bdata->input, button->code, 1);
> > > > > > > > > +			pm_wakeup_event(bdata->input->dev.parent, 0);
> > > > > > 
> > > > > > There is already pm_stay_awake() above.
> > > > > 
> > > > > But that doesn't help with the fact that userspace gets KEY_POWER from this
> > > > > and reacts to it.
> > > > > 
> > > > > > 
> > > > > > > > >      		}
> > > > > > > > >      	}
> > > > > > > > 
> > > > > > > > Hmm, we have the same problem on many Bay Trail / Cherry Trail
> > > > > > > > windows 8 / win10 tablets, so  this has been discussed before and e.g.
> > > > > > > > Android userspace actually needs the button-press (evdev) event to not
> > > > > > > > immediately go back to sleep, so a similar patch has been nacked in
> > > > > > > > the past.
> > > > > > > > 
> > > > > > > > At least for GNOME this has been fixed in userspace by ignoring
> > > > > > > > power-button events the first few seconds after a resume from suspend.
> > > > > > > > 
> > > > > > > 
> > > > > > > The default behavior for logind is:
> > > > > > > 
> > > > > > > HandlePowerKey=poweroff
> > > > > > > 
> > > > > > > Can you share more about what version of GNOME has a workaround?
> > > > > > > This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> > > > > > > 
> > > > > > > Nonetheless if this is dependent on an Android userspace problem could we
> > > > > > > perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> > > > > > 
> > > > > > No it is not only Android, other userspace may want to distinguish
> > > > > > between normal and "dark" resume based on keyboard or other user
> > > > > > activity.
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > In this specific case does the key passed up to satisfy this userspace
> > > > > requirement and keep it awake need to specifically be a fabricated
> > > > > KEY_POWER?
> > > > > 
> > > > > Or could we find a key that doesn't require some userspace to ignore
> > > > > KEY_POWER?
> > > > > 
> > > > > Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> > > > 
> > > > The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> > > > It simply passes event to userspace for processing.
> > > 
> > > Right.  I don't expect a problem with most keys, but my proposal is to
> > > special case KEY_POWER while suspended.  If a key press event must be sent
> > > to keep Android and other userspace happy I suggest sending something
> > > different just for that situation.
> > 
> > I do not know if userspace specifically looks for KEY_POWER or if it
> > looks for user input in general, and I'd rather be on safe side and not
> > mangle user input.
> > 
> > As Hans mentioned, at least some userspace already prepared to deal with
> > this issue. And again, this only works if by the time ISR/debounce
> > runs the key is already released. What if it is still pressed? You still
> > going to observe KEY_POWER and need to suppress turning off the screen.
> > 
> > > 
> > > Like this:
> > > 
> > > diff --git a/drivers/input/keyboard/gpio_keys.c
> > > b/drivers/input/keyboard/gpio_keys.c
> > > index 773aa5294d269..66e788d381956 100644
> > > --- a/drivers/input/keyboard/gpio_keys.c
> > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
> > > *dev_id)
> > >                           * already released by the time we got interrupt
> > >                           * handler to run.
> > >                           */
> > > -                       input_report_key(bdata->input, button->code, 1);
> > > +                       if (button->code == KEY_POWER)
> > > +                               input_report_key(bdata->input, KEY_WAKEUP,
> > > 1);
> > 
> > Just FYI: Here your KEY_WAKEUP is stuck forever.
> 
> Thanks.
> 
> > 
> > > +                       else
> > > +                               input_report_key(bdata->input, button->code,
> > > 1);
> > >                  }
> > >          }
> > > 
> > > 
> > > 
> > > > 
> > > > You need to fix your userspace. Even with your tweak it is possible for
> > > > userspace to get a normal key event "too early" and turn off the screen
> > > > again, so you still need to handle this situation.
> > > > 
> > > > Thanks.
> > > > 
> > > 
> > > I want to note this driver works quite differently than how ACPI power
> > > button does.
> > > 
_> > > You can see in acpi_button_notify() that the "keypress" is only forwarded
> > > when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > > patch was modeling).
> > > 
> > > https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > > [1]
> > 
> > If you check acpi_button_resume() you will see that the events are sent
> > from there. Except that for some reason they chose to use KEY_WAKEUP and
> > not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > multiple other platforms.
> > 
> > Thanks.
> > 
> 
> Well that would explain the difference, and git blame gives the history [1].
> 
> It's from enablement for Android with ACPI power button.  That commit also
> mentions that Android can handle both POWER and WAKEUP from input device to
> wakeup the system.  Non-Android userspace doesn't do anything with
> KEY_WAKEUP today.

*All* non-Android userspace?

> 
> So this has me thinking the proposal I had above to special case KEY_POWER
> and translate to KEY_WAKEUP is the right way forward, just making sure to
> release the key as you rightfully pointed out.

You keep ignoring the fact that it does not solve your issue when the
key/button is pressed just a tad longer.

There are a ton of drivers that report KEY_POWER and do not convert it
to anything else on resume. I will not accept patches that mangle input
events in the gpio_keys driver, sorry.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Andy Shevchenko]

On Thu, Jun 26, 2025 at 01:55:29PM -0500, Mario Limonciello wrote:

Mario, I see that this discussion takes longer than expected, however I agree
that adding debounce call in GPIO ACPI lib is right thing to do independently
on the outcome of this discussion. So, I have looked into the code and think
that the patches do not need to be squashed. I have some tweaks and I would
like to take your patches as two separate entities and add my code on top.
I will cook something tomorrow and send out based on this version of yours.

P.S. TWIMC, It's only about the code that touches gpiolib-acpi.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
>>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
>>>>
>>>>
>>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
>>>>> Hi Mario,
>>>>>
>>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>>>>>
>>>>>>
>>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>>>
>>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>>>>>> causes a suspended system to wake up and then potentially turn off if
>>>>>>>> userspace is configured to turn off on power button presses.
>>>>>>>>
>>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>>>>>
>>>>>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>>> ---
>>>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>>>>> index 773aa5294d269..4c6876b099c43 100644
>>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>>>>>     		pm_stay_awake(bdata->input->dev.parent);
>>>>>>>>     		if (bdata->suspended  &&
>>>>>>>>     		    (button->type == 0 || button->type == EV_KEY)) {
>>>>>>>> -			/*
>>>>>>>> -			 * Simulate wakeup key press in case the key has
>>>>>>>> -			 * already released by the time we got interrupt
>>>>>>>> -			 * handler to run.
>>>>>>>> -			 */
>>>>>>>> -			input_report_key(bdata->input, button->code, 1);
>>>>>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>>>>>
>>>>> There is already pm_stay_awake() above.
>>>>
>>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
>>>> and reacts to it.
>>>>
>>>>>
>>>>>>>>     		}
>>>>>>>>     	}
>>>>>>>
>>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>>>>>> Android userspace actually needs the button-press (evdev) event to not
>>>>>>> immediately go back to sleep, so a similar patch has been nacked in
>>>>>>> the past.
>>>>>>>
>>>>>>> At least for GNOME this has been fixed in userspace by ignoring
>>>>>>> power-button events the first few seconds after a resume from suspend.
>>>>>>>
>>>>>>
>>>>>> The default behavior for logind is:
>>>>>>
>>>>>> HandlePowerKey=poweroff
>>>>>>
>>>>>> Can you share more about what version of GNOME has a workaround?
>>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>>>>>
>>>>>> Nonetheless if this is dependent on an Android userspace problem could we
>>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
>>>>>
>>>>> No it is not only Android, other userspace may want to distinguish
>>>>> between normal and "dark" resume based on keyboard or other user
>>>>> activity.
>>>>>
>>>>> Thanks.
>>>>>
>>>> In this specific case does the key passed up to satisfy this userspace
>>>> requirement and keep it awake need to specifically be a fabricated
>>>> KEY_POWER?
>>>>
>>>> Or could we find a key that doesn't require some userspace to ignore
>>>> KEY_POWER?
>>>>
>>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
>>>
>>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
>>> It simply passes event to userspace for processing.
>>
>> Right.  I don't expect a problem with most keys, but my proposal is to
>> special case KEY_POWER while suspended.  If a key press event must be sent
>> to keep Android and other userspace happy I suggest sending something
>> different just for that situation.
> 
> I do not know if userspace specifically looks for KEY_POWER or if it
> looks for user input in general, and I'd rather be on safe side and not
> mangle user input.
> 
> As Hans mentioned, at least some userspace already prepared to deal with
> this issue. And again, this only works if by the time ISR/debounce
> runs the key is already released. What if it is still pressed? You still
> going to observe KEY_POWER and need to suppress turning off the screen.
> 
>>
>> Like this:
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c
>> b/drivers/input/keyboard/gpio_keys.c
>> index 773aa5294d269..66e788d381956 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>> *dev_id)
>>                          * already released by the time we got interrupt
>>                          * handler to run.
>>                          */
>> -                       input_report_key(bdata->input, button->code, 1);
>> +                       if (button->code == KEY_POWER)
>> +                               input_report_key(bdata->input, KEY_WAKEUP,
>> 1);
> 
> Just FYI: Here your KEY_WAKEUP is stuck forever.
> 
>> +                       else
>> +                               input_report_key(bdata->input, button->code,
>> 1);
>>                 }
>>         }
>>
>>
>>
>>>
>>> You need to fix your userspace. Even with your tweak it is possible for
>>> userspace to get a normal key event "too early" and turn off the screen
>>> again, so you still need to handle this situation.
>>>
>>> Thanks.
>>>
>>
>> I want to note this driver works quite differently than how ACPI power
>> button does.
>>
>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>> patch was modeling).
>>
>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>> [1]
> 
> If you check acpi_button_resume() you will see that the events are sent
> from there. Except that for some reason they chose to use KEY_WAKEUP and
> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> multiple other platforms.

Interesting, but the ACPI button code presumably only does this on resume
for a normal press while the system is awake it does use KEY_POWER, right ?

Regards,

Hans


> 
> Thanks.
> 

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> >> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> >>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> >>>>
> >>>>
> >>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> >>>>> Hi Mario,
> >>>>>
> >>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
> >>>>>>> Hi Mario,
> >>>>>>>
> >>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
> >>>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>>>>>>
> >>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
> >>>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
> >>>>>>>> causes a suspended system to wake up and then potentially turn off if
> >>>>>>>> userspace is configured to turn off on power button presses.
> >>>>>>>>
> >>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
> >>>>>>>>
> >>>>>>>> Cc: Hans de Goede <hansg@kernel.org>
> >>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> >>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>>>>>>> ---
> >>>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
> >>>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> >>>>>>>> index 773aa5294d269..4c6876b099c43 100644
> >>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
> >>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> >>>>>>>>     		pm_stay_awake(bdata->input->dev.parent);
> >>>>>>>>     		if (bdata->suspended  &&
> >>>>>>>>     		    (button->type == 0 || button->type == EV_KEY)) {
> >>>>>>>> -			/*
> >>>>>>>> -			 * Simulate wakeup key press in case the key has
> >>>>>>>> -			 * already released by the time we got interrupt
> >>>>>>>> -			 * handler to run.
> >>>>>>>> -			 */
> >>>>>>>> -			input_report_key(bdata->input, button->code, 1);
> >>>>>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
> >>>>>
> >>>>> There is already pm_stay_awake() above.
> >>>>
> >>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
> >>>> and reacts to it.
> >>>>
> >>>>>
> >>>>>>>>     		}
> >>>>>>>>     	}
> >>>>>>>
> >>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
> >>>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
> >>>>>>> Android userspace actually needs the button-press (evdev) event to not
> >>>>>>> immediately go back to sleep, so a similar patch has been nacked in
> >>>>>>> the past.
> >>>>>>>
> >>>>>>> At least for GNOME this has been fixed in userspace by ignoring
> >>>>>>> power-button events the first few seconds after a resume from suspend.
> >>>>>>>
> >>>>>>
> >>>>>> The default behavior for logind is:
> >>>>>>
> >>>>>> HandlePowerKey=poweroff
> >>>>>>
> >>>>>> Can you share more about what version of GNOME has a workaround?
> >>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> >>>>>>
> >>>>>> Nonetheless if this is dependent on an Android userspace problem could we
> >>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> >>>>>
> >>>>> No it is not only Android, other userspace may want to distinguish
> >>>>> between normal and "dark" resume based on keyboard or other user
> >>>>> activity.
> >>>>>
> >>>>> Thanks.
> >>>>>
> >>>> In this specific case does the key passed up to satisfy this userspace
> >>>> requirement and keep it awake need to specifically be a fabricated
> >>>> KEY_POWER?
> >>>>
> >>>> Or could we find a key that doesn't require some userspace to ignore
> >>>> KEY_POWER?
> >>>>
> >>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> >>>
> >>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> >>> It simply passes event to userspace for processing.
> >>
> >> Right.  I don't expect a problem with most keys, but my proposal is to
> >> special case KEY_POWER while suspended.  If a key press event must be sent
> >> to keep Android and other userspace happy I suggest sending something
> >> different just for that situation.
> > 
> > I do not know if userspace specifically looks for KEY_POWER or if it
> > looks for user input in general, and I'd rather be on safe side and not
> > mangle user input.
> > 
> > As Hans mentioned, at least some userspace already prepared to deal with
> > this issue. And again, this only works if by the time ISR/debounce
> > runs the key is already released. What if it is still pressed? You still
> > going to observe KEY_POWER and need to suppress turning off the screen.
> > 
> >>
> >> Like this:
> >>
> >> diff --git a/drivers/input/keyboard/gpio_keys.c
> >> b/drivers/input/keyboard/gpio_keys.c
> >> index 773aa5294d269..66e788d381956 100644
> >> --- a/drivers/input/keyboard/gpio_keys.c
> >> +++ b/drivers/input/keyboard/gpio_keys.c
> >> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
> >> *dev_id)
> >>                          * already released by the time we got interrupt
> >>                          * handler to run.
> >>                          */
> >> -                       input_report_key(bdata->input, button->code, 1);
> >> +                       if (button->code == KEY_POWER)
> >> +                               input_report_key(bdata->input, KEY_WAKEUP,
> >> 1);
> > 
> > Just FYI: Here your KEY_WAKEUP is stuck forever.
> > 
> >> +                       else
> >> +                               input_report_key(bdata->input, button->code,
> >> 1);
> >>                 }
> >>         }
> >>
> >>
> >>
> >>>
> >>> You need to fix your userspace. Even with your tweak it is possible for
> >>> userspace to get a normal key event "too early" and turn off the screen
> >>> again, so you still need to handle this situation.
> >>>
> >>> Thanks.
> >>>
> >>
> >> I want to note this driver works quite differently than how ACPI power
> >> button does.
> >>
> >> You can see in acpi_button_notify() that the "keypress" is only forwarded
> >> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> >> patch was modeling).
> >>
> >> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> >> [1]
> > 
> > If you check acpi_button_resume() you will see that the events are sent
> > from there. Except that for some reason they chose to use KEY_WAKEUP and
> > not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > multiple other platforms.
> 
> Interesting, but the ACPI button code presumably only does this on resume
> for a normal press while the system is awake it does use KEY_POWER, right ?

Yes. It is unclear to me why they chose to mangle the event on wakeup,
it does not seem to be captured in the email discussions or in the patch
description.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>>>> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
>>>>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
>>>>>>
>>>>>>
>>>>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
>>>>>>> Hi Mario,
>>>>>>>
>>>>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>>>>>>>> Hi Mario,
>>>>>>>>>
>>>>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>>>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>>>>>
>>>>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>>>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>>>>>>>> causes a suspended system to wake up and then potentially turn off if
>>>>>>>>>> userspace is configured to turn off on power button presses.
>>>>>>>>>>
>>>>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>>>>>>>
>>>>>>>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>>>>> ---
>>>>>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>>>>>>> index 773aa5294d269..4c6876b099c43 100644
>>>>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>>>>>>>     		pm_stay_awake(bdata->input->dev.parent);
>>>>>>>>>>     		if (bdata->suspended  &&
>>>>>>>>>>     		    (button->type == 0 || button->type == EV_KEY)) {
>>>>>>>>>> -			/*
>>>>>>>>>> -			 * Simulate wakeup key press in case the key has
>>>>>>>>>> -			 * already released by the time we got interrupt
>>>>>>>>>> -			 * handler to run.
>>>>>>>>>> -			 */
>>>>>>>>>> -			input_report_key(bdata->input, button->code, 1);
>>>>>>>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>>>>>>>
>>>>>>> There is already pm_stay_awake() above.
>>>>>>
>>>>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
>>>>>> and reacts to it.
>>>>>>
>>>>>>>
>>>>>>>>>>     		}
>>>>>>>>>>     	}
>>>>>>>>>
>>>>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>>>>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>>>>>>>> Android userspace actually needs the button-press (evdev) event to not
>>>>>>>>> immediately go back to sleep, so a similar patch has been nacked in
>>>>>>>>> the past.
>>>>>>>>>
>>>>>>>>> At least for GNOME this has been fixed in userspace by ignoring
>>>>>>>>> power-button events the first few seconds after a resume from suspend.
>>>>>>>>>
>>>>>>>>
>>>>>>>> The default behavior for logind is:
>>>>>>>>
>>>>>>>> HandlePowerKey=poweroff
>>>>>>>>
>>>>>>>> Can you share more about what version of GNOME has a workaround?
>>>>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>>>>>>>
>>>>>>>> Nonetheless if this is dependent on an Android userspace problem could we
>>>>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
>>>>>>>
>>>>>>> No it is not only Android, other userspace may want to distinguish
>>>>>>> between normal and "dark" resume based on keyboard or other user
>>>>>>> activity.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>> In this specific case does the key passed up to satisfy this userspace
>>>>>> requirement and keep it awake need to specifically be a fabricated
>>>>>> KEY_POWER?
>>>>>>
>>>>>> Or could we find a key that doesn't require some userspace to ignore
>>>>>> KEY_POWER?
>>>>>>
>>>>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
>>>>>
>>>>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
>>>>> It simply passes event to userspace for processing.
>>>>
>>>> Right.  I don't expect a problem with most keys, but my proposal is to
>>>> special case KEY_POWER while suspended.  If a key press event must be sent
>>>> to keep Android and other userspace happy I suggest sending something
>>>> different just for that situation.
>>>
>>> I do not know if userspace specifically looks for KEY_POWER or if it
>>> looks for user input in general, and I'd rather be on safe side and not
>>> mangle user input.
>>>
>>> As Hans mentioned, at least some userspace already prepared to deal with
>>> this issue. And again, this only works if by the time ISR/debounce
>>> runs the key is already released. What if it is still pressed? You still
>>> going to observe KEY_POWER and need to suppress turning off the screen.
>>>
>>>>
>>>> Like this:
>>>>
>>>> diff --git a/drivers/input/keyboard/gpio_keys.c
>>>> b/drivers/input/keyboard/gpio_keys.c
>>>> index 773aa5294d269..66e788d381956 100644
>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>>>> *dev_id)
>>>>                          * already released by the time we got interrupt
>>>>                          * handler to run.
>>>>                          */
>>>> -                       input_report_key(bdata->input, button->code, 1);
>>>> +                       if (button->code == KEY_POWER)
>>>> +                               input_report_key(bdata->input, KEY_WAKEUP,
>>>> 1);
>>>
>>> Just FYI: Here your KEY_WAKEUP is stuck forever.
>>>
>>>> +                       else
>>>> +                               input_report_key(bdata->input, button->code,
>>>> 1);
>>>>                 }
>>>>         }
>>>>
>>>>
>>>>
>>>>>
>>>>> You need to fix your userspace. Even with your tweak it is possible for
>>>>> userspace to get a normal key event "too early" and turn off the screen
>>>>> again, so you still need to handle this situation.
>>>>>
>>>>> Thanks.
>>>>>
>>>>
>>>> I want to note this driver works quite differently than how ACPI power
>>>> button does.
>>>>
>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>>>> patch was modeling).
>>>>
>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>>>> [1]
>>>
>>> If you check acpi_button_resume() you will see that the events are sent
>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
>>> multiple other platforms.
>>
>> Interesting, but the ACPI button code presumably only does this on resume
>> for a normal press while the system is awake it does use KEY_POWER, right ?
> 
> Yes. It is unclear to me why they chose to mangle the event on wakeup,
> it does not seem to be captured in the email discussions or in the patch
> description.

I assume they did this to avoid the immediate re-suspend on wakeup by
power-button issue. GNOME has a workaround for this, but I assume that
some userspace desktop environments are still going to have a problem
with this.

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Rafael J. Wysocki]

On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> >>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> >>>> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> >>>>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> >>>>>>> Hi Mario,
> >>>>>>>
> >>>>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
> >>>>>>>>> Hi Mario,
> >>>>>>>>>
> >>>>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
> >>>>>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
> >>>>>>>>>>
> >>>>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
> >>>>>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
> >>>>>>>>>> causes a suspended system to wake up and then potentially turn off if
> >>>>>>>>>> userspace is configured to turn off on power button presses.
> >>>>>>>>>>
> >>>>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
> >>>>>>>>>>
> >>>>>>>>>> Cc: Hans de Goede <hansg@kernel.org>
> >>>>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> >>>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >>>>>>>>>> ---
> >>>>>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
> >>>>>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> >>>>>>>>>> index 773aa5294d269..4c6876b099c43 100644
> >>>>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
> >>>>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>>>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> >>>>>>>>>>              pm_stay_awake(bdata->input->dev.parent);
> >>>>>>>>>>              if (bdata->suspended  &&
> >>>>>>>>>>                  (button->type == 0 || button->type == EV_KEY)) {
> >>>>>>>>>> -                    /*
> >>>>>>>>>> -                     * Simulate wakeup key press in case the key has
> >>>>>>>>>> -                     * already released by the time we got interrupt
> >>>>>>>>>> -                     * handler to run.
> >>>>>>>>>> -                     */
> >>>>>>>>>> -                    input_report_key(bdata->input, button->code, 1);
> >>>>>>>>>> +                    pm_wakeup_event(bdata->input->dev.parent, 0);
> >>>>>>>
> >>>>>>> There is already pm_stay_awake() above.
> >>>>>>
> >>>>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
> >>>>>> and reacts to it.
> >>>>>>
> >>>>>>>
> >>>>>>>>>>              }
> >>>>>>>>>>      }
> >>>>>>>>>
> >>>>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
> >>>>>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
> >>>>>>>>> Android userspace actually needs the button-press (evdev) event to not
> >>>>>>>>> immediately go back to sleep, so a similar patch has been nacked in
> >>>>>>>>> the past.
> >>>>>>>>>
> >>>>>>>>> At least for GNOME this has been fixed in userspace by ignoring
> >>>>>>>>> power-button events the first few seconds after a resume from suspend.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The default behavior for logind is:
> >>>>>>>>
> >>>>>>>> HandlePowerKey=poweroff
> >>>>>>>>
> >>>>>>>> Can you share more about what version of GNOME has a workaround?
> >>>>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> >>>>>>>>
> >>>>>>>> Nonetheless if this is dependent on an Android userspace problem could we
> >>>>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> >>>>>>>
> >>>>>>> No it is not only Android, other userspace may want to distinguish
> >>>>>>> between normal and "dark" resume based on keyboard or other user
> >>>>>>> activity.
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>>>
> >>>>>> In this specific case does the key passed up to satisfy this userspace
> >>>>>> requirement and keep it awake need to specifically be a fabricated
> >>>>>> KEY_POWER?
> >>>>>>
> >>>>>> Or could we find a key that doesn't require some userspace to ignore
> >>>>>> KEY_POWER?
> >>>>>>
> >>>>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> >>>>>
> >>>>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> >>>>> It simply passes event to userspace for processing.
> >>>>
> >>>> Right.  I don't expect a problem with most keys, but my proposal is to
> >>>> special case KEY_POWER while suspended.  If a key press event must be sent
> >>>> to keep Android and other userspace happy I suggest sending something
> >>>> different just for that situation.
> >>>
> >>> I do not know if userspace specifically looks for KEY_POWER or if it
> >>> looks for user input in general, and I'd rather be on safe side and not
> >>> mangle user input.
> >>>
> >>> As Hans mentioned, at least some userspace already prepared to deal with
> >>> this issue. And again, this only works if by the time ISR/debounce
> >>> runs the key is already released. What if it is still pressed? You still
> >>> going to observe KEY_POWER and need to suppress turning off the screen.
> >>>
> >>>>
> >>>> Like this:
> >>>>
> >>>> diff --git a/drivers/input/keyboard/gpio_keys.c
> >>>> b/drivers/input/keyboard/gpio_keys.c
> >>>> index 773aa5294d269..66e788d381956 100644
> >>>> --- a/drivers/input/keyboard/gpio_keys.c
> >>>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>>> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
> >>>> *dev_id)
> >>>>                          * already released by the time we got interrupt
> >>>>                          * handler to run.
> >>>>                          */
> >>>> -                       input_report_key(bdata->input, button->code, 1);
> >>>> +                       if (button->code == KEY_POWER)
> >>>> +                               input_report_key(bdata->input, KEY_WAKEUP,
> >>>> 1);
> >>>
> >>> Just FYI: Here your KEY_WAKEUP is stuck forever.
> >>>
> >>>> +                       else
> >>>> +                               input_report_key(bdata->input, button->code,
> >>>> 1);
> >>>>                 }
> >>>>         }
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>> You need to fix your userspace. Even with your tweak it is possible for
> >>>>> userspace to get a normal key event "too early" and turn off the screen
> >>>>> again, so you still need to handle this situation.
> >>>>>
> >>>>> Thanks.
> >>>>>
> >>>>
> >>>> I want to note this driver works quite differently than how ACPI power
> >>>> button does.
> >>>>
> >>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> >>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> >>>> patch was modeling).
> >>>>
> >>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> >>>> [1]
> >>>
> >>> If you check acpi_button_resume() you will see that the events are sent
> >>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> >>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> >>> multiple other platforms.
> >>
> >> Interesting, but the ACPI button code presumably only does this on resume
> >> for a normal press while the system is awake it does use KEY_POWER, right ?
> >
> > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > it does not seem to be captured in the email discussions or in the patch
> > description.
>
> I assume they did this to avoid the immediate re-suspend on wakeup by
> power-button issue. GNOME has a workaround for this, but I assume that
> some userspace desktop environments are still going to have a problem
> with this.

It was done for this reason IIRC, but it should have been documented
more thoroughly.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> >
> > Hi,
> >
> > On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> > >> Hi,
> > >>
> > >> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > >>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
[...]
> > >>>> I want to note this driver works quite differently than how ACPI power
> > >>>> button does.
> > >>>>
> > >>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> > >>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > >>>> patch was modeling).
> > >>>>
> > >>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > >>>> [1]
> > >>>
> > >>> If you check acpi_button_resume() you will see that the events are sent
> > >>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> > >>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > >>> multiple other platforms.
> > >>
> > >> Interesting, but the ACPI button code presumably only does this on resume
> > >> for a normal press while the system is awake it does use KEY_POWER, right ?
> > >
> > > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > > it does not seem to be captured in the email discussions or in the patch
> > > description.
> >
> > I assume they did this to avoid the immediate re-suspend on wakeup by
> > power-button issue. GNOME has a workaround for this, but I assume that
> > some userspace desktop environments are still going to have a problem
> > with this.
> 
> It was done for this reason IIRC, but it should have been documented
> more thoroughly.

I assert that it should not have been done and instead dealt with in
userspace. There are numerous drivers in the kernel emitting
KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
what keys to process and when.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Rafael J. Wysocki]

On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > > > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> > > >> Hi,
> > > >>
> > > >> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > > >>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> [...]
> > > >>>> I want to note this driver works quite differently than how ACPI power
> > > >>>> button does.
> > > >>>>
> > > >>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> > > >>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > > >>>> patch was modeling).
> > > >>>>
> > > >>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > > >>>> [1]
> > > >>>
> > > >>> If you check acpi_button_resume() you will see that the events are sent
> > > >>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> > > >>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > > >>> multiple other platforms.
> > > >>
> > > >> Interesting, but the ACPI button code presumably only does this on resume
> > > >> for a normal press while the system is awake it does use KEY_POWER, right ?
> > > >
> > > > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > > > it does not seem to be captured in the email discussions or in the patch
> > > > description.
> > >
> > > I assume they did this to avoid the immediate re-suspend on wakeup by
> > > power-button issue. GNOME has a workaround for this, but I assume that
> > > some userspace desktop environments are still going to have a problem
> > > with this.
> >
> > It was done for this reason IIRC, but it should have been documented
> > more thoroughly.
>
> I assert that it should not have been done and instead dealt with in
> userspace. There are numerous drivers in the kernel emitting
> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
> what keys to process and when.

Please see my last message in this thread (just sent) and see the
changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
events").

This appears to be about cases when no event would be signaled to user
space at all (power button wakeup from ACPI S3).

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > > > > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> > > > >> Hi,
> > > > >>
> > > > >> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > > > >>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> > [...]
> > > > >>>> I want to note this driver works quite differently than how ACPI power
> > > > >>>> button does.
> > > > >>>>
> > > > >>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> > > > >>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > > > >>>> patch was modeling).
> > > > >>>>
> > > > >>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > > > >>>> [1]
> > > > >>>
> > > > >>> If you check acpi_button_resume() you will see that the events are sent
> > > > >>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> > > > >>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > > > >>> multiple other platforms.
> > > > >>
> > > > >> Interesting, but the ACPI button code presumably only does this on resume
> > > > >> for a normal press while the system is awake it does use KEY_POWER, right ?
> > > > >
> > > > > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > > > > it does not seem to be captured in the email discussions or in the patch
> > > > > description.
> > > >
> > > > I assume they did this to avoid the immediate re-suspend on wakeup by
> > > > power-button issue. GNOME has a workaround for this, but I assume that
> > > > some userspace desktop environments are still going to have a problem
> > > > with this.
> > >
> > > It was done for this reason IIRC, but it should have been documented
> > > more thoroughly.
> >
> > I assert that it should not have been done and instead dealt with in
> > userspace. There are numerous drivers in the kernel emitting
> > KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
> > what keys to process and when.
> 
> Please see my last message in this thread (just sent) and see the
> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
> events").
> 
> This appears to be about cases when no event would be signaled to user
> space at all (power button wakeup from ACPI S3).

Ahh, in S3 we do not know if we've been woken up with Sleep or Power
button, right? So we can not send the "right" event code and use
"neutral" KEY_WAKEUP for both. Is this right?

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>>
>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>>> [...]
>>>>>>>>> I want to note this driver works quite differently than how ACPI power
>>>>>>>>> button does.
>>>>>>>>>
>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>>>>>>>>> patch was modeling).
>>>>>>>>>
>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>>>>>>>>> [1]
>>>>>>>>
>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
>>>>>>>> multiple other platforms.
>>>>>>>
>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
>>>>>>
>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
>>>>>> it does not seem to be captured in the email discussions or in the patch
>>>>>> description.
>>>>>
>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
>>>>> power-button issue. GNOME has a workaround for this, but I assume that
>>>>> some userspace desktop environments are still going to have a problem
>>>>> with this.
>>>>
>>>> It was done for this reason IIRC, but it should have been documented
>>>> more thoroughly.
>>>
>>> I assert that it should not have been done and instead dealt with in
>>> userspace. There are numerous drivers in the kernel emitting
>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
>>> what keys to process and when.
>>
>> Please see my last message in this thread (just sent) and see the
>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
>> events").
>>
>> This appears to be about cases when no event would be signaled to user
>> space at all (power button wakeup from ACPI S3).
> 
> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
> button, right? So we can not send the "right" event code and use
> "neutral" KEY_WAKEUP for both. Is this right?
> 
> Thanks.
> 

I did some more experiments with this affected system that started this 
thread (which uses s2idle).

I only applied patch 3 in this series to help the debounce behavior and 
figure out impacts from patch 4 with existing Linux userspace.

If suspended using systemd in GNOME (click the GUI button) on Ubuntu 
24.04 the GNOME workaround mitigates this problem and no visible impact.

If I suspend by hand using the kernel interface and then press power 
button to wake:

# echo mem | sudo tee /sys/power/state:

* When GNOME is running:
I get the shutdown popup and it eventually shuts down.

* When GNOME isn't running (just on a VT):
System shuts down.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > 
> > > > On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> > > > > On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > > > > > > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > > > > > > > > On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> > > > [...]
> > > > > > > > > > I want to note this driver works quite differently than how ACPI power
> > > > > > > > > > button does.
> > > > > > > > > > 
> > > > > > > > > > You can see in acpi_button_notify() that the "keypress" is only forwarded
> > > > > > > > > > when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > > > > > > > > > patch was modeling).
> > > > > > > > > > 
> > > > > > > > > > https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > > > > > > > > > [1]
> > > > > > > > > 
> > > > > > > > > If you check acpi_button_resume() you will see that the events are sent
> > > > > > > > > from there. Except that for some reason they chose to use KEY_WAKEUP and
> > > > > > > > > not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > > > > > > > > multiple other platforms.
> > > > > > > > 
> > > > > > > > Interesting, but the ACPI button code presumably only does this on resume
> > > > > > > > for a normal press while the system is awake it does use KEY_POWER, right ?
> > > > > > > 
> > > > > > > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > > > > > > it does not seem to be captured in the email discussions or in the patch
> > > > > > > description.
> > > > > > 
> > > > > > I assume they did this to avoid the immediate re-suspend on wakeup by
> > > > > > power-button issue. GNOME has a workaround for this, but I assume that
> > > > > > some userspace desktop environments are still going to have a problem
> > > > > > with this.
> > > > > 
> > > > > It was done for this reason IIRC, but it should have been documented
> > > > > more thoroughly.
> > > > 
> > > > I assert that it should not have been done and instead dealt with in
> > > > userspace. There are numerous drivers in the kernel emitting
> > > > KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
> > > > what keys to process and when.
> > > 
> > > Please see my last message in this thread (just sent) and see the
> > > changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
> > > events").
> > > 
> > > This appears to be about cases when no event would be signaled to user
> > > space at all (power button wakeup from ACPI S3).
> > 
> > Ahh, in S3 we do not know if we've been woken up with Sleep or Power
> > button, right? So we can not send the "right" event code and use
> > "neutral" KEY_WAKEUP for both. Is this right?
> > 
> > Thanks.
> > 
> 
> I did some more experiments with this affected system that started this
> thread (which uses s2idle).
> 
> I only applied patch 3 in this series to help the debounce behavior and
> figure out impacts from patch 4 with existing Linux userspace.
> 
> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
> the GNOME workaround mitigates this problem and no visible impact.
> 
> If I suspend by hand using the kernel interface and then press power button
> to wake:
> 
> # echo mem | sudo tee /sys/power/state:
> 
> * When GNOME is running:
> I get the shutdown popup and it eventually shuts down.
> 
> * When GNOME isn't running (just on a VT):
> System shuts down.

For the latter you may want to raise an issue with systemd, and for the
former I guess it is being too clever and does not activate the
workaround if suspend was not initiated by it? I think Gnome is being
too careful.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
> On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
>> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
>>> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
>>>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>
>>>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
>>>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
>>>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
>>>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>>>>> [...]
>>>>>>>>>>> I want to note this driver works quite differently than how ACPI power
>>>>>>>>>>> button does.
>>>>>>>>>>>
>>>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>>>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>>>>>>>>>>> patch was modeling).
>>>>>>>>>>>
>>>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>>>>>>>>>>> [1]
>>>>>>>>>>
>>>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
>>>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
>>>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
>>>>>>>>>> multiple other platforms.
>>>>>>>>>
>>>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
>>>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
>>>>>>>>
>>>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
>>>>>>>> it does not seem to be captured in the email discussions or in the patch
>>>>>>>> description.
>>>>>>>
>>>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
>>>>>>> power-button issue. GNOME has a workaround for this, but I assume that
>>>>>>> some userspace desktop environments are still going to have a problem
>>>>>>> with this.
>>>>>>
>>>>>> It was done for this reason IIRC, but it should have been documented
>>>>>> more thoroughly.
>>>>>
>>>>> I assert that it should not have been done and instead dealt with in
>>>>> userspace. There are numerous drivers in the kernel emitting
>>>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
>>>>> what keys to process and when.
>>>>
>>>> Please see my last message in this thread (just sent) and see the
>>>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
>>>> events").
>>>>
>>>> This appears to be about cases when no event would be signaled to user
>>>> space at all (power button wakeup from ACPI S3).
>>>
>>> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
>>> button, right? So we can not send the "right" event code and use
>>> "neutral" KEY_WAKEUP for both. Is this right?
>>>
>>> Thanks.
>>>
>>
>> I did some more experiments with this affected system that started this
>> thread (which uses s2idle).
>>
>> I only applied patch 3 in this series to help the debounce behavior and
>> figure out impacts from patch 4 with existing Linux userspace.
>>
>> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
>> the GNOME workaround mitigates this problem and no visible impact.
>>
>> If I suspend by hand using the kernel interface and then press power button
>> to wake:
>>
>> # echo mem | sudo tee /sys/power/state:
>>
>> * When GNOME is running:
>> I get the shutdown popup and it eventually shuts down.
>>
>> * When GNOME isn't running (just on a VT):
>> System shuts down.
> 
> For the latter you may want to raise an issue with systemd, and for the
> former I guess it is being too clever and does not activate the
> workaround if suspend was not initiated by it? I think Gnome is being
> too careful.
> 
> Thanks.
> 

Sure I could file bugs with both the projects.

But before I do if all userspace needs to account for this with a series 
of workarounds at resume time, you still think that is that really the 
best way forward?

Hans, you have a lot of experience in the GNOME community.  Your thoughts?

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
>> On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
>>> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
>>>> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
>>>>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
>>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
>>>>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
>>>>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>>>>>> [...]
>>>>>>>>>>>> I want to note this driver works quite differently than how ACPI power
>>>>>>>>>>>> button does.
>>>>>>>>>>>>
>>>>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>>>>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>>>>>>>>>>>> patch was modeling).
>>>>>>>>>>>>
>>>>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>>>>>>>>>>>> [1]
>>>>>>>>>>>
>>>>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
>>>>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
>>>>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
>>>>>>>>>>> multiple other platforms.
>>>>>>>>>>
>>>>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
>>>>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
>>>>>>>>>
>>>>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
>>>>>>>>> it does not seem to be captured in the email discussions or in the patch
>>>>>>>>> description.
>>>>>>>>
>>>>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
>>>>>>>> power-button issue. GNOME has a workaround for this, but I assume that
>>>>>>>> some userspace desktop environments are still going to have a problem
>>>>>>>> with this.
>>>>>>>
>>>>>>> It was done for this reason IIRC, but it should have been documented
>>>>>>> more thoroughly.
>>>>>>
>>>>>> I assert that it should not have been done and instead dealt with in
>>>>>> userspace. There are numerous drivers in the kernel emitting
>>>>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
>>>>>> what keys to process and when.
>>>>>
>>>>> Please see my last message in this thread (just sent) and see the
>>>>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
>>>>> events").
>>>>>
>>>>> This appears to be about cases when no event would be signaled to user
>>>>> space at all (power button wakeup from ACPI S3).
>>>>
>>>> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
>>>> button, right? So we can not send the "right" event code and use
>>>> "neutral" KEY_WAKEUP for both. Is this right?
>>>>
>>>> Thanks.
>>>>
>>>
>>> I did some more experiments with this affected system that started this
>>> thread (which uses s2idle).
>>>
>>> I only applied patch 3 in this series to help the debounce behavior and
>>> figure out impacts from patch 4 with existing Linux userspace.
>>>
>>> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
>>> the GNOME workaround mitigates this problem and no visible impact.
>>>
>>> If I suspend by hand using the kernel interface and then press power button
>>> to wake:
>>>
>>> # echo mem | sudo tee /sys/power/state:
>>>
>>> * When GNOME is running:
>>> I get the shutdown popup and it eventually shuts down.
>>>
>>> * When GNOME isn't running (just on a VT):
>>> System shuts down.
>>
>> For the latter you may want to raise an issue with systemd, and for the
>> former I guess it is being too clever and does not activate the
>> workaround if suspend was not initiated by it? I think Gnome is being
>> too careful.
>>
>> Thanks.
>>
> 
> Sure I could file bugs with both the projects.
> 
> But before I do if all userspace needs to account for this with a series of workarounds at resume time, you still think that is that really the best way forward?
> 
> Hans, you have a lot of experience in the GNOME community.  Your thoughts?

I guess it would be good to fix this in the kernel, sending
KEY_WAKEUP from gpio_key when the event is KEY_POWER and
we are going through the special wakeup path in gpio_keys.

When this was discussed quite a while ago the ACPI button
driver simply did not send any event at all on wkaeup
by ACPI power-button. Know that it does send an event
it would be good to mimic this, at least when the gpio_key
devices where instantiated by soc_button_array.

So maybe add a new field to struct gpio_keys_button
called wakeup_code and when that is not 0 use that
instead of the plain "code" member on wakeups ?

That would keep the gpio_keys code generic while
allowing to mimic the ACPI button behavior.

And then set wakeup_code to KEY_WAKEUP for
the power-button in soc_button_array.

To me this sounds better then trying to fix all userspace
code which does something on KEY_POWER of which there
is quite a lot.

The special GNOME power-button handling was always
a workaround because last time a kernel fix was
nacked. But now with the KEY_WAKEUP done by the ACPI
button code it looks like we do have a good way
to fix this in the kernel, so that would be better
IMHO.

Dmitry, what do you think of adding a wakeup_code
field to struct gpio_keys_button and let the code
creating the gpio_keys_button decide if a different
code should be used on wakeup or not ?

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
> > On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
> >> On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
> >>> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
> >>>> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
> >>>>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
> >>>>> <dmitry.torokhov@gmail.com> wrote:
> >>>>>>
> >>>>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> >>>>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> >>>>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> >>>>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> >>>>>> [...]
> >>>>>>>>>>>> I want to note this driver works quite differently than how ACPI power
> >>>>>>>>>>>> button does.
> >>>>>>>>>>>>
> >>>>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> >>>>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> >>>>>>>>>>>> patch was modeling).
> >>>>>>>>>>>>
> >>>>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> >>>>>>>>>>>> [1]
> >>>>>>>>>>>
> >>>>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
> >>>>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> >>>>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> >>>>>>>>>>> multiple other platforms.
> >>>>>>>>>>
> >>>>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
> >>>>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
> >>>>>>>>>
> >>>>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
> >>>>>>>>> it does not seem to be captured in the email discussions or in the patch
> >>>>>>>>> description.
> >>>>>>>>
> >>>>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
> >>>>>>>> power-button issue. GNOME has a workaround for this, but I assume that
> >>>>>>>> some userspace desktop environments are still going to have a problem
> >>>>>>>> with this.
> >>>>>>>
> >>>>>>> It was done for this reason IIRC, but it should have been documented
> >>>>>>> more thoroughly.
> >>>>>>
> >>>>>> I assert that it should not have been done and instead dealt with in
> >>>>>> userspace. There are numerous drivers in the kernel emitting
> >>>>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
> >>>>>> what keys to process and when.
> >>>>>
> >>>>> Please see my last message in this thread (just sent) and see the
> >>>>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
> >>>>> events").
> >>>>>
> >>>>> This appears to be about cases when no event would be signaled to user
> >>>>> space at all (power button wakeup from ACPI S3).
> >>>>
> >>>> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
> >>>> button, right? So we can not send the "right" event code and use
> >>>> "neutral" KEY_WAKEUP for both. Is this right?
> >>>>
> >>>> Thanks.
> >>>>
> >>>
> >>> I did some more experiments with this affected system that started this
> >>> thread (which uses s2idle).
> >>>
> >>> I only applied patch 3 in this series to help the debounce behavior and
> >>> figure out impacts from patch 4 with existing Linux userspace.
> >>>
> >>> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
> >>> the GNOME workaround mitigates this problem and no visible impact.
> >>>
> >>> If I suspend by hand using the kernel interface and then press power button
> >>> to wake:
> >>>
> >>> # echo mem | sudo tee /sys/power/state:
> >>>
> >>> * When GNOME is running:
> >>> I get the shutdown popup and it eventually shuts down.
> >>>
> >>> * When GNOME isn't running (just on a VT):
> >>> System shuts down.
> >>
> >> For the latter you may want to raise an issue with systemd, and for the
> >> former I guess it is being too clever and does not activate the
> >> workaround if suspend was not initiated by it? I think Gnome is being
> >> too careful.
> >>
> >> Thanks.
> >>
> > 
> > Sure I could file bugs with both the projects.
> > 
> > But before I do if all userspace needs to account for this with a series of workarounds at resume time, you still think that is that really the best way forward?
> > 
> > Hans, you have a lot of experience in the GNOME community.  Your thoughts?
> 
> I guess it would be good to fix this in the kernel, sending
> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
> we are going through the special wakeup path in gpio_keys.
> 
> When this was discussed quite a while ago the ACPI button
> driver simply did not send any event at all on wkaeup
> by ACPI power-button. Know that it does send an event
> it would be good to mimic this, at least when the gpio_key
> devices where instantiated by soc_button_array.
> 
> So maybe add a new field to struct gpio_keys_button
> called wakeup_code and when that is not 0 use that
> instead of the plain "code" member on wakeups ?
> 
> That would keep the gpio_keys code generic while
> allowing to mimic the ACPI button behavior.
> 
> And then set wakeup_code to KEY_WAKEUP for
> the power-button in soc_button_array.
> 
> To me this sounds better then trying to fix all userspace
> code which does something on KEY_POWER of which there
> is quite a lot.
> 
> The special GNOME power-button handling was always
> a workaround because last time a kernel fix was
> nacked. But now with the KEY_WAKEUP done by the ACPI
> button code it looks like we do have a good way
> to fix this in the kernel, so that would be better
> IMHO.
> 
> Dmitry, what do you think of adding a wakeup_code
> field to struct gpio_keys_button and let the code
> creating the gpio_keys_button decide if a different
> code should be used on wakeup or not ?

And what is the plan on dealing with all other drivers that emit
KEY_POWER? What about acpi button behavior when using S0ix? What about
holding power button for too long so that normal reporting "catches" the
pressed state?

Kernel reports hardware events, interpreting them and applying certain
policies is task for userspace.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi Dmitry,

On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
> On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
>>> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
>>>> On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
>>>>> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
>>>>>> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
>>>>>>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
>>>>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
>>>>>>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
>>>>>>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
>>>>>>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>>>>>>>> [...]
>>>>>>>>>>>>>> I want to note this driver works quite differently than how ACPI power
>>>>>>>>>>>>>> button does.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>>>>>>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>>>>>>>>>>>>>> patch was modeling).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>
>>>>>>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
>>>>>>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
>>>>>>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
>>>>>>>>>>>>> multiple other platforms.
>>>>>>>>>>>>
>>>>>>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
>>>>>>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
>>>>>>>>>>>
>>>>>>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
>>>>>>>>>>> it does not seem to be captured in the email discussions or in the patch
>>>>>>>>>>> description.
>>>>>>>>>>
>>>>>>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
>>>>>>>>>> power-button issue. GNOME has a workaround for this, but I assume that
>>>>>>>>>> some userspace desktop environments are still going to have a problem
>>>>>>>>>> with this.
>>>>>>>>>
>>>>>>>>> It was done for this reason IIRC, but it should have been documented
>>>>>>>>> more thoroughly.
>>>>>>>>
>>>>>>>> I assert that it should not have been done and instead dealt with in
>>>>>>>> userspace. There are numerous drivers in the kernel emitting
>>>>>>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
>>>>>>>> what keys to process and when.
>>>>>>>
>>>>>>> Please see my last message in this thread (just sent) and see the
>>>>>>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
>>>>>>> events").
>>>>>>>
>>>>>>> This appears to be about cases when no event would be signaled to user
>>>>>>> space at all (power button wakeup from ACPI S3).
>>>>>>
>>>>>> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
>>>>>> button, right? So we can not send the "right" event code and use
>>>>>> "neutral" KEY_WAKEUP for both. Is this right?
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>
>>>>> I did some more experiments with this affected system that started this
>>>>> thread (which uses s2idle).
>>>>>
>>>>> I only applied patch 3 in this series to help the debounce behavior and
>>>>> figure out impacts from patch 4 with existing Linux userspace.
>>>>>
>>>>> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
>>>>> the GNOME workaround mitigates this problem and no visible impact.
>>>>>
>>>>> If I suspend by hand using the kernel interface and then press power button
>>>>> to wake:
>>>>>
>>>>> # echo mem | sudo tee /sys/power/state:
>>>>>
>>>>> * When GNOME is running:
>>>>> I get the shutdown popup and it eventually shuts down.
>>>>>
>>>>> * When GNOME isn't running (just on a VT):
>>>>> System shuts down.
>>>>
>>>> For the latter you may want to raise an issue with systemd, and for the
>>>> former I guess it is being too clever and does not activate the
>>>> workaround if suspend was not initiated by it? I think Gnome is being
>>>> too careful.
>>>>
>>>> Thanks.
>>>>
>>>
>>> Sure I could file bugs with both the projects.
>>>
>>> But before I do if all userspace needs to account for this with a series of workarounds at resume time, you still think that is that really the best way forward?
>>>
>>> Hans, you have a lot of experience in the GNOME community.  Your thoughts?
>>
>> I guess it would be good to fix this in the kernel, sending
>> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
>> we are going through the special wakeup path in gpio_keys.
>>
>> When this was discussed quite a while ago the ACPI button
>> driver simply did not send any event at all on wkaeup
>> by ACPI power-button. Know that it does send an event
>> it would be good to mimic this, at least when the gpio_key
>> devices where instantiated by soc_button_array.
>>
>> So maybe add a new field to struct gpio_keys_button
>> called wakeup_code and when that is not 0 use that
>> instead of the plain "code" member on wakeups ?
>>
>> That would keep the gpio_keys code generic while
>> allowing to mimic the ACPI button behavior.
>>
>> And then set wakeup_code to KEY_WAKEUP for
>> the power-button in soc_button_array.
>>
>> To me this sounds better then trying to fix all userspace
>> code which does something on KEY_POWER of which there
>> is quite a lot.
>>
>> The special GNOME power-button handling was always
>> a workaround because last time a kernel fix was
>> nacked. But now with the KEY_WAKEUP done by the ACPI
>> button code it looks like we do have a good way
>> to fix this in the kernel, so that would be better
>> IMHO.
>>
>> Dmitry, what do you think of adding a wakeup_code
>> field to struct gpio_keys_button and let the code
>> creating the gpio_keys_button decide if a different
>> code should be used on wakeup or not ?
> 
> And what is the plan on dealing with all other drivers that emit
> KEY_POWER?

There actually aren't that many that I'm aware of.

Note that this gpio_keys KEY_POWER evdev event generation
on resume issue goes way back until the last time we had
this conversation and it still has not really been fixed.

And I've not seen any bug-reports about the same problem
with any other drivers.

> What about acpi button behavior when using S0ix?

AFAIK it is the same as with S3, at least it is not
causing any issues. I've never seen the ACPI button code
cause re-suspend immediately on wakeup by what for all
intends and purposes is a spurious KEY_POWER event.

Last time we discussed this I wasn't really happy with
the outcome of the discussion but I just went for it
because of Android's reliance on the event and we
lacked a better plan.

Now that we've a fix for this in the form of KEY_WAKEUP
it is time to properly fix this instead of doing userspace
kludges.

> What about
> holding power button for too long so that normal reporting "catches" the
> pressed state?

The key-down event is send as KEY_WAKEUP instead,
so userspace sees KEY_WAKEUP pressed not KEY_POWER.

> Kernel reports hardware events, interpreting them and applying certain
> policies is task for userspace.

And atm it is actually doing a shitty job of reporting
hwevents because there is no way for userspace to be able
to differentiate between:

1. User pressed power-button to wakeup system
2. User pressed power-button after resume to do
   power-button-action (e.g. suspend system)

Even though *the kernel* does *know* the difference.

So the suggested change actually makes the kernel
do its job of reporting hw-events better by making
the reporting more accurate.

ATM if I resume say a tablet with GNOME and then
change my mind and press the power button within
3 seconds of resume to suspend it again the second
power-button press will outright be ignored

The current userspace workaround is racy like this,
again the whole workaround in GNOME is just an ugly
kludge which I did back then because we couldn't
agree on a better way to deal with this in the kernel /
because just suppressing sending KEY_POWER would break
Android.

The suggested use of KEY_WAKEUP is lightyears better
then doing ignore KEY_POWER events for xx seconds
after resume which is simply always going to be racy
and always was just an ugly hack / never was
a great solution.

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Andy Shevchenko]

On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
> On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
> > On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
> >> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
> >>> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:

...

> >>> Hans, you have a lot of experience in the GNOME community.  Your thoughts?
> >>
> >> I guess it would be good to fix this in the kernel, sending
> >> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
> >> we are going through the special wakeup path in gpio_keys.
> >>
> >> When this was discussed quite a while ago the ACPI button
> >> driver simply did not send any event at all on wkaeup
> >> by ACPI power-button. Know that it does send an event
> >> it would be good to mimic this, at least when the gpio_key
> >> devices where instantiated by soc_button_array.
> >>
> >> So maybe add a new field to struct gpio_keys_button
> >> called wakeup_code and when that is not 0 use that
> >> instead of the plain "code" member on wakeups ?
> >>
> >> That would keep the gpio_keys code generic while
> >> allowing to mimic the ACPI button behavior.
> >>
> >> And then set wakeup_code to KEY_WAKEUP for
> >> the power-button in soc_button_array.
> >>
> >> To me this sounds better then trying to fix all userspace
> >> code which does something on KEY_POWER of which there
> >> is quite a lot.
> >>
> >> The special GNOME power-button handling was always
> >> a workaround because last time a kernel fix was
> >> nacked. But now with the KEY_WAKEUP done by the ACPI
> >> button code it looks like we do have a good way
> >> to fix this in the kernel, so that would be better
> >> IMHO.
> >>
> >> Dmitry, what do you think of adding a wakeup_code
> >> field to struct gpio_keys_button and let the code
> >> creating the gpio_keys_button decide if a different
> >> code should be used on wakeup or not ?
> > 
> > And what is the plan on dealing with all other drivers that emit
> > KEY_POWER?
> 
> There actually aren't that many that I'm aware of.
> 
> Note that this gpio_keys KEY_POWER evdev event generation
> on resume issue goes way back until the last time we had
> this conversation and it still has not really been fixed.
> 
> And I've not seen any bug-reports about the same problem
> with any other drivers.
> 
> > What about acpi button behavior when using S0ix?
> 
> AFAIK it is the same as with S3, at least it is not
> causing any issues. I've never seen the ACPI button code
> cause re-suspend immediately on wakeup by what for all
> intends and purposes is a spurious KEY_POWER event.
> 
> Last time we discussed this I wasn't really happy with
> the outcome of the discussion but I just went for it
> because of Android's reliance on the event and we
> lacked a better plan.
> 
> Now that we've a fix for this in the form of KEY_WAKEUP
> it is time to properly fix this instead of doing userspace
> kludges.
> 
> > What about
> > holding power button for too long so that normal reporting "catches" the
> > pressed state?
> 
> The key-down event is send as KEY_WAKEUP instead,
> so userspace sees KEY_WAKEUP pressed not KEY_POWER.
> 
> > Kernel reports hardware events, interpreting them and applying certain
> > policies is task for userspace.
> 
> And atm it is actually doing a shitty job of reporting
> hwevents because there is no way for userspace to be able
> to differentiate between:
> 
> 1. User pressed power-button to wakeup system
> 2. User pressed power-button after resume to do
>    power-button-action (e.g. suspend system)
> 
> Even though *the kernel* does *know* the difference.
> 
> So the suggested change actually makes the kernel
> do its job of reporting hw-events better by making
> the reporting more accurate.
> 
> ATM if I resume say a tablet with GNOME and then
> change my mind and press the power button within
> 3 seconds of resume to suspend it again the second
> power-button press will outright be ignored
> 
> The current userspace workaround is racy like this,
> again the whole workaround in GNOME is just an ugly
> kludge which I did back then because we couldn't
> agree on a better way to deal with this in the kernel /
> because just suppressing sending KEY_POWER would break
> Android.
> 
> The suggested use of KEY_WAKEUP is lightyears better
> then doing ignore KEY_POWER events for xx seconds
> after resume which is simply always going to be racy
> and always was just an ugly hack / never was
> a great solution.

My take away from this discussion that in a sleep state the power button
(or actually any wakeup source) should tell userspace "hey, we want to wake
up". It doesn't tell "hey, we want to wake to power off".
This sounds good to me as a user. Yes, if laptop is sleeping we need to wake
it up to continue, the power off is just a next step of this flow.
The  expected hot topic here is the longer presses of power button, but I think
if we have a timer and tell after say 3 second that the K_WUP was up and K_PW
is down is not a solution, it will be always flaky.

Just my 2c.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 27-Jun-25 6:12 PM, Andy Shevchenko wrote:
> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>> On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
>>> On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
>>>> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
>>>>> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
> 
> ...
> 
>>>>> Hans, you have a lot of experience in the GNOME community.  Your thoughts?
>>>>
>>>> I guess it would be good to fix this in the kernel, sending
>>>> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
>>>> we are going through the special wakeup path in gpio_keys.
>>>>
>>>> When this was discussed quite a while ago the ACPI button
>>>> driver simply did not send any event at all on wkaeup
>>>> by ACPI power-button. Know that it does send an event
>>>> it would be good to mimic this, at least when the gpio_key
>>>> devices where instantiated by soc_button_array.
>>>>
>>>> So maybe add a new field to struct gpio_keys_button
>>>> called wakeup_code and when that is not 0 use that
>>>> instead of the plain "code" member on wakeups ?
>>>>
>>>> That would keep the gpio_keys code generic while
>>>> allowing to mimic the ACPI button behavior.
>>>>
>>>> And then set wakeup_code to KEY_WAKEUP for
>>>> the power-button in soc_button_array.
>>>>
>>>> To me this sounds better then trying to fix all userspace
>>>> code which does something on KEY_POWER of which there
>>>> is quite a lot.
>>>>
>>>> The special GNOME power-button handling was always
>>>> a workaround because last time a kernel fix was
>>>> nacked. But now with the KEY_WAKEUP done by the ACPI
>>>> button code it looks like we do have a good way
>>>> to fix this in the kernel, so that would be better
>>>> IMHO.
>>>>
>>>> Dmitry, what do you think of adding a wakeup_code
>>>> field to struct gpio_keys_button and let the code
>>>> creating the gpio_keys_button decide if a different
>>>> code should be used on wakeup or not ?
>>>
>>> And what is the plan on dealing with all other drivers that emit
>>> KEY_POWER?
>>
>> There actually aren't that many that I'm aware of.
>>
>> Note that this gpio_keys KEY_POWER evdev event generation
>> on resume issue goes way back until the last time we had
>> this conversation and it still has not really been fixed.
>>
>> And I've not seen any bug-reports about the same problem
>> with any other drivers.
>>
>>> What about acpi button behavior when using S0ix?
>>
>> AFAIK it is the same as with S3, at least it is not
>> causing any issues. I've never seen the ACPI button code
>> cause re-suspend immediately on wakeup by what for all
>> intends and purposes is a spurious KEY_POWER event.
>>
>> Last time we discussed this I wasn't really happy with
>> the outcome of the discussion but I just went for it
>> because of Android's reliance on the event and we
>> lacked a better plan.
>>
>> Now that we've a fix for this in the form of KEY_WAKEUP
>> it is time to properly fix this instead of doing userspace
>> kludges.
>>
>>> What about
>>> holding power button for too long so that normal reporting "catches" the
>>> pressed state?
>>
>> The key-down event is send as KEY_WAKEUP instead,
>> so userspace sees KEY_WAKEUP pressed not KEY_POWER.
>>
>>> Kernel reports hardware events, interpreting them and applying certain
>>> policies is task for userspace.
>>
>> And atm it is actually doing a shitty job of reporting
>> hwevents because there is no way for userspace to be able
>> to differentiate between:
>>
>> 1. User pressed power-button to wakeup system
>> 2. User pressed power-button after resume to do
>>    power-button-action (e.g. suspend system)
>>
>> Even though *the kernel* does *know* the difference.
>>
>> So the suggested change actually makes the kernel
>> do its job of reporting hw-events better by making
>> the reporting more accurate.
>>
>> ATM if I resume say a tablet with GNOME and then
>> change my mind and press the power button within
>> 3 seconds of resume to suspend it again the second
>> power-button press will outright be ignored
>>
>> The current userspace workaround is racy like this,
>> again the whole workaround in GNOME is just an ugly
>> kludge which I did back then because we couldn't
>> agree on a better way to deal with this in the kernel /
>> because just suppressing sending KEY_POWER would break
>> Android.
>>
>> The suggested use of KEY_WAKEUP is lightyears better
>> then doing ignore KEY_POWER events for xx seconds
>> after resume which is simply always going to be racy
>> and always was just an ugly hack / never was
>> a great solution.
> 
> My take away from this discussion that in a sleep state the power button
> (or actually any wakeup source) should tell userspace "hey, we want to wake
> up". It doesn't tell "hey, we want to wake to power off".
> This sounds good to me as a user. Yes, if laptop is sleeping we need to wake
> it up to continue, the power off is just a next step of this flow.

Exactly.

These are really 2 different events and the problem
is that atm the kernel reports this as one and
the same event type. It really is as simple as this.

Note that I'm not proposing on making this change
something which all gpio_keys drivers will get
automatically.

My idea of adding a wakeup_code field to
struct gpio_keys_button allows individual gpio_keys
users to opt-in to the behavior of
sending KEY_SOMETHINGELSE on wakeup-by-gpio-button-press.

Since soc_button_array is only used on x86/ACPI platforms
and since by far the most x86/ACPI platforms use
the ACPI button code for the power-button, which already
behaves this way I do not expect any userspace problems
from doing such a change in soc_button_array because
if that were a problem then we would already have bug
reports because of the ACPI button code's behavior.

> The  expected hot topic here is the longer presses of power button, but I think
> if we have a timer and tell after say 3 second that the K_WUP was up and K_PW
> is down is not a solution, it will be always flaky.

If users do a long power-button press on x86 they are
*always* trying to do a forced power-off and after 4s
(or longer in some special cases) the hw itself will
do such a forced poweroff. So I do not believe that
we need to worry about long presses since those
have a very specific meaning on x86/ACPI platforms.

Also if there is a long press userspace will simply
see KEY_WAKEUP never getting released, which is
actually an accurate representation of things,
the user woke up the system through the power-button
and never released ir.

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Fri, Jun 27, 2025 at 07:59:56PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 27-Jun-25 6:12 PM, Andy Shevchenko wrote:
> > On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
> >> On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
> >>> On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
> >>>> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
> >>>>> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
> > 
> > ...
> > 
> >>>>> Hans, you have a lot of experience in the GNOME community.  Your thoughts?
> >>>>
> >>>> I guess it would be good to fix this in the kernel, sending
> >>>> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
> >>>> we are going through the special wakeup path in gpio_keys.
> >>>>
> >>>> When this was discussed quite a while ago the ACPI button
> >>>> driver simply did not send any event at all on wkaeup
> >>>> by ACPI power-button. Know that it does send an event
> >>>> it would be good to mimic this, at least when the gpio_key
> >>>> devices where instantiated by soc_button_array.
> >>>>
> >>>> So maybe add a new field to struct gpio_keys_button
> >>>> called wakeup_code and when that is not 0 use that
> >>>> instead of the plain "code" member on wakeups ?
> >>>>
> >>>> That would keep the gpio_keys code generic while
> >>>> allowing to mimic the ACPI button behavior.
> >>>>
> >>>> And then set wakeup_code to KEY_WAKEUP for
> >>>> the power-button in soc_button_array.
> >>>>
> >>>> To me this sounds better then trying to fix all userspace
> >>>> code which does something on KEY_POWER of which there
> >>>> is quite a lot.
> >>>>
> >>>> The special GNOME power-button handling was always
> >>>> a workaround because last time a kernel fix was
> >>>> nacked. But now with the KEY_WAKEUP done by the ACPI
> >>>> button code it looks like we do have a good way
> >>>> to fix this in the kernel, so that would be better
> >>>> IMHO.
> >>>>
> >>>> Dmitry, what do you think of adding a wakeup_code
> >>>> field to struct gpio_keys_button and let the code
> >>>> creating the gpio_keys_button decide if a different
> >>>> code should be used on wakeup or not ?
> >>>
> >>> And what is the plan on dealing with all other drivers that emit
> >>> KEY_POWER?
> >>
> >> There actually aren't that many that I'm aware of.
> >>
> >> Note that this gpio_keys KEY_POWER evdev event generation
> >> on resume issue goes way back until the last time we had
> >> this conversation and it still has not really been fixed.
> >>
> >> And I've not seen any bug-reports about the same problem
> >> with any other drivers.
> >>
> >>> What about acpi button behavior when using S0ix?
> >>
> >> AFAIK it is the same as with S3, at least it is not
> >> causing any issues. I've never seen the ACPI button code
> >> cause re-suspend immediately on wakeup by what for all
> >> intends and purposes is a spurious KEY_POWER event.
> >>
> >> Last time we discussed this I wasn't really happy with
> >> the outcome of the discussion but I just went for it
> >> because of Android's reliance on the event and we
> >> lacked a better plan.
> >>
> >> Now that we've a fix for this in the form of KEY_WAKEUP
> >> it is time to properly fix this instead of doing userspace
> >> kludges.
> >>
> >>> What about
> >>> holding power button for too long so that normal reporting "catches" the
> >>> pressed state?
> >>
> >> The key-down event is send as KEY_WAKEUP instead,
> >> so userspace sees KEY_WAKEUP pressed not KEY_POWER.
> >>
> >>> Kernel reports hardware events, interpreting them and applying certain
> >>> policies is task for userspace.
> >>
> >> And atm it is actually doing a shitty job of reporting
> >> hwevents because there is no way for userspace to be able
> >> to differentiate between:
> >>
> >> 1. User pressed power-button to wakeup system
> >> 2. User pressed power-button after resume to do
> >>    power-button-action (e.g. suspend system)
> >>
> >> Even though *the kernel* does *know* the difference.
> >>
> >> So the suggested change actually makes the kernel
> >> do its job of reporting hw-events better by making
> >> the reporting more accurate.
> >>
> >> ATM if I resume say a tablet with GNOME and then
> >> change my mind and press the power button within
> >> 3 seconds of resume to suspend it again the second
> >> power-button press will outright be ignored
> >>
> >> The current userspace workaround is racy like this,
> >> again the whole workaround in GNOME is just an ugly
> >> kludge which I did back then because we couldn't
> >> agree on a better way to deal with this in the kernel /
> >> because just suppressing sending KEY_POWER would break
> >> Android.
> >>
> >> The suggested use of KEY_WAKEUP is lightyears better
> >> then doing ignore KEY_POWER events for xx seconds
> >> after resume which is simply always going to be racy
> >> and always was just an ugly hack / never was
> >> a great solution.
> > 
> > My take away from this discussion that in a sleep state the power button
> > (or actually any wakeup source) should tell userspace "hey, we want to wake
> > up". It doesn't tell "hey, we want to wake to power off".
> > This sounds good to me as a user. Yes, if laptop is sleeping we need to wake
> > it up to continue, the power off is just a next step of this flow.
> 
> Exactly.
> 
> These are really 2 different events and the problem
> is that atm the kernel reports this as one and
> the same event type. It really is as simple as this.
> 
> Note that I'm not proposing on making this change
> something which all gpio_keys drivers will get
> automatically.
> 
> My idea of adding a wakeup_code field to
> struct gpio_keys_button allows individual gpio_keys
> users to opt-in to the behavior of
> sending KEY_SOMETHINGELSE on wakeup-by-gpio-button-press.
> 
> Since soc_button_array is only used on x86/ACPI platforms
> and since by far the most x86/ACPI platforms use
> the ACPI button code for the power-button, which already
> behaves this way I do not expect any userspace problems
> from doing such a change in soc_button_array because
> if that were a problem then we would already have bug
> reports because of the ACPI button code's behavior.

ACPI/x86 system does not necessarily mean something running generic
Linux distribution, and it may have different set of devices and
drivers. Or it may even use soc_button_array and still not want this
behavior.

> 
> > The  expected hot topic here is the longer presses of power button, but I think
> > if we have a timer and tell after say 3 second that the K_WUP was up and K_PW
> > is down is not a solution, it will be always flaky.
> 
> If users do a long power-button press on x86 they are
> *always* trying to do a forced power-off and after 4s
> (or longer in some special cases) the hw itself will
> do such a forced poweroff. So I do not believe that
> we need to worry about long presses since those
> have a very specific meaning on x86/ACPI platforms.

You only consider one environment: Gnome. There are other alternatives.
For example on Chrome OS pressing power button for longish time (but
shorter that forced power off) will result in orderly system shutdown.

> 
> Also if there is a long press userspace will simply
> see KEY_WAKEUP never getting released, which is
> actually an accurate representation of things,
> the user woke up the system through the power-button
> and never released ir.

Yeah, it will break everyone who is monitoring user activity.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
> Hi Dmitry,
> 
> On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
> > On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
> >>> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
> >>>> On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
> >>>>> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
> >>>>>> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
> >>>>>>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
> >>>>>>> <dmitry.torokhov@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> >>>>>>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> >>>>>>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> >>>>>>>>>>>> Hi,
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> >>>>>>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> >>>>>>>> [...]
> >>>>>>>>>>>>>> I want to note this driver works quite differently than how ACPI power
> >>>>>>>>>>>>>> button does.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> >>>>>>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> >>>>>>>>>>>>>> patch was modeling).
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> >>>>>>>>>>>>>> [1]
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
> >>>>>>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> >>>>>>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> >>>>>>>>>>>>> multiple other platforms.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
> >>>>>>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
> >>>>>>>>>>>
> >>>>>>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
> >>>>>>>>>>> it does not seem to be captured in the email discussions or in the patch
> >>>>>>>>>>> description.
> >>>>>>>>>>
> >>>>>>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
> >>>>>>>>>> power-button issue. GNOME has a workaround for this, but I assume that
> >>>>>>>>>> some userspace desktop environments are still going to have a problem
> >>>>>>>>>> with this.
> >>>>>>>>>
> >>>>>>>>> It was done for this reason IIRC, but it should have been documented
> >>>>>>>>> more thoroughly.
> >>>>>>>>
> >>>>>>>> I assert that it should not have been done and instead dealt with in
> >>>>>>>> userspace. There are numerous drivers in the kernel emitting
> >>>>>>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
> >>>>>>>> what keys to process and when.
> >>>>>>>
> >>>>>>> Please see my last message in this thread (just sent) and see the
> >>>>>>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
> >>>>>>> events").
> >>>>>>>
> >>>>>>> This appears to be about cases when no event would be signaled to user
> >>>>>>> space at all (power button wakeup from ACPI S3).
> >>>>>>
> >>>>>> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
> >>>>>> button, right? So we can not send the "right" event code and use
> >>>>>> "neutral" KEY_WAKEUP for both. Is this right?
> >>>>>>
> >>>>>> Thanks.
> >>>>>>
> >>>>>
> >>>>> I did some more experiments with this affected system that started this
> >>>>> thread (which uses s2idle).
> >>>>>
> >>>>> I only applied patch 3 in this series to help the debounce behavior and
> >>>>> figure out impacts from patch 4 with existing Linux userspace.
> >>>>>
> >>>>> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
> >>>>> the GNOME workaround mitigates this problem and no visible impact.
> >>>>>
> >>>>> If I suspend by hand using the kernel interface and then press power button
> >>>>> to wake:
> >>>>>
> >>>>> # echo mem | sudo tee /sys/power/state:
> >>>>>
> >>>>> * When GNOME is running:
> >>>>> I get the shutdown popup and it eventually shuts down.
> >>>>>
> >>>>> * When GNOME isn't running (just on a VT):
> >>>>> System shuts down.
> >>>>
> >>>> For the latter you may want to raise an issue with systemd, and for the
> >>>> former I guess it is being too clever and does not activate the
> >>>> workaround if suspend was not initiated by it? I think Gnome is being
> >>>> too careful.
> >>>>
> >>>> Thanks.
> >>>>
> >>>
> >>> Sure I could file bugs with both the projects.
> >>>
> >>> But before I do if all userspace needs to account for this with a series of workarounds at resume time, you still think that is that really the best way forward?
> >>>
> >>> Hans, you have a lot of experience in the GNOME community.  Your thoughts?
> >>
> >> I guess it would be good to fix this in the kernel, sending
> >> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
> >> we are going through the special wakeup path in gpio_keys.
> >>
> >> When this was discussed quite a while ago the ACPI button
> >> driver simply did not send any event at all on wkaeup
> >> by ACPI power-button. Know that it does send an event
> >> it would be good to mimic this, at least when the gpio_key
> >> devices where instantiated by soc_button_array.
> >>
> >> So maybe add a new field to struct gpio_keys_button
> >> called wakeup_code and when that is not 0 use that
> >> instead of the plain "code" member on wakeups ?
> >>
> >> That would keep the gpio_keys code generic while
> >> allowing to mimic the ACPI button behavior.
> >>
> >> And then set wakeup_code to KEY_WAKEUP for
> >> the power-button in soc_button_array.
> >>
> >> To me this sounds better then trying to fix all userspace
> >> code which does something on KEY_POWER of which there
> >> is quite a lot.
> >>
> >> The special GNOME power-button handling was always
> >> a workaround because last time a kernel fix was
> >> nacked. But now with the KEY_WAKEUP done by the ACPI
> >> button code it looks like we do have a good way
> >> to fix this in the kernel, so that would be better
> >> IMHO.
> >>
> >> Dmitry, what do you think of adding a wakeup_code
> >> field to struct gpio_keys_button and let the code
> >> creating the gpio_keys_button decide if a different
> >> code should be used on wakeup or not ?
> > 
> > And what is the plan on dealing with all other drivers that emit
> > KEY_POWER?
> 
> There actually aren't that many that I'm aware of.

dtor@dtor-ws:~/kernel/work $ git grep -l KEY_POWER -- drivers/{input,hid,platform}/ | wc -l
51

Additionally:

dtor@dtor-ws:~/kernel/work $ git grep -l KEY_POWER -- arch/arm*/boot/dts | wc -l
254

> 
> Note that this gpio_keys KEY_POWER evdev event generation
> on resume issue goes way back until the last time we had
> this conversation and it still has not really been fixed.

I am sorry to hear that. I am not involved in Gnome so I am not sure why
it takes so long, I guess not tablets or detachables are a minority of
deployments so it is not prioritized? Android seems to have a handle on
this as does Chrome OS...

> 
> And I've not seen any bug-reports about the same problem
> with any other drivers.

I guess you will next want to patch cros_ec_keyb in case someone uses
generic distro with x86 Chromebooks, and then matrix keypad, and then
all drivers that are used outside of x86.

> 
> > What about acpi button behavior when using S0ix?
> 
> AFAIK it is the same as with S3, at least it is not
> causing any issues. I've never seen the ACPI button code
> cause re-suspend immediately on wakeup by what for all
> intends and purposes is a spurious KEY_POWER event.

Rafael has made assertion that in S3 it is impossible to differentiate
whether the button is power button or not. But looking at this I do not
think it is correct assertion. "Notify Wake" is sent for a given
button. Or maybe I misunderstood and he meant that the *state* if button
is not available in "Notify Wake"?

In fact, I think login in the ACPI button is pretty broken and needs to
be undone/reverted:

1. The driver sends KEY_WAKEUP events on every resume. It does not
matter if wakeup is done by pressing power button, wake or lan packet,
or an act of God, it will send KEY_WAKEUP as part of the button *device*
resume.

2. There is a patch from Mario (a8605b0ed187) suppressing sending
KEY_POWER as part of "normal" wakeup handling, pretty much the same as
what he and you are proposing to do in gpio-keys (and eventually in
every driver keyboard or button driver in the kernel). This means we no
longer can tell if wakeup is done by power button or sleep button (on
systems with dual-button models, see ACPI 4.8.3.1).

To me it seems we are piling workarounds on top of workarounds. We
should report either KEY_POWER or KEY_SLEEP in the notify event for both
"Notify Wake" and "Notify State", and let userspace make decision on how
to handle this best.

> 
> Last time we discussed this I wasn't really happy with
> the outcome of the discussion but I just went for it
> because of Android's reliance on the event and we
> lacked a better plan.
> 
> Now that we've a fix for this in the form of KEY_WAKEUP
> it is time to properly fix this instead of doing userspace
> kludges.

I do not understand why you call KEY_WAKEUP being "proper". Especially
with gpio-keys you do not know if wakeup is really due to this
particular button being pressed, or the system woke up for some other
reason and user also happened to have the button pressed. ACPI button is
really an outlier here because firmware interface defines a dedicated
event for waking up.

> 
> > What about
> > holding power button for too long so that normal reporting "catches" the
> > pressed state?
> 
> The key-down event is send as KEY_WAKEUP instead,
> so userspace sees KEY_WAKEUP pressed not KEY_POWER.

This assumes the driver does not report state between press and release,
which is not guaranteed. Or we now require drivers to keep track whether
release for KEY_WAKEUP has been done and that it can return to reporting
KEY_POWER.

> 
> > Kernel reports hardware events, interpreting them and applying certain
> > policies is task for userspace.
> 
> And atm it is actually doing a shitty job of reporting
> hwevents because there is no way for userspace to be able
> to differentiate between:
> 
> 1. User pressed power-button to wakeup system
> 2. User pressed power-button after resume to do
>    power-button-action (e.g. suspend system)
> 
> Even though *the kernel* does *know* the difference.

Does it really? Aside of ACPI button you have no idea if button press
coincided with some other event waking up the system. You can guess, but
that is it, a guess.

> 
> So the suggested change actually makes the kernel
> do its job of reporting hw-events better by making
> the reporting more accurate.

No, you are making assumptions, but instead of doing it in userspace
where you can give user choice to adjust the behavior you are
hard-coding it.

> 
> ATM if I resume say a tablet with GNOME and then
> change my mind and press the power button within
> 3 seconds of resume to suspend it again the second
> power-button press will outright be ignored

Please fix Gnome. It is possible to handle rapid power button presses
(because other OSed/environments do that).

> 
> The current userspace workaround is racy like this,
> again the whole workaround in GNOME is just an ugly
> kludge which I did back then because we couldn't
> agree on a better way to deal with this in the kernel /
> because just suppressing sending KEY_POWER would break
> Android.
> 
> The suggested use of KEY_WAKEUP is lightyears better
> then doing ignore KEY_POWER events for xx seconds
> after resume which is simply always going to be racy
> and always was just an ugly hack / never was
> a great solution.

My opinion is that KEY_WAKEUP is not much better and we are actually
losing information if we try to implement that (again, because you do
not know if button press was the real cause of wakeup or it simply
coincided with something else).

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>> Hi Dmitry,
>>
>> On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
>>> On Fri, Jun 27, 2025 at 04:14:38PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 27-Jun-25 4:06 PM, Mario Limonciello wrote:
>>>>> On 6/26/2025 11:56 PM, Dmitry Torokhov wrote:
>>>>>> On Thu, Jun 26, 2025 at 05:21:35PM -0500, Mario Limonciello wrote:
>>>>>>> On 6/26/2025 2:40 PM, Dmitry Torokhov wrote:
>>>>>>>> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
>>>>>>>>> On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
>>>>>>>>> <dmitry.torokhov@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
>>>>>>>>>>> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 26-Jun-25 21:14, Dmitry Torokhov wrote:
>>>>>>>>>>>>> On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
>>>>>>>>>>>>>>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
>>>>>>>>>> [...]
>>>>>>>>>>>>>>>> I want to note this driver works quite differently than how ACPI power
>>>>>>>>>>>>>>>> button does.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
>>>>>>>>>>>>>>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
>>>>>>>>>>>>>>>> patch was modeling).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If you check acpi_button_resume() you will see that the events are sent
>>>>>>>>>>>>>>> from there. Except that for some reason they chose to use KEY_WAKEUP and
>>>>>>>>>>>>>>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
>>>>>>>>>>>>>>> multiple other platforms.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Interesting, but the ACPI button code presumably only does this on resume
>>>>>>>>>>>>>> for a normal press while the system is awake it does use KEY_POWER, right ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes. It is unclear to me why they chose to mangle the event on wakeup,
>>>>>>>>>>>>> it does not seem to be captured in the email discussions or in the patch
>>>>>>>>>>>>> description.
>>>>>>>>>>>>
>>>>>>>>>>>> I assume they did this to avoid the immediate re-suspend on wakeup by
>>>>>>>>>>>> power-button issue. GNOME has a workaround for this, but I assume that
>>>>>>>>>>>> some userspace desktop environments are still going to have a problem
>>>>>>>>>>>> with this.
>>>>>>>>>>>
>>>>>>>>>>> It was done for this reason IIRC, but it should have been documented
>>>>>>>>>>> more thoroughly.
>>>>>>>>>>
>>>>>>>>>> I assert that it should not have been done and instead dealt with in
>>>>>>>>>> userspace. There are numerous drivers in the kernel emitting
>>>>>>>>>> KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
>>>>>>>>>> what keys to process and when.
>>>>>>>>>
>>>>>>>>> Please see my last message in this thread (just sent) and see the
>>>>>>>>> changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
>>>>>>>>> events").
>>>>>>>>>
>>>>>>>>> This appears to be about cases when no event would be signaled to user
>>>>>>>>> space at all (power button wakeup from ACPI S3).
>>>>>>>>
>>>>>>>> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
>>>>>>>> button, right? So we can not send the "right" event code and use
>>>>>>>> "neutral" KEY_WAKEUP for both. Is this right?
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>
>>>>>>> I did some more experiments with this affected system that started this
>>>>>>> thread (which uses s2idle).
>>>>>>>
>>>>>>> I only applied patch 3 in this series to help the debounce behavior and
>>>>>>> figure out impacts from patch 4 with existing Linux userspace.
>>>>>>>
>>>>>>> If suspended using systemd in GNOME (click the GUI button) on Ubuntu 24.04
>>>>>>> the GNOME workaround mitigates this problem and no visible impact.
>>>>>>>
>>>>>>> If I suspend by hand using the kernel interface and then press power button
>>>>>>> to wake:
>>>>>>>
>>>>>>> # echo mem | sudo tee /sys/power/state:
>>>>>>>
>>>>>>> * When GNOME is running:
>>>>>>> I get the shutdown popup and it eventually shuts down.
>>>>>>>
>>>>>>> * When GNOME isn't running (just on a VT):
>>>>>>> System shuts down.
>>>>>>
>>>>>> For the latter you may want to raise an issue with systemd, and for the
>>>>>> former I guess it is being too clever and does not activate the
>>>>>> workaround if suspend was not initiated by it? I think Gnome is being
>>>>>> too careful.
>>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>
>>>>> Sure I could file bugs with both the projects.
>>>>>
>>>>> But before I do if all userspace needs to account for this with a series of workarounds at resume time, you still think that is that really the best way forward?
>>>>>
>>>>> Hans, you have a lot of experience in the GNOME community.  Your thoughts?
>>>>
>>>> I guess it would be good to fix this in the kernel, sending
>>>> KEY_WAKEUP from gpio_key when the event is KEY_POWER and
>>>> we are going through the special wakeup path in gpio_keys.
>>>>
>>>> When this was discussed quite a while ago the ACPI button
>>>> driver simply did not send any event at all on wkaeup
>>>> by ACPI power-button. Know that it does send an event
>>>> it would be good to mimic this, at least when the gpio_key
>>>> devices where instantiated by soc_button_array.
>>>>
>>>> So maybe add a new field to struct gpio_keys_button
>>>> called wakeup_code and when that is not 0 use that
>>>> instead of the plain "code" member on wakeups ?
>>>>
>>>> That would keep the gpio_keys code generic while
>>>> allowing to mimic the ACPI button behavior.
>>>>
>>>> And then set wakeup_code to KEY_WAKEUP for
>>>> the power-button in soc_button_array.
>>>>
>>>> To me this sounds better then trying to fix all userspace
>>>> code which does something on KEY_POWER of which there
>>>> is quite a lot.
>>>>
>>>> The special GNOME power-button handling was always
>>>> a workaround because last time a kernel fix was
>>>> nacked. But now with the KEY_WAKEUP done by the ACPI
>>>> button code it looks like we do have a good way
>>>> to fix this in the kernel, so that would be better
>>>> IMHO.
>>>>
>>>> Dmitry, what do you think of adding a wakeup_code
>>>> field to struct gpio_keys_button and let the code
>>>> creating the gpio_keys_button decide if a different
>>>> code should be used on wakeup or not ?
>>>
>>> And what is the plan on dealing with all other drivers that emit
>>> KEY_POWER?
>>
>> There actually aren't that many that I'm aware of.
> 
> dtor@dtor-ws:~/kernel/work $ git grep -l KEY_POWER -- drivers/{input,hid,platform}/ | wc -l
> 51
> 
> Additionally:
> 
> dtor@dtor-ws:~/kernel/work $ git grep -l KEY_POWER -- arch/arm*/boot/dts | wc -l
> 254
> 
>>
>> Note that this gpio_keys KEY_POWER evdev event generation
>> on resume issue goes way back until the last time we had
>> this conversation and it still has not really been fixed.
> 
> I am sorry to hear that. I am not involved in Gnome so I am not sure why
> it takes so long, I guess not tablets or detachables are a minority of
> deployments so it is not prioritized? Android seems to have a handle on
> this as does Chrome OS...
> 
>>
>> And I've not seen any bug-reports about the same problem
>> with any other drivers.
> 
> I guess you will next want to patch cros_ec_keyb in case someone uses
> generic distro with x86 Chromebooks, and then matrix keypad, and then
> all drivers that are used outside of x86.
> 
>>
>>> What about acpi button behavior when using S0ix?
>>
>> AFAIK it is the same as with S3, at least it is not
>> causing any issues. I've never seen the ACPI button code
>> cause re-suspend immediately on wakeup by what for all
>> intends and purposes is a spurious KEY_POWER event.
> 
> Rafael has made assertion that in S3 it is impossible to differentiate
> whether the button is power button or not. But looking at this I do not
> think it is correct assertion. "Notify Wake" is sent for a given
> button. Or maybe I misunderstood and he meant that the *state* if button
> is not available in "Notify Wake"?
> 
> In fact, I think login in the ACPI button is pretty broken and needs to
> be undone/reverted:
> 
> 1. The driver sends KEY_WAKEUP events on every resume. It does not
> matter if wakeup is done by pressing power button, wake or lan packet,
> or an act of God, it will send KEY_WAKEUP as part of the button *device*
> resume.

This seems like a real bug.  IMO you're right it should only be sending 
it when the key was received.

> 
> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
> what he and you are proposing to do in gpio-keys (and eventually in
> every driver keyboard or button driver in the kernel). This means we no
> longer can tell if wakeup is done by power button or sleep button (on
> systems with dual-button models, see ACPI 4.8.3.1).

Actually a8605b0ed187 was about a runtime regression not a suspend 
regression.  I didn't change anything with sending KEY_POWER during 
wakeup handling.

> 
> To me it seems we are piling workarounds on top of workarounds. We
> should report either KEY_POWER or KEY_SLEEP in the notify event for both
> "Notify Wake" and "Notify State", and let userspace make decision on how
> to handle this best.
> 
>>
>> Last time we discussed this I wasn't really happy with
>> the outcome of the discussion but I just went for it
>> because of Android's reliance on the event and we
>> lacked a better plan.
>>
>> Now that we've a fix for this in the form of KEY_WAKEUP
>> it is time to properly fix this instead of doing userspace
>> kludges.
> 
> I do not understand why you call KEY_WAKEUP being "proper". Especially
> with gpio-keys you do not know if wakeup is really due to this
> particular button being pressed, or the system woke up for some other
> reason and user also happened to have the button pressed. ACPI button is
> really an outlier here because firmware interface defines a dedicated
> event for waking up.
> 
>>
>>> What about
>>> holding power button for too long so that normal reporting "catches" the
>>> pressed state?
>>
>> The key-down event is send as KEY_WAKEUP instead,
>> so userspace sees KEY_WAKEUP pressed not KEY_POWER.
> 
> This assumes the driver does not report state between press and release,
> which is not guaranteed. Or we now require drivers to keep track whether
> release for KEY_WAKEUP has been done and that it can return to reporting
> KEY_POWER.
> 
>>
>>> Kernel reports hardware events, interpreting them and applying certain
>>> policies is task for userspace.
>>
>> And atm it is actually doing a shitty job of reporting
>> hwevents because there is no way for userspace to be able
>> to differentiate between:
>>
>> 1. User pressed power-button to wakeup system
>> 2. User pressed power-button after resume to do
>>     power-button-action (e.g. suspend system)
>>
>> Even though *the kernel* does *know* the difference.
> 
> Does it really? Aside of ACPI button you have no idea if button press
> coincided with some other event waking up the system. You can guess, but
> that is it, a guess.
> 
>>
>> So the suggested change actually makes the kernel
>> do its job of reporting hw-events better by making
>> the reporting more accurate.
> 
> No, you are making assumptions, but instead of doing it in userspace
> where you can give user choice to adjust the behavior you are
> hard-coding it.
> 
>>
>> ATM if I resume say a tablet with GNOME and then
>> change my mind and press the power button within
>> 3 seconds of resume to suspend it again the second
>> power-button press will outright be ignored
> 
> Please fix Gnome. It is possible to handle rapid power button presses
> (because other OSed/environments do that).
> 
>>
>> The current userspace workaround is racy like this,
>> again the whole workaround in GNOME is just an ugly
>> kludge which I did back then because we couldn't
>> agree on a better way to deal with this in the kernel /
>> because just suppressing sending KEY_POWER would break
>> Android.
>>
>> The suggested use of KEY_WAKEUP is lightyears better
>> then doing ignore KEY_POWER events for xx seconds
>> after resume which is simply always going to be racy
>> and always was just an ugly hack / never was
>> a great solution.
> 
> My opinion is that KEY_WAKEUP is not much better and we are actually
> losing information if we try to implement that (again, because you do
> not know if button press was the real cause of wakeup or it simply
> coincided with something else).
> 
> Thanks.
> 

FTR I did test Hans suggestion and it does work effectively (code below).

diff --git a/drivers/input/keyboard/gpio_keys.c 
b/drivers/input/keyboard/gpio_keys.c
index f9db86da0818b..3bc8c95e9943b 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void 
*dev_id)
                          * already released by the time we got interrupt
                          * handler to run.
                          */
-                       input_report_key(bdata->input, button->code, 1);
+                       input_report_key(bdata->input, *bdata->code, 1);
+                       input_sync(bdata->input);
                 }
         }

@@ -644,6 +645,14 @@ static int gpio_keys_setup_key(struct 
platform_device *pdev,
         bdata->code = &ddata->keymap[idx];
         *bdata->code = button->code;
         input_set_capability(input, button->type ?: EV_KEY, *bdata->code);
+       if (button->wakeup_code) {
+               /*
+                * If wakeup_code is specified, we use it to report
+                * the button press during resume from suspend.
+                */
+               input_set_capability(input, button->type ?: EV_KEY,
+                                    button->wakeup_code);
+       }

         /*
          * Install custom action to cancel release timer and
@@ -1009,6 +1018,8 @@ gpio_keys_enable_wakeup(struct gpio_keys_drvdata 
*ddata)
                         if (error)
                                 goto err_out;
                 }
+               *bdata->code = bdata->button->wakeup_code ?
+                              bdata->button->wakeup_code : 
bdata->button->code;
                 bdata->suspended = true;
         }

@@ -1034,6 +1045,7 @@ gpio_keys_disable_wakeup(struct gpio_keys_drvdata 
*ddata)
         for (i = 0; i < ddata->pdata->nbuttons; i++) {
                 bdata = &ddata->data[i];
                 bdata->suspended = false;
+               *bdata->code = bdata->button->code;
                 if (irqd_is_wakeup_set(irq_get_irq_data(bdata->irq)))
                         gpio_keys_button_disable_wakeup(bdata);
         }
diff --git a/drivers/input/misc/soc_button_array.c 
b/drivers/input/misc/soc_button_array.c
index b8cad415c62ca..7afebc23b6f85 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -216,6 +216,8 @@ soc_button_device_create(struct platform_device *pdev,

                 gpio_keys[n_buttons].type = info->event_type;
                 gpio_keys[n_buttons].code = info->event_code;
+               if (info->event_code == KEY_POWER)
+                       gpio_keys[n_buttons].wakeup_code = KEY_WAKEUP;
                 gpio_keys[n_buttons].active_low = info->active_low;
                 gpio_keys[n_buttons].desc = info->name;
                 gpio_keys[n_buttons].wakeup = info->wakeup;
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index 80fa930b04c67..9759f2d4015f9 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -22,6 +22,7 @@ struct device;
   * @value:             axis value for %EV_ABS
   * @irq:               Irq number in case of interrupt keys
   * @wakeirq:           Optional dedicated wake-up interrupt
+ * @wakeup_code:       code to report when the button is during resume 
from suspend
   */
  struct gpio_keys_button {
         unsigned int code;
@@ -36,6 +37,7 @@ struct gpio_keys_button {
         int value;
         unsigned int irq;
         unsigned int wakeirq;
+       unsigned int wakeup_code;
  };

  /**

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Dmitry Torokhov]

On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
> > On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:

[ ... trim ... ]

> > 
> > 2. There is a patch from Mario (a8605b0ed187) suppressing sending
> > KEY_POWER as part of "normal" wakeup handling, pretty much the same as
> > what he and you are proposing to do in gpio-keys (and eventually in
> > every driver keyboard or button driver in the kernel). This means we no
> > longer can tell if wakeup is done by power button or sleep button (on
> > systems with dual-button models, see ACPI 4.8.3.1).
> 
> Actually a8605b0ed187 was about a runtime regression not a suspend
> regression.  I didn't change anything with sending KEY_POWER during wakeup
> handling.

Ah, right, ignorng events for "suspended" buttons was done in
e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
events"). Again trying to add heuristic to the kernel instead of
enlightening userspace.

I am curious why the system is sending "Notify Wake" events when not
sleeping though?

[ .. skip .. ]

> 
> FTR I did test Hans suggestion and it does work effectively (code below).
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c
> b/drivers/input/keyboard/gpio_keys.c
> index f9db86da0818b..3bc8c95e9943b 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
> *dev_id)
>                          * already released by the time we got interrupt
>                          * handler to run.
>                          */
> -                       input_report_key(bdata->input, button->code, 1);
> +                       input_report_key(bdata->input, *bdata->code, 1);
> +                       input_sync(bdata->input);

I start wondering if we should keep the fake press given that we do not
know for sure if wakeup truly happened because of this button press...

Can we track back to the wakeup source and determine this? It will not
help your problem, but I still believe userspace is where policy should
live.

Thanks.

-- 
Dmitry

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 27-Jun-25 9:18 PM, Dmitry Torokhov wrote:
> On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
>> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
>>> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
> 
> [ ... trim ... ]
> 
>>>
>>> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
>>> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
>>> what he and you are proposing to do in gpio-keys (and eventually in
>>> every driver keyboard or button driver in the kernel). This means we no
>>> longer can tell if wakeup is done by power button or sleep button (on
>>> systems with dual-button models, see ACPI 4.8.3.1).
>>
>> Actually a8605b0ed187 was about a runtime regression not a suspend
>> regression.  I didn't change anything with sending KEY_POWER during wakeup
>> handling.
> 
> Ah, right, ignorng events for "suspended" buttons was done in
> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
> events"). Again trying to add heuristic to the kernel instead of
> enlightening userspace.
> 
> I am curious why the system is sending "Notify Wake" events when not
> sleeping though?
> 
> [ .. skip .. ]
> 
>>
>> FTR I did test Hans suggestion and it does work effectively (code below).
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c
>> b/drivers/input/keyboard/gpio_keys.c
>> index f9db86da0818b..3bc8c95e9943b 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>> *dev_id)
>>                          * already released by the time we got interrupt
>>                          * handler to run.
>>                          */
>> -                       input_report_key(bdata->input, button->code, 1);
>> +                       input_report_key(bdata->input, *bdata->code, 1);
>> +                       input_sync(bdata->input);
> 
> I start wondering if we should keep the fake press given that we do not
> know for sure if wakeup truly happened because of this button press...

AFAIK we cannot drop the fake press because then Android userspace
will immediately go back to sleep again assuming the wakeup was
e.g. just some data coming in from the modem which did not result
in a notification to show, so no need to turn on the display,
but instead immediately go back to sleep.

IIRC last time we had this discussion (man years ago) the reason
to send KEY_POWER was to let Android know that it should actualy
turn on the display and show the unlock screen because the user
wants that to happen.

I believe this is also what the KEY_WAKEUP thing in the ACPI button
code is for.

> Can we track back to the wakeup source and determine this? It will not
> help your problem, but I still believe userspace is where policy should
> live.

There is /sys/power/pm_wakeup_irq we could correlate that to the IRQ
number of the ISR and then AFAICT we will definitively know if
the power-button was the wakeup source ?

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/27/2025 2:38 PM, Hans de Goede wrote:
> Hi,
> 
> On 27-Jun-25 9:18 PM, Dmitry Torokhov wrote:
>> On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
>>> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
>>>> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>>
>> [ ... trim ... ]
>>
>>>>
>>>> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
>>>> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
>>>> what he and you are proposing to do in gpio-keys (and eventually in
>>>> every driver keyboard or button driver in the kernel). This means we no
>>>> longer can tell if wakeup is done by power button or sleep button (on
>>>> systems with dual-button models, see ACPI 4.8.3.1).
>>>
>>> Actually a8605b0ed187 was about a runtime regression not a suspend
>>> regression.  I didn't change anything with sending KEY_POWER during wakeup
>>> handling.
>>
>> Ah, right, ignorng events for "suspended" buttons was done in
>> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
>> events"). Again trying to add heuristic to the kernel instead of
>> enlightening userspace.
>>
>> I am curious why the system is sending "Notify Wake" events when not
>> sleeping though?
>>
>> [ .. skip .. ]
>>
>>>
>>> FTR I did test Hans suggestion and it does work effectively (code below).
>>>
>>> diff --git a/drivers/input/keyboard/gpio_keys.c
>>> b/drivers/input/keyboard/gpio_keys.c
>>> index f9db86da0818b..3bc8c95e9943b 100644
>>> --- a/drivers/input/keyboard/gpio_keys.c
>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>> @@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>>> *dev_id)
>>>                           * already released by the time we got interrupt
>>>                           * handler to run.
>>>                           */
>>> -                       input_report_key(bdata->input, button->code, 1);
>>> +                       input_report_key(bdata->input, *bdata->code, 1);
>>> +                       input_sync(bdata->input);
>>
>> I start wondering if we should keep the fake press given that we do not
>> know for sure if wakeup truly happened because of this button press...
> 
> AFAIK we cannot drop the fake press because then Android userspace
> will immediately go back to sleep again assuming the wakeup was
> e.g. just some data coming in from the modem which did not result
> in a notification to show, so no need to turn on the display,
> but instead immediately go back to sleep.
> 
> IIRC last time we had this discussion (man years ago) the reason
> to send KEY_POWER was to let Android know that it should actualy
> turn on the display and show the unlock screen because the user
> wants that to happen.
> 
> I believe this is also what the KEY_WAKEUP thing in the ACPI button
> code is for.
> 
>> Can we track back to the wakeup source and determine this? It will not
>> help your problem, but I still believe userspace is where policy should
>> live.
> 
> There is /sys/power/pm_wakeup_irq we could correlate that to the IRQ
> number of the ISR and then AFAICT we will definitively know if
> the power-button was the wakeup source ?
> 

So at least in my case when woken up by this power button press the IRQ 
isn't the one for the GPIO itself, but rather for the GPIO controller 
master interrupt.

# cat /sys/power/pm_wakeup_irq
7
# grep . /sys/kernel/irq/7/*
/sys/kernel/irq/7/actions:pinctrl_amd
/sys/kernel/irq/7/chip_name:IR-IO-APIC
/sys/kernel/irq/7/hwirq:7
/sys/kernel/irq/7/name:fasteoi
/sys/kernel/irq/7/per_cpu_count:0,0,0,0,0,5,0,0
/sys/kernel/irq/7/type:level
/sys/kernel/irq/7/wakeup:enabled

# grep . /sys/kernel/irq/102/*
/sys/kernel/irq/102/actions:power
/sys/kernel/irq/102/chip_name:amd_gpio
/sys/kernel/irq/102/hwirq:0
/sys/kernel/irq/102/per_cpu_count:0,1,0,2,1,0,0,1
/sys/kernel/irq/102/type:edge
/sys/kernel/irq/102/wakeup:disabled

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 27-Jun-25 9:44 PM, Mario Limonciello wrote:
> On 6/27/2025 2:38 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 27-Jun-25 9:18 PM, Dmitry Torokhov wrote:
>>> On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
>>>> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
>>>>> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>>>
>>> [ ... trim ... ]
>>>
>>>>>
>>>>> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
>>>>> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
>>>>> what he and you are proposing to do in gpio-keys (and eventually in
>>>>> every driver keyboard or button driver in the kernel). This means we no
>>>>> longer can tell if wakeup is done by power button or sleep button (on
>>>>> systems with dual-button models, see ACPI 4.8.3.1).
>>>>
>>>> Actually a8605b0ed187 was about a runtime regression not a suspend
>>>> regression.  I didn't change anything with sending KEY_POWER during wakeup
>>>> handling.
>>>
>>> Ah, right, ignorng events for "suspended" buttons was done in
>>> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
>>> events"). Again trying to add heuristic to the kernel instead of
>>> enlightening userspace.
>>>
>>> I am curious why the system is sending "Notify Wake" events when not
>>> sleeping though?
>>>
>>> [ .. skip .. ]
>>>
>>>>
>>>> FTR I did test Hans suggestion and it does work effectively (code below).
>>>>
>>>> diff --git a/drivers/input/keyboard/gpio_keys.c
>>>> b/drivers/input/keyboard/gpio_keys.c
>>>> index f9db86da0818b..3bc8c95e9943b 100644
>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>> @@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>>>> *dev_id)
>>>>                           * already released by the time we got interrupt
>>>>                           * handler to run.
>>>>                           */
>>>> -                       input_report_key(bdata->input, button->code, 1);
>>>> +                       input_report_key(bdata->input, *bdata->code, 1);
>>>> +                       input_sync(bdata->input);
>>>
>>> I start wondering if we should keep the fake press given that we do not
>>> know for sure if wakeup truly happened because of this button press...
>>
>> AFAIK we cannot drop the fake press because then Android userspace
>> will immediately go back to sleep again assuming the wakeup was
>> e.g. just some data coming in from the modem which did not result
>> in a notification to show, so no need to turn on the display,
>> but instead immediately go back to sleep.
>>
>> IIRC last time we had this discussion (man years ago) the reason
>> to send KEY_POWER was to let Android know that it should actualy
>> turn on the display and show the unlock screen because the user
>> wants that to happen.
>>
>> I believe this is also what the KEY_WAKEUP thing in the ACPI button
>> code is for.
>>
>>> Can we track back to the wakeup source and determine this? It will not
>>> help your problem, but I still believe userspace is where policy should
>>> live.
>>
>> There is /sys/power/pm_wakeup_irq we could correlate that to the IRQ
>> number of the ISR and then AFAICT we will definitively know if
>> the power-button was the wakeup source ?
>>
> 
> So at least in my case when woken up by this power button press the IRQ isn't the one for the GPIO itself, but rather for the GPIO controller master interrupt.
> 
> # cat /sys/power/pm_wakeup_irq
> 7
> # grep . /sys/kernel/irq/7/*
> /sys/kernel/irq/7/actions:pinctrl_amd
> /sys/kernel/irq/7/chip_name:IR-IO-APIC
> /sys/kernel/irq/7/hwirq:7
> /sys/kernel/irq/7/name:fasteoi
> /sys/kernel/irq/7/per_cpu_count:0,0,0,0,0,5,0,0
> /sys/kernel/irq/7/type:level
> /sys/kernel/irq/7/wakeup:enabled
> 
> # grep . /sys/kernel/irq/102/*
> /sys/kernel/irq/102/actions:power
> /sys/kernel/irq/102/chip_name:amd_gpio
> /sys/kernel/irq/102/hwirq:0
> /sys/kernel/irq/102/per_cpu_count:0,1,0,2,1,0,0,1
> /sys/kernel/irq/102/type:edge
> /sys/kernel/irq/102/wakeup:disabled

Ah, right.

But thinking more about this I do not think believe
that wakeup racing is really a big issue here.

Wakeup racing only hits if the button ISR runs before
gpio_keys_resume() has run and cleared the bdata->suspended
flag. IOW the button was pressed before the system has
completely resumed in that case the users intend to me
very clearly was to wakeup the system.

So I still believe that sending key-wakeup for the simulated
keypress is the right thing to do in wakeup race cases even
if the system was actually woken up by e.g. network traffic.

As for Mario's patch from earlier in the thread that needs
some more work because it will release the wrong code if
the release ISR runs after gpio_keys_resume().

But working further on that only is useful if we can get
agreement from Dmitry on that approach.

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/27/2025 3:25 PM, Hans de Goede wrote:
> Hi,
> 
> On 27-Jun-25 9:44 PM, Mario Limonciello wrote:
>> On 6/27/2025 2:38 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 27-Jun-25 9:18 PM, Dmitry Torokhov wrote:
>>>> On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
>>>>> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
>>>>>> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>>>>
>>>> [ ... trim ... ]
>>>>
>>>>>>
>>>>>> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
>>>>>> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
>>>>>> what he and you are proposing to do in gpio-keys (and eventually in
>>>>>> every driver keyboard or button driver in the kernel). This means we no
>>>>>> longer can tell if wakeup is done by power button or sleep button (on
>>>>>> systems with dual-button models, see ACPI 4.8.3.1).
>>>>>
>>>>> Actually a8605b0ed187 was about a runtime regression not a suspend
>>>>> regression.  I didn't change anything with sending KEY_POWER during wakeup
>>>>> handling.
>>>>
>>>> Ah, right, ignorng events for "suspended" buttons was done in
>>>> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
>>>> events"). Again trying to add heuristic to the kernel instead of
>>>> enlightening userspace.
>>>>
>>>> I am curious why the system is sending "Notify Wake" events when not
>>>> sleeping though?
>>>>
>>>> [ .. skip .. ]
>>>>
>>>>>
>>>>> FTR I did test Hans suggestion and it does work effectively (code below).
>>>>>
>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c
>>>>> b/drivers/input/keyboard/gpio_keys.c
>>>>> index f9db86da0818b..3bc8c95e9943b 100644
>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>> @@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>>>>> *dev_id)
>>>>>                            * already released by the time we got interrupt
>>>>>                            * handler to run.
>>>>>                            */
>>>>> -                       input_report_key(bdata->input, button->code, 1);
>>>>> +                       input_report_key(bdata->input, *bdata->code, 1);
>>>>> +                       input_sync(bdata->input);
>>>>
>>>> I start wondering if we should keep the fake press given that we do not
>>>> know for sure if wakeup truly happened because of this button press...
>>>
>>> AFAIK we cannot drop the fake press because then Android userspace
>>> will immediately go back to sleep again assuming the wakeup was
>>> e.g. just some data coming in from the modem which did not result
>>> in a notification to show, so no need to turn on the display,
>>> but instead immediately go back to sleep.
>>>
>>> IIRC last time we had this discussion (man years ago) the reason
>>> to send KEY_POWER was to let Android know that it should actualy
>>> turn on the display and show the unlock screen because the user
>>> wants that to happen.
>>>
>>> I believe this is also what the KEY_WAKEUP thing in the ACPI button
>>> code is for.
>>>
>>>> Can we track back to the wakeup source and determine this? It will not
>>>> help your problem, but I still believe userspace is where policy should
>>>> live.
>>>
>>> There is /sys/power/pm_wakeup_irq we could correlate that to the IRQ
>>> number of the ISR and then AFAICT we will definitively know if
>>> the power-button was the wakeup source ?
>>>
>>
>> So at least in my case when woken up by this power button press the IRQ isn't the one for the GPIO itself, but rather for the GPIO controller master interrupt.
>>
>> # cat /sys/power/pm_wakeup_irq
>> 7
>> # grep . /sys/kernel/irq/7/*
>> /sys/kernel/irq/7/actions:pinctrl_amd
>> /sys/kernel/irq/7/chip_name:IR-IO-APIC
>> /sys/kernel/irq/7/hwirq:7
>> /sys/kernel/irq/7/name:fasteoi
>> /sys/kernel/irq/7/per_cpu_count:0,0,0,0,0,5,0,0
>> /sys/kernel/irq/7/type:level
>> /sys/kernel/irq/7/wakeup:enabled
>>
>> # grep . /sys/kernel/irq/102/*
>> /sys/kernel/irq/102/actions:power
>> /sys/kernel/irq/102/chip_name:amd_gpio
>> /sys/kernel/irq/102/hwirq:0
>> /sys/kernel/irq/102/per_cpu_count:0,1,0,2,1,0,0,1
>> /sys/kernel/irq/102/type:edge
>> /sys/kernel/irq/102/wakeup:disabled
> 
> Ah, right.
> 
> But thinking more about this I do not think believe
> that wakeup racing is really a big issue here.
> 
> Wakeup racing only hits if the button ISR runs before
> gpio_keys_resume() has run and cleared the bdata->suspended
> flag. IOW the button was pressed before the system has
> completely resumed in that case the users intend to me
> very clearly was to wakeup the system.
> 
> So I still believe that sending key-wakeup for the simulated
> keypress is the right thing to do in wakeup race cases even
> if the system was actually woken up by e.g. network traffic.
> 
> As for Mario's patch from earlier in the thread that needs
> some more work because it will release the wrong code if
> the release ISR runs after gpio_keys_resume().
 > > But working further on that only is useful if we can get
> agreement from Dmitry on that approach.
> 

Right; good catch.  I had thought this was a risk but didn't want to 
over-invest in solving it until we had alignment.  I mostly wanted to 
share it to demonstrate the idea works.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/27/2025 2:18 PM, Dmitry Torokhov wrote:
> On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
>> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
>>> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
> 
> [ ... trim ... ]
> 
>>>
>>> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
>>> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
>>> what he and you are proposing to do in gpio-keys (and eventually in
>>> every driver keyboard or button driver in the kernel). This means we no
>>> longer can tell if wakeup is done by power button or sleep button (on
>>> systems with dual-button models, see ACPI 4.8.3.1).
>>
>> Actually a8605b0ed187 was about a runtime regression not a suspend
>> regression.  I didn't change anything with sending KEY_POWER during wakeup
>> handling.
> 
> Ah, right, ignorng events for "suspended" buttons was done in
> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
> events"). Again trying to add heuristic to the kernel instead of
> enlightening userspace.
> 
> I am curious why the system is sending "Notify Wake" events when not
> sleeping though?

I wondered the same thing.  My guess is this is a BIOS bug.

> 
> [ .. skip .. ]
> 
>>
>> FTR I did test Hans suggestion and it does work effectively (code below).
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c
>> b/drivers/input/keyboard/gpio_keys.c
>> index f9db86da0818b..3bc8c95e9943b 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
>> *dev_id)
>>                           * already released by the time we got interrupt
>>                           * handler to run.
>>                           */
>> -                       input_report_key(bdata->input, button->code, 1);
>> +                       input_report_key(bdata->input, *bdata->code, 1);
>> +                       input_sync(bdata->input);
> 
> I start wondering if we should keep the fake press given that we do not
> know for sure if wakeup truly happened because of this button press...
> 
> Can we track back to the wakeup source and determine this? It will not
> help your problem, but I still believe userspace is where policy should
> live.
> 
> Thanks.
> 

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Rafael J. Wysocki]

On Thu, Jun 26, 2025 at 9:40 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Jun 26, 2025 at 09:31:12PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Jun 26, 2025 at 9:28 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Thu, Jun 26, 2025 at 09:18:56PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > > > > > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> > > > > >> Hi,
> > > > > >>
> > > > > >> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > > > > >>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> > > [...]
> > > > > >>>> I want to note this driver works quite differently than how ACPI power
> > > > > >>>> button does.
> > > > > >>>>
> > > > > >>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> > > > > >>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > > > > >>>> patch was modeling).
> > > > > >>>>
> > > > > >>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > > > > >>>> [1]
> > > > > >>>
> > > > > >>> If you check acpi_button_resume() you will see that the events are sent
> > > > > >>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> > > > > >>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > > > > >>> multiple other platforms.
> > > > > >>
> > > > > >> Interesting, but the ACPI button code presumably only does this on resume
> > > > > >> for a normal press while the system is awake it does use KEY_POWER, right ?
> > > > > >
> > > > > > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > > > > > it does not seem to be captured in the email discussions or in the patch
> > > > > > description.
> > > > >
> > > > > I assume they did this to avoid the immediate re-suspend on wakeup by
> > > > > power-button issue. GNOME has a workaround for this, but I assume that
> > > > > some userspace desktop environments are still going to have a problem
> > > > > with this.
> > > >
> > > > It was done for this reason IIRC, but it should have been documented
> > > > more thoroughly.
> > >
> > > I assert that it should not have been done and instead dealt with in
> > > userspace. There are numerous drivers in the kernel emitting
> > > KEY_POWER. Let userspace decide how to handle this, what keys to ignore,
> > > what keys to process and when.
> >
> > Please see my last message in this thread (just sent) and see the
> > changelog of commit 16f70feaabe9 ("ACPI: button: trigger wakeup key
> > events").
> >
> > This appears to be about cases when no event would be signaled to user
> > space at all (power button wakeup from ACPI S3).
>
> Ahh, in S3 we do not know if we've been woken up with Sleep or Power
> button, right? So we can not send the "right" event code and use
> "neutral" KEY_WAKEUP for both. Is this right?

Yes, it is, AFAICS.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Rafael J. Wysocki]

On Thu, Jun 26, 2025 at 9:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@kernel.org> wrote:
> >
> > Hi,
> >
> > On 26-Jun-25 21:14, Dmitry Torokhov wrote:
> > > On Thu, Jun 26, 2025 at 08:57:30PM +0200, Hans de Goede wrote:
> > >> Hi,
> > >>
> > >> On 26-Jun-25 20:48, Dmitry Torokhov wrote:
> > >>> On Thu, Jun 26, 2025 at 01:20:54PM -0500, Mario Limonciello wrote:
> > >>>> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> > >>>>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> > >>>>>>> Hi Mario,
> > >>>>>>>
> > >>>>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
> > >>>>>>>>> Hi Mario,
> > >>>>>>>>>
> > >>>>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
> > >>>>>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
> > >>>>>>>>>>
> > >>>>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
> > >>>>>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
> > >>>>>>>>>> causes a suspended system to wake up and then potentially turn off if
> > >>>>>>>>>> userspace is configured to turn off on power button presses.
> > >>>>>>>>>>
> > >>>>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
> > >>>>>>>>>>
> > >>>>>>>>>> Cc: Hans de Goede <hansg@kernel.org>
> > >>>>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > >>>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > >>>>>>>>>> ---
> > >>>>>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
> > >>>>>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
> > >>>>>>>>>>
> > >>>>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > >>>>>>>>>> index 773aa5294d269..4c6876b099c43 100644
> > >>>>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
> > >>>>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
> > >>>>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> > >>>>>>>>>>              pm_stay_awake(bdata->input->dev.parent);
> > >>>>>>>>>>              if (bdata->suspended  &&
> > >>>>>>>>>>                  (button->type == 0 || button->type == EV_KEY)) {
> > >>>>>>>>>> -                    /*
> > >>>>>>>>>> -                     * Simulate wakeup key press in case the key has
> > >>>>>>>>>> -                     * already released by the time we got interrupt
> > >>>>>>>>>> -                     * handler to run.
> > >>>>>>>>>> -                     */
> > >>>>>>>>>> -                    input_report_key(bdata->input, button->code, 1);
> > >>>>>>>>>> +                    pm_wakeup_event(bdata->input->dev.parent, 0);
> > >>>>>>>
> > >>>>>>> There is already pm_stay_awake() above.
> > >>>>>>
> > >>>>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
> > >>>>>> and reacts to it.
> > >>>>>>
> > >>>>>>>
> > >>>>>>>>>>              }
> > >>>>>>>>>>      }
> > >>>>>>>>>
> > >>>>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
> > >>>>>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
> > >>>>>>>>> Android userspace actually needs the button-press (evdev) event to not
> > >>>>>>>>> immediately go back to sleep, so a similar patch has been nacked in
> > >>>>>>>>> the past.
> > >>>>>>>>>
> > >>>>>>>>> At least for GNOME this has been fixed in userspace by ignoring
> > >>>>>>>>> power-button events the first few seconds after a resume from suspend.
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>> The default behavior for logind is:
> > >>>>>>>>
> > >>>>>>>> HandlePowerKey=poweroff
> > >>>>>>>>
> > >>>>>>>> Can you share more about what version of GNOME has a workaround?
> > >>>>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> > >>>>>>>>
> > >>>>>>>> Nonetheless if this is dependent on an Android userspace problem could we
> > >>>>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> > >>>>>>>
> > >>>>>>> No it is not only Android, other userspace may want to distinguish
> > >>>>>>> between normal and "dark" resume based on keyboard or other user
> > >>>>>>> activity.
> > >>>>>>>
> > >>>>>>> Thanks.
> > >>>>>>>
> > >>>>>> In this specific case does the key passed up to satisfy this userspace
> > >>>>>> requirement and keep it awake need to specifically be a fabricated
> > >>>>>> KEY_POWER?
> > >>>>>>
> > >>>>>> Or could we find a key that doesn't require some userspace to ignore
> > >>>>>> KEY_POWER?
> > >>>>>>
> > >>>>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> > >>>>>
> > >>>>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> > >>>>> It simply passes event to userspace for processing.
> > >>>>
> > >>>> Right.  I don't expect a problem with most keys, but my proposal is to
> > >>>> special case KEY_POWER while suspended.  If a key press event must be sent
> > >>>> to keep Android and other userspace happy I suggest sending something
> > >>>> different just for that situation.
> > >>>
> > >>> I do not know if userspace specifically looks for KEY_POWER or if it
> > >>> looks for user input in general, and I'd rather be on safe side and not
> > >>> mangle user input.
> > >>>
> > >>> As Hans mentioned, at least some userspace already prepared to deal with
> > >>> this issue. And again, this only works if by the time ISR/debounce
> > >>> runs the key is already released. What if it is still pressed? You still
> > >>> going to observe KEY_POWER and need to suppress turning off the screen.
> > >>>
> > >>>>
> > >>>> Like this:
> > >>>>
> > >>>> diff --git a/drivers/input/keyboard/gpio_keys.c
> > >>>> b/drivers/input/keyboard/gpio_keys.c
> > >>>> index 773aa5294d269..66e788d381956 100644
> > >>>> --- a/drivers/input/keyboard/gpio_keys.c
> > >>>> +++ b/drivers/input/keyboard/gpio_keys.c
> > >>>> @@ -425,7 +425,10 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void
> > >>>> *dev_id)
> > >>>>                          * already released by the time we got interrupt
> > >>>>                          * handler to run.
> > >>>>                          */
> > >>>> -                       input_report_key(bdata->input, button->code, 1);
> > >>>> +                       if (button->code == KEY_POWER)
> > >>>> +                               input_report_key(bdata->input, KEY_WAKEUP,
> > >>>> 1);
> > >>>
> > >>> Just FYI: Here your KEY_WAKEUP is stuck forever.
> > >>>
> > >>>> +                       else
> > >>>> +                               input_report_key(bdata->input, button->code,
> > >>>> 1);
> > >>>>                 }
> > >>>>         }
> > >>>>
> > >>>>
> > >>>>
> > >>>>>
> > >>>>> You need to fix your userspace. Even with your tweak it is possible for
> > >>>>> userspace to get a normal key event "too early" and turn off the screen
> > >>>>> again, so you still need to handle this situation.
> > >>>>>
> > >>>>> Thanks.
> > >>>>>
> > >>>>
> > >>>> I want to note this driver works quite differently than how ACPI power
> > >>>> button does.
> > >>>>
> > >>>> You can see in acpi_button_notify() that the "keypress" is only forwarded
> > >>>> when not suspended [1].  Otherwise it's just wakeup event (which is what my
> > >>>> patch was modeling).
> > >>>>
> > >>>> https://github.com/torvalds/linux/blob/v6.16-rc3/drivers/acpi/button.c#L461
> > >>>> [1]
> > >>>
> > >>> If you check acpi_button_resume() you will see that the events are sent
> > >>> from there. Except that for some reason they chose to use KEY_WAKEUP and
> > >>> not KEY_POWER, oh well. Unlike acpi button driver gpio_keys is used on
> > >>> multiple other platforms.
> > >>
> > >> Interesting, but the ACPI button code presumably only does this on resume
> > >> for a normal press while the system is awake it does use KEY_POWER, right ?
> > >
> > > Yes. It is unclear to me why they chose to mangle the event on wakeup,
> > > it does not seem to be captured in the email discussions or in the patch
> > > description.
> >
> > I assume they did this to avoid the immediate re-suspend on wakeup by
> > power-button issue. GNOME has a workaround for this, but I assume that
> > some userspace desktop environments are still going to have a problem
> > with this.
>
> It was done for this reason IIRC, but it should have been documented
> more thoroughly.

Actually, I'm thinking that this should only be done on S3 resume, not
on s2idle resume.

On S3 resume, there is no indication from the kernel to user space
which event has caused the wakeup if it is the power button and so
commit 16f70feaabe9 ("ACPI: button: trigger wakeup key events")
attempted to mitigate this.

However, on s2idle wakeup the actual input event is sent to user
space, so there is no need to make up one.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi Mario,

On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> 
> 
> On 6/26/25 3:35 AM, Hans de Goede wrote:
> > Hi Mario,
> > 
> > On 25-Jun-25 23:58, Mario Limonciello wrote:
> > > From: Mario Limonciello <mario.limonciello@amd.com>
> > > 
> > > Sending an input event to wake a system does wake it, but userspace picks
> > > up the keypress and processes it.  This isn't the intended behavior as it
> > > causes a suspended system to wake up and then potentially turn off if
> > > userspace is configured to turn off on power button presses.
> > > 
> > > Instead send a PM wakeup event for the PM core to handle waking the system.
> > > 
> > > Cc: Hans de Goede <hansg@kernel.org>
> > > Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > >   drivers/input/keyboard/gpio_keys.c | 7 +------
> > >   1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> > > index 773aa5294d269..4c6876b099c43 100644
> > > --- a/drivers/input/keyboard/gpio_keys.c
> > > +++ b/drivers/input/keyboard/gpio_keys.c
> > > @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> > >   		pm_stay_awake(bdata->input->dev.parent);
> > >   		if (bdata->suspended  &&
> > >   		    (button->type == 0 || button->type == EV_KEY)) {
> > > -			/*
> > > -			 * Simulate wakeup key press in case the key has
> > > -			 * already released by the time we got interrupt
> > > -			 * handler to run.
> > > -			 */
> > > -			input_report_key(bdata->input, button->code, 1);
> > > +			pm_wakeup_event(bdata->input->dev.parent, 0);
> > >   		}
> > >   	}
> > 
> > Hmm, we have the same problem on many Bay Trail / Cherry Trail
> > windows 8 / win10 tablets, so  this has been discussed before and e.g.
> > Android userspace actually needs the button-press (evdev) event to not
> > immediately go back to sleep, so a similar patch has been nacked in
> > the past.
> > 
> > At least for GNOME this has been fixed in userspace by ignoring
> > power-button events the first few seconds after a resume from suspend.
> > 
> 
> The default behavior for logind is:
> 
> HandlePowerKey=poweroff

Right note poweroff not suspend, GNOME inhibits logind's power-button
handling and substitutes its own handling which is done by gsd-media-keys.

> Can you share more about what version of GNOME has a workaround?
> This was actually GNOME (on Ubuntu 24.04) that I found this issue.

See:

https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/main/plugins/media-keys/gsd-media-keys-manager.c?ref_type=heads#L94

and the code in that file using that define.

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/2025 1:37 PM, Hans de Goede wrote:
> Hi Mario,
> 
> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>
>>
>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>> Hi Mario,
>>>
>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>> causes a suspended system to wake up and then potentially turn off if
>>>> userspace is configured to turn off on power button presses.
>>>>
>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>
>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>    1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>> index 773aa5294d269..4c6876b099c43 100644
>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>    		pm_stay_awake(bdata->input->dev.parent);
>>>>    		if (bdata->suspended  &&
>>>>    		    (button->type == 0 || button->type == EV_KEY)) {
>>>> -			/*
>>>> -			 * Simulate wakeup key press in case the key has
>>>> -			 * already released by the time we got interrupt
>>>> -			 * handler to run.
>>>> -			 */
>>>> -			input_report_key(bdata->input, button->code, 1);
>>>> +			pm_wakeup_event(bdata->input->dev.parent, 0);
>>>>    		}
>>>>    	}
>>>
>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>> Android userspace actually needs the button-press (evdev) event to not
>>> immediately go back to sleep, so a similar patch has been nacked in
>>> the past.
>>>
>>> At least for GNOME this has been fixed in userspace by ignoring
>>> power-button events the first few seconds after a resume from suspend.
>>>
>>
>> The default behavior for logind is:
>>
>> HandlePowerKey=poweroff
> 
> Right note poweroff not suspend, GNOME inhibits logind's power-button
> handling and substitutes its own handling which is done by gsd-media-keys.
> 
>> Can you share more about what version of GNOME has a workaround?
>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> 
> See:
> 
> https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/main/plugins/media-keys/gsd-media-keys-manager.c?ref_type=heads#L94
> 
> and the code in that file using that define.
> 
> Regards,
> 
> Hans
> 

This is pretty ancient, it's part of GNOME 40 and later.  Ubuntu 24.04 
is GNOME 46.

I have a hypothesis why the issue is happening.  I wasn't suspending 
using logind; I was using rtcwake (in case power button didn't work I 
needed a means to wake the system).

So this power button disable/enable delay code didn't get activated.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Hans de Goede]

Hi,

On 26-Jun-25 20:42, Mario Limonciello wrote:
> On 6/26/2025 1:37 PM, Hans de Goede wrote:
>> Hi Mario,
>>
>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>>
>>>
>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>>> Hi Mario,
>>>>
>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>
>>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>>> causes a suspended system to wake up and then potentially turn off if
>>>>> userspace is configured to turn off on power button presses.
>>>>>
>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>>
>>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>>>    drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>>    1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>> index 773aa5294d269..4c6876b099c43 100644
>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>>            pm_stay_awake(bdata->input->dev.parent);
>>>>>            if (bdata->suspended  &&
>>>>>                (button->type == 0 || button->type == EV_KEY)) {
>>>>> -            /*
>>>>> -             * Simulate wakeup key press in case the key has
>>>>> -             * already released by the time we got interrupt
>>>>> -             * handler to run.
>>>>> -             */
>>>>> -            input_report_key(bdata->input, button->code, 1);
>>>>> +            pm_wakeup_event(bdata->input->dev.parent, 0);
>>>>>            }
>>>>>        }
>>>>
>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>>> Android userspace actually needs the button-press (evdev) event to not
>>>> immediately go back to sleep, so a similar patch has been nacked in
>>>> the past.
>>>>
>>>> At least for GNOME this has been fixed in userspace by ignoring
>>>> power-button events the first few seconds after a resume from suspend.
>>>>
>>>
>>> The default behavior for logind is:
>>>
>>> HandlePowerKey=poweroff
>>
>> Right note poweroff not suspend, GNOME inhibits logind's power-button
>> handling and substitutes its own handling which is done by gsd-media-keys.
>>
>>> Can you share more about what version of GNOME has a workaround?
>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>
>> See:
>>
>> https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/main/plugins/media-keys/gsd-media-keys-manager.c?ref_type=heads#L94
>>
>> and the code in that file using that define.
>>
>> Regards,
>>
>> Hans
>>
> 
> This is pretty ancient, it's part of GNOME 40 and later.  Ubuntu 24.04 is GNOME 46.

Right, this power-button evdev key event on resume causing an immediate
suspend again is a pretty old problem. When I said "discussed before"
I meant discussed quite a while ago :)

Regards,

Hans

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello]

On 6/26/2025 1:45 PM, Hans de Goede wrote:
> Hi,
> 
> On 26-Jun-25 20:42, Mario Limonciello wrote:
>> On 6/26/2025 1:37 PM, Hans de Goede wrote:
>>> Hi Mario,
>>>
>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
>>>>
>>>>
>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
>>>>> Hi Mario,
>>>>>
>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>
>>>>>> Sending an input event to wake a system does wake it, but userspace picks
>>>>>> up the keypress and processes it.  This isn't the intended behavior as it
>>>>>> causes a suspended system to wake up and then potentially turn off if
>>>>>> userspace is configured to turn off on power button presses.
>>>>>>
>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
>>>>>>
>>>>>> Cc: Hans de Goede <hansg@kernel.org>
>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>>>     drivers/input/keyboard/gpio_keys.c | 7 +------
>>>>>>     1 file changed, 1 insertion(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
>>>>>> index 773aa5294d269..4c6876b099c43 100644
>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>>>>>>             pm_stay_awake(bdata->input->dev.parent);
>>>>>>             if (bdata->suspended  &&
>>>>>>                 (button->type == 0 || button->type == EV_KEY)) {
>>>>>> -            /*
>>>>>> -             * Simulate wakeup key press in case the key has
>>>>>> -             * already released by the time we got interrupt
>>>>>> -             * handler to run.
>>>>>> -             */
>>>>>> -            input_report_key(bdata->input, button->code, 1);
>>>>>> +            pm_wakeup_event(bdata->input->dev.parent, 0);
>>>>>>             }
>>>>>>         }
>>>>>
>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
>>>>> windows 8 / win10 tablets, so  this has been discussed before and e.g.
>>>>> Android userspace actually needs the button-press (evdev) event to not
>>>>> immediately go back to sleep, so a similar patch has been nacked in
>>>>> the past.
>>>>>
>>>>> At least for GNOME this has been fixed in userspace by ignoring
>>>>> power-button events the first few seconds after a resume from suspend.
>>>>>
>>>>
>>>> The default behavior for logind is:
>>>>
>>>> HandlePowerKey=poweroff
>>>
>>> Right note poweroff not suspend, GNOME inhibits logind's power-button
>>> handling and substitutes its own handling which is done by gsd-media-keys.
>>>
>>>> Can you share more about what version of GNOME has a workaround?
>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
>>>
>>> See:
>>>
>>> https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/main/plugins/media-keys/gsd-media-keys-manager.c?ref_type=heads#L94
>>>
>>> and the code in that file using that define.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>
>> This is pretty ancient, it's part of GNOME 40 and later.  Ubuntu 24.04 is GNOME 46.
> 
> Right, this power-button evdev key event on resume causing an immediate
> suspend again is a pretty old problem. When I said "discussed before"
> I meant discussed quite a while ago :)
> 
> Regards,
> 
> Hans
> 

:)

I'm quite used to most platforms using ACPI power button which doesn't 
send a key press event during the resume sequence, so this is the first 
exposure for me to it.

Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 04:58:13PM -0500, Mario Limonciello wrote:
> 
> Sending an input event to wake a system does wake it, but userspace picks
> up the keypress and processes it.  This isn't the intended behavior as it
> causes a suspended system to wake up and then potentially turn off if
> userspace is configured to turn off on power button presses.
> 
> Instead send a PM wakeup event for the PM core to handle waking the system.

It seems the Subject is incorrect WRT the pattern used in input subsystem
(in this and in the previous patches).

Should be something like: Input: $driver - $summary

where $driver and $summary the exact driver name and summary you want to put.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] Fix soc-button-array debounce

[Andy Shevchenko]

On Wed, Jun 25, 2025 at 04:58:09PM -0500, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> I have some hardware in front of me that uses the soc-button-array
> driver but the power button doesn't work.
> 
> Digging into it, it's because the ASL prescribes a debounce of 0 for
> the power button, but the soc-button-array driver hardcodes 50ms.
> 
> Hardcoding it to what the ASL expects the power button works.
> 
> I looked at the callpath into the GPIO core and I believe it's
> because the debounce value from _CRS is never programmed to the
> hardware the way that the GPIO gets setup.
> 
> This series add that programming path and then sets the hardcoded
> value on on some quirked systems.  Hopefully Hans can confirm this
> continues to work on the hardware that he originally developed the
> hardcoding for.

There is no a note about routing the patch in upstream. My proposal to take
GPIO ACPI library patch and provide and immutable tag for others.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] Fix soc-button-array debounce

[Mario Limonciello]

On 6/26/2025 9:35 AM, Andy Shevchenko wrote:
> On Wed, Jun 25, 2025 at 04:58:09PM -0500, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> I have some hardware in front of me that uses the soc-button-array
>> driver but the power button doesn't work.
>>
>> Digging into it, it's because the ASL prescribes a debounce of 0 for
>> the power button, but the soc-button-array driver hardcodes 50ms.
>>
>> Hardcoding it to what the ASL expects the power button works.
>>
>> I looked at the callpath into the GPIO core and I believe it's
>> because the debounce value from _CRS is never programmed to the
>> hardware the way that the GPIO gets setup.
>>
>> This series add that programming path and then sets the hardcoded
>> value on on some quirked systems.  Hopefully Hans can confirm this
>> continues to work on the hardware that he originally developed the
>> hardcoding for.
> 
> There is no a note about routing the patch in upstream. My proposal to take
> GPIO ACPI library patch and provide and immutable tag for others.
> 

Sure.  I don't feel strongly on how this needs to go, fine by me.  I'll 
add a note in cover letter for this for v4.