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

[Mario Limonciello]

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.