On Thu, Jun 26, 2025 at 9:16 PM Hans de Goede <hansg@xxxxxxxxxx> 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:
> >>>> On 6/26/2025 1:07 PM, Dmitry Torokhov wrote:
> >>>>> On Thu, Jun 26, 2025 at 12:53:02PM -0500, Mario Limonciello wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 6/26/25 12:44 PM, Dmitry Torokhov wrote:
> >>>>>>> Hi Mario,
> >>>>>>>
> >>>>>>> On Thu, Jun 26, 2025 at 06:33:08AM -0500, Mario Limonciello wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 6/26/25 3:35 AM, Hans de Goede wrote:
> >>>>>>>>> Hi Mario,
> >>>>>>>>>
> >>>>>>>>> On 25-Jun-25 23:58, Mario Limonciello wrote:
> >>>>>>>>>> From: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>>>>>>>>>
> >>>>>>>>>> Sending an input event to wake a system does wake it, but userspace picks
> >>>>>>>>>> up the keypress and processes it. This isn't the intended behavior as it
> >>>>>>>>>> causes a suspended system to wake up and then potentially turn off if
> >>>>>>>>>> userspace is configured to turn off on power button presses.
> >>>>>>>>>>
> >>>>>>>>>> Instead send a PM wakeup event for the PM core to handle waking the system.
> >>>>>>>>>>
> >>>>>>>>>> Cc: Hans de Goede <hansg@xxxxxxxxxx>
> >>>>>>>>>> Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> >>>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/input/keyboard/gpio_keys.c | 7 +------
> >>>>>>>>>> 1 file changed, 1 insertion(+), 6 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> >>>>>>>>>> index 773aa5294d269..4c6876b099c43 100644
> >>>>>>>>>> --- a/drivers/input/keyboard/gpio_keys.c
> >>>>>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>>>>>>>>> @@ -420,12 +420,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> >>>>>>>>>> pm_stay_awake(bdata->input->dev.parent);
> >>>>>>>>>> if (bdata->suspended &&
> >>>>>>>>>> (button->type == 0 || button->type == EV_KEY)) {
> >>>>>>>>>> - /*
> >>>>>>>>>> - * Simulate wakeup key press in case the key has
> >>>>>>>>>> - * already released by the time we got interrupt
> >>>>>>>>>> - * handler to run.
> >>>>>>>>>> - */
> >>>>>>>>>> - input_report_key(bdata->input, button->code, 1);
> >>>>>>>>>> + pm_wakeup_event(bdata->input->dev.parent, 0);
> >>>>>>>
> >>>>>>> There is already pm_stay_awake() above.
> >>>>>>
> >>>>>> But that doesn't help with the fact that userspace gets KEY_POWER from this
> >>>>>> and reacts to it.
> >>>>>>
> >>>>>>>
> >>>>>>>>>> }
> >>>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> Hmm, we have the same problem on many Bay Trail / Cherry Trail
> >>>>>>>>> windows 8 / win10 tablets, so this has been discussed before and e.g.
> >>>>>>>>> Android userspace actually needs the button-press (evdev) event to not
> >>>>>>>>> immediately go back to sleep, so a similar patch has been nacked in
> >>>>>>>>> the past.
> >>>>>>>>>
> >>>>>>>>> At least for GNOME this has been fixed in userspace by ignoring
> >>>>>>>>> power-button events the first few seconds after a resume from suspend.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The default behavior for logind is:
> >>>>>>>>
> >>>>>>>> HandlePowerKey=poweroff
> >>>>>>>>
> >>>>>>>> Can you share more about what version of GNOME has a workaround?
> >>>>>>>> This was actually GNOME (on Ubuntu 24.04) that I found this issue.
> >>>>>>>>
> >>>>>>>> Nonetheless if this is dependent on an Android userspace problem could we
> >>>>>>>> perhaps conditionalize it on CONFIG_ANDROID_BINDER_DEVICES?
> >>>>>>>
> >>>>>>> No it is not only Android, other userspace may want to distinguish
> >>>>>>> between normal and "dark" resume based on keyboard or other user
> >>>>>>> activity.
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>>>
> >>>>>> In this specific case does the key passed up to satisfy this userspace
> >>>>>> requirement and keep it awake need to specifically be a fabricated
> >>>>>> KEY_POWER?
> >>>>>>
> >>>>>> Or could we find a key that doesn't require some userspace to ignore
> >>>>>> KEY_POWER?
> >>>>>>
> >>>>>> Maybe something like KEY_RESERVED, KEY_FN, or KEY_POWER2?
> >>>>>
> >>>>> The code makes no distinction between KEY_POWER and KEY_A or KEY_B, etc.
> >>>>> It simply passes event to userspace for processing.
> >>>>
> >>>> Right. I don't expect a problem with most keys, but my proposal is to
> >>>> special case KEY_POWER while suspended. If a key press event must be sent
> >>>> to keep Android and other userspace happy I suggest sending something
> >>>> different just for that situation.
> >>>
> >>> I do not know if userspace specifically looks for KEY_POWER or if it
> >>> looks for user input in general, and I'd rather be on safe side and not
> >>> mangle user input.
> >>>
> >>> As Hans mentioned, at least some userspace already prepared to deal with
> >>> this issue. And again, this only works if by the time ISR/debounce
> >>> runs the key is already released. What if it is still pressed? You still
> >>> going to observe KEY_POWER and need to suppress turning off the screen.
> >>>
> >>>>
> >>>> Like this:
> >>>>
> >>>> diff --git a/drivers/input/keyboard/gpio_keys.c
> >>>> b/drivers/input/keyboard/gpio_keys.c
> >>>> index 773aa5294d269..66e788d381956 100644
> >>>> --- a/drivers/input/keyboard/gpio_keys.c
> >>>> +++ b/drivers/input/keyboard/gpio_keys.c
> >>>> @@ -425,7 +425,10 @@ 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);
> >>>> + if (button->code == KEY_POWER)
> >>>> + input_report_key(bdata->input, KEY_WAKEUP,
> >>>> 1);
> >>>
> >>> Just FYI: Here your KEY_WAKEUP is stuck forever.
> >>>
> >>>> + else
> >>>> + input_report_key(bdata->input, button->code,
> >>>> 1);
> >>>> }
> >>>> }
> >>>>
> >>>>
> >>>>
> >>>>>
> >>>>> You need to fix your userspace. Even with your tweak it is possible for
> >>>>> userspace to get a normal key event "too early" and turn off the screen
> >>>>> again, so you still need to handle this situation.
> >>>>>
> >>>>> Thanks.
> >>>>>
> >>>>
> >>>> 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.
