Re: [PATCH v3 4/4] Input: Don't send fake button presses to wake system

[Mario Limonciello <superm1@kernel.org>]

On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>> Hi Dmitry,
>>
>> On 27-Jun-25 4:44 PM, Dmitry Torokhov wrote:
>>> 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@gmail.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@kernel.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?
>>
>> There actually aren't that many that I'm aware of.
> 
> dtor@dtor-ws:~/kernel/work $ git grep -l KEY_POWER -- drivers/{input,hid,platform}/ | wc -l
> 51
> 
> Additionally:
> 
> dtor@dtor-ws:~/kernel/work $ git grep -l KEY_POWER -- arch/arm*/boot/dts | wc -l
> 254
> 
>>
>> 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.
> 
> I am sorry to hear that. I am not involved in Gnome so I am not sure why
> it takes so long, I guess not tablets or detachables are a minority of
> deployments so it is not prioritized? Android seems to have a handle on
> this as does Chrome OS...
> 
>>
>> And I've not seen any bug-reports about the same problem
>> with any other drivers.
> 
> I guess you will next want to patch cros_ec_keyb in case someone uses
> generic distro with x86 Chromebooks, and then matrix keypad, and then
> all drivers that are used outside of x86.
> 
>>
>>> 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.
> 
> Rafael has made assertion that in S3 it is impossible to differentiate
> whether the button is power button or not. But looking at this I do not
> think it is correct assertion. "Notify Wake" is sent for a given
> button. Or maybe I misunderstood and he meant that the *state* if button
> is not available in "Notify Wake"?
> 
> In fact, I think login in the ACPI button is pretty broken and needs to
> be undone/reverted:
> 
> 1. The driver sends KEY_WAKEUP events on every resume. It does not
> matter if wakeup is done by pressing power button, wake or lan packet,
> or an act of God, it will send KEY_WAKEUP as part of the button *device*
> resume.

This seems like a real bug.  IMO you're right it should only be sending 
it when the key was received.

> 
> 2. There is a patch from Mario (a8605b0ed187) suppressing sending
> KEY_POWER as part of "normal" wakeup handling, pretty much the same as
> what he and you are proposing to do in gpio-keys (and eventually in
> every driver keyboard or button driver in the kernel). This means we no
> longer can tell if wakeup is done by power button or sleep button (on
> systems with dual-button models, see ACPI 4.8.3.1).

Actually a8605b0ed187 was about a runtime regression not a suspend 
regression.  I didn't change anything with sending KEY_POWER during 
wakeup handling.

> 
> To me it seems we are piling workarounds on top of workarounds. We
> should report either KEY_POWER or KEY_SLEEP in the notify event for both
> "Notify Wake" and "Notify State", and let userspace make decision on how
> to handle this best.
> 
>>
>> 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.
> 
> I do not understand why you call KEY_WAKEUP being "proper". Especially
> with gpio-keys you do not know if wakeup is really due to this
> particular button being pressed, or the system woke up for some other
> reason and user also happened to have the button pressed. ACPI button is
> really an outlier here because firmware interface defines a dedicated
> event for waking up.
> 
>>
>>> 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.
> 
> This assumes the driver does not report state between press and release,
> which is not guaranteed. Or we now require drivers to keep track whether
> release for KEY_WAKEUP has been done and that it can return to reporting
> 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.
> 
> Does it really? Aside of ACPI button you have no idea if button press
> coincided with some other event waking up the system. You can guess, but
> that is it, a guess.
> 
>>
>> So the suggested change actually makes the kernel
>> do its job of reporting hw-events better by making
>> the reporting more accurate.
> 
> No, you are making assumptions, but instead of doing it in userspace
> where you can give user choice to adjust the behavior you are
> hard-coding it.
> 
>>
>> 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
> 
> Please fix Gnome. It is possible to handle rapid power button presses
> (because other OSed/environments do that).
> 
>>
>> 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 opinion is that KEY_WAKEUP is not much better and we are actually
> losing information if we try to implement that (again, because you do
> not know if button press was the real cause of wakeup or it simply
> coincided with something else).
> 
> Thanks.
> 

FTR I did test Hans suggestion and it does work effectively (code below).

diff --git a/drivers/input/keyboard/gpio_keys.c 
b/drivers/input/keyboard/gpio_keys.c
index f9db86da0818b..3bc8c95e9943b 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -425,7 +425,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void 
*dev_id)
                          * already released by the time we got interrupt
                          * handler to run.
                          */
-                       input_report_key(bdata->input, button->code, 1);
+                       input_report_key(bdata->input, *bdata->code, 1);
+                       input_sync(bdata->input);
                 }
         }

@@ -644,6 +645,14 @@ static int gpio_keys_setup_key(struct 
platform_device *pdev,
         bdata->code = &ddata->keymap[idx];
         *bdata->code = button->code;
         input_set_capability(input, button->type ?: EV_KEY, *bdata->code);
+       if (button->wakeup_code) {
+               /*
+                * If wakeup_code is specified, we use it to report
+                * the button press during resume from suspend.
+                */
+               input_set_capability(input, button->type ?: EV_KEY,
+                                    button->wakeup_code);
+       }

         /*
          * Install custom action to cancel release timer and
@@ -1009,6 +1018,8 @@ gpio_keys_enable_wakeup(struct gpio_keys_drvdata 
*ddata)
                         if (error)
                                 goto err_out;
                 }
+               *bdata->code = bdata->button->wakeup_code ?
+                              bdata->button->wakeup_code : 
bdata->button->code;
                 bdata->suspended = true;
         }

@@ -1034,6 +1045,7 @@ gpio_keys_disable_wakeup(struct gpio_keys_drvdata 
*ddata)
         for (i = 0; i < ddata->pdata->nbuttons; i++) {
                 bdata = &ddata->data[i];
                 bdata->suspended = false;
+               *bdata->code = bdata->button->code;
                 if (irqd_is_wakeup_set(irq_get_irq_data(bdata->irq)))
                         gpio_keys_button_disable_wakeup(bdata);
         }
diff --git a/drivers/input/misc/soc_button_array.c 
b/drivers/input/misc/soc_button_array.c
index b8cad415c62ca..7afebc23b6f85 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -216,6 +216,8 @@ soc_button_device_create(struct platform_device *pdev,

                 gpio_keys[n_buttons].type = info->event_type;
                 gpio_keys[n_buttons].code = info->event_code;
+               if (info->event_code == KEY_POWER)
+                       gpio_keys[n_buttons].wakeup_code = KEY_WAKEUP;
                 gpio_keys[n_buttons].active_low = info->active_low;
                 gpio_keys[n_buttons].desc = info->name;
                 gpio_keys[n_buttons].wakeup = info->wakeup;
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index 80fa930b04c67..9759f2d4015f9 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -22,6 +22,7 @@ struct device;
   * @value:             axis value for %EV_ABS
   * @irq:               Irq number in case of interrupt keys
   * @wakeirq:           Optional dedicated wake-up interrupt
+ * @wakeup_code:       code to report when the button is during resume 
from suspend
   */
  struct gpio_keys_button {
         unsigned int code;
@@ -36,6 +37,7 @@ struct gpio_keys_button {
         int value;
         unsigned int irq;
         unsigned int wakeirq;
+       unsigned int wakeup_code;
  };

  /**