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@xxxxxxxxx> 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@xxxxxxxxxx> 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
