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

[Mario Limonciello <superm1@kernel.org>]

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