| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <obpakvzyludc4jskqzyxf65dhqds7ie3jkbfsqdve32ouuaili@xvogkmwvbmbf>
Date: Thu, 26 Jun 2025 11:48:45 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Mario Limonciello <superm1@...nel.org>
Cc: Hans de Goede <hansg@...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 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@....com>
> > > > > > >
> > > > > > > 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@...nel.org>
> > > > > > > Fixes: 0f107573da417 ("Input: gpio_keys - handle the missing key press event in resume phase")
> > > > > > > Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> > > > > > > ---
> > > > > > > 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.
Thanks.
--
Dmitry
Powered by blists - more mailing lists