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

[Hans de Goede <hansg@kernel.org>]

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