Message-ID: <bnivc2kt4klb3krkyltafnp3i4ivu5permgm4fgysnmjlukspe@gjvhufpciz3m>
Date: Fri, 27 Jun 2025 11:47:55 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Hans de Goede <hansg@...nel.org>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Mario Limonciello <superm1@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Mika Westerberg <westeri@...nel.org>, Linus Walleij <linus.walleij@...aro.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, "open list:GPIO ACPI SUPPORT" <linux-gpio@...r.kernel.org>, 
	"open list:GPIO ACPI SUPPORT" <linux-acpi@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>, 
	"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..." <linux-input@...r.kernel.org>, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake
 system

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

Powered by blists - more mailing lists