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

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

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