Message-ID: <5c8ca17f-2f77-4492-ade7-a78dba45df7d@kernel.org>
Date: Fri, 27 Jun 2025 22:25:59 +0200
From: Hans de Goede <hansg@...nel.org>
To: Mario Limonciello <superm1@...nel.org>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: "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

Hi,

On 27-Jun-25 9:44 PM, Mario Limonciello wrote:
> On 6/27/2025 2:38 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 27-Jun-25 9:18 PM, Dmitry Torokhov wrote:
>>> On Fri, Jun 27, 2025 at 01:56:53PM -0500, Mario Limonciello wrote:
>>>> On 6/27/2025 1:36 PM, Dmitry Torokhov wrote:
>>>>> On Fri, Jun 27, 2025 at 05:56:05PM +0200, Hans de Goede wrote:
>>>
>>> [ ... trim ... ]
>>>
>>>>>
>>>>> 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.
>>>
>>> Ah, right, ignorng events for "suspended" buttons was done in
>>> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend
>>> events"). Again trying to add heuristic to the kernel instead of
>>> enlightening userspace.
>>>
>>> I am curious why the system is sending "Notify Wake" events when not
>>> sleeping though?
>>>
>>> [ .. skip .. ]
>>>
>>>>
>>>> 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);
>>>
>>> I start wondering if we should keep the fake press given that we do not
>>> know for sure if wakeup truly happened because of this button press...
>>
>> AFAIK we cannot drop the fake press because then Android userspace
>> will immediately go back to sleep again assuming the wakeup was
>> e.g. just some data coming in from the modem which did not result
>> in a notification to show, so no need to turn on the display,
>> but instead immediately go back to sleep.
>>
>> IIRC last time we had this discussion (man years ago) the reason
>> to send KEY_POWER was to let Android know that it should actualy
>> turn on the display and show the unlock screen because the user
>> wants that to happen.
>>
>> I believe this is also what the KEY_WAKEUP thing in the ACPI button
>> code is for.
>>
>>> Can we track back to the wakeup source and determine this? It will not
>>> help your problem, but I still believe userspace is where policy should
>>> live.
>>
>> There is /sys/power/pm_wakeup_irq we could correlate that to the IRQ
>> number of the ISR and then AFAICT we will definitively know if
>> the power-button was the wakeup source ?
>>
> 
> So at least in my case when woken up by this power button press the IRQ isn't the one for the GPIO itself, but rather for the GPIO controller master interrupt.
> 
> # cat /sys/power/pm_wakeup_irq
> 7
> # grep . /sys/kernel/irq/7/*
> /sys/kernel/irq/7/actions:pinctrl_amd
> /sys/kernel/irq/7/chip_name:IR-IO-APIC
> /sys/kernel/irq/7/hwirq:7
> /sys/kernel/irq/7/name:fasteoi
> /sys/kernel/irq/7/per_cpu_count:0,0,0,0,0,5,0,0
> /sys/kernel/irq/7/type:level
> /sys/kernel/irq/7/wakeup:enabled
> 
> # grep . /sys/kernel/irq/102/*
> /sys/kernel/irq/102/actions:power
> /sys/kernel/irq/102/chip_name:amd_gpio
> /sys/kernel/irq/102/hwirq:0
> /sys/kernel/irq/102/per_cpu_count:0,1,0,2,1,0,0,1
> /sys/kernel/irq/102/type:edge
> /sys/kernel/irq/102/wakeup:disabled

Ah, right.

But thinking more about this I do not think believe
that wakeup racing is really a big issue here.

Wakeup racing only hits if the button ISR runs before
gpio_keys_resume() has run and cleared the bdata->suspended
flag. IOW the button was pressed before the system has
completely resumed in that case the users intend to me
very clearly was to wakeup the system.

So I still believe that sending key-wakeup for the simulated
keypress is the right thing to do in wakeup race cases even
if the system was actually woken up by e.g. network traffic.

As for Mario's patch from earlier in the thread that needs
some more work because it will release the wrong code if
the release ISR runs after gpio_keys_resume().

But working further on that only is useful if we can get
agreement from Dmitry on that approach.

Regards,

Hans

Powered by blists - more mailing lists