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
