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

[Mario Limonciello <superm1@kernel.org>]

On 6/27/2025 3:25 PM, Hans de Goede wrote:
> 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.
> 

Right; good catch.  I had thought this was a risk but didn't want to 
over-invest in solving it until we had alignment.  I mostly wanted to 
share it to demonstrate the idea works.