Message-ID: <4ajmcrl3bqeikki2etek5bafzszelgevr322tvuubx4pxxyju2@qqxz6lzcb6e5>
Date: Fri, 27 Jun 2025 07:44:05 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Hans de Goede <hansg@...nel.org>
Cc: Mario Limonciello <superm1@...nel.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Mika Westerberg <westeri@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 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 04:14:38PM +0200, Hans de Goede wrote:
> 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@...il.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@...nel.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 ?

And what is the plan on dealing with all other drivers that emit
KEY_POWER? What about acpi button behavior when using S0ix? What about
holding power button for too long so that normal reporting "catches" the
pressed state?

Kernel reports hardware events, interpreting them and applying certain
policies is task for userspace.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists