Message-ID: <aF7C0B038Qi-CUSk@smile.fi.intel.com>
Date: Fri, 27 Jun 2025 19:12:00 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Hans de Goede <hansg@...nel.org>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.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 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.
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.

Just my 2c.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists