[PATCH] Input: gpio-keys - Detect long press events in sleep mode

[PATCH] Input: gpio-keys - Detect long press events in sleep mode

[Hua Li]

Previously, long pressing the gpio key could only detect short press
events and could not report long press events in sleep mode, we need
to recognize long press events in sleep mode and fix this issue.

Signed-off-by: Hua Li <lihua@huaqin.corp-partner.google.com>
---
 drivers/input/keyboard/gpio_keys.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 5c39a217b94c..b546f38ecf8f 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -426,6 +426,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
 			 * handler to run.
 			 */
 			input_report_key(bdata->input, button->code, 1);
+			input_sync(bdata->input);
+			return IRQ_HANDLED;
 		}
 	}
 
-- 
2.17.1

Re: [PATCH] Input: gpio-keys - Detect long press events in sleep mode

[lihua -]

Hello, All linux team members:
      Could you please review this modification as soon as possible?


On Tue, May 6, 2025 at 1:58 PM Hua Li
<lihua@huaqin.corp-partner.google.com> wrote:
>
> Previously, long pressing the gpio key could only detect short press
> events and could not report long press events in sleep mode, we need
> to recognize long press events in sleep mode and fix this issue.
>
> Signed-off-by: Hua Li <lihua@huaqin.corp-partner.google.com>
> ---
>  drivers/input/keyboard/gpio_keys.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 5c39a217b94c..b546f38ecf8f 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -426,6 +426,8 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
>                          * handler to run.
>                          */
>                         input_report_key(bdata->input, button->code, 1);
> +                       input_sync(bdata->input);
> +                       return IRQ_HANDLED;
>                 }
>         }
>
> --
> 2.17.1
>

Re: [PATCH] Input: gpio-keys - Detect long press events in sleep mode

[Andy Shevchenko]

On Mon, May 12, 2025 at 03:57:17PM +0800, lihua - wrote:

First of all, do not top-post!

> Hello, All linux team members:
>       Could you please review this modification as soon as possible?

You even haven't waited for a full week...

If it's an (important) fix, made it look so (Fixes: tag, Cc: stable@, etc).

> On Tue, May 6, 2025 at 1:58 PM Hua Li
> <lihua@huaqin.corp-partner.google.com> wrote:
> >
> > Previously, long pressing the gpio key could only detect short press
> > events and could not report long press events in sleep mode, we need
> > to recognize long press events in sleep mode and fix this issue.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Input: gpio-keys - Detect long press events in sleep mode

[Dmitry Torokhov]

On Mon, May 12, 2025 at 11:50:15AM +0300, Andy Shevchenko wrote:
> On Mon, May 12, 2025 at 03:57:17PM +0800, lihua - wrote:
> 
> First of all, do not top-post!
> 
> > Hello, All linux team members:
> >       Could you please review this modification as soon as possible?
> 
> You even haven't waited for a full week...
> 
> If it's an (important) fix, made it look so (Fixes: tag, Cc: stable@, etc).

I can see that missing input_sync() might hurt, but the patch
description makes no sense. The driver does not really differentiate
between long and sort press.

Please provide a better explanation how missing sync interferes with
your use case.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: gpio-keys - Detect long press events in sleep mode

[Dmitry Torokhov]

On Thu, May 15, 2025 at 11:57:58AM -0700, Dmitry Torokhov wrote:
> On Mon, May 12, 2025 at 11:50:15AM +0300, Andy Shevchenko wrote:
> > On Mon, May 12, 2025 at 03:57:17PM +0800, lihua - wrote:
> > 
> > First of all, do not top-post!
> > 
> > > Hello, All linux team members:
> > >       Could you please review this modification as soon as possible?
> > 
> > You even haven't waited for a full week...
> > 
> > If it's an (important) fix, made it look so (Fixes: tag, Cc: stable@, etc).
> 
> I can see that missing input_sync() might hurt, but the patch
> description makes no sense. The driver does not really differentiate
> between long and sort press.
> 
> Please provide a better explanation how missing sync interferes with
> your use case.

Also simply returning from the interrupt handler is wrong: what happens
if you simulate the key press, but the button is indeed released by the
time the handler runs? What will generate the release? The button will
appear "stuck".

Thanks.

-- 
Dmitry

Re: [PATCH] Input: gpio-keys - Detect long press events in sleep mode

[Dmitry Torokhov]

[ adding back linux-input list... ]

On Mon, May 26, 2025 at 09:54:05AM +0800, lihua - wrote:
> Hi, i thought about it, i don't think the scenario you mentioned would
> exist.

And yet it does...

> If it is convenient, please give a specific scenario so that i can verify
> it.
> 
> In addition, as mentioned before, this linux code logic cannot detect long
> press events in suspend state, which is also a problem in itself.
> Does the community have a repair plan?
> 
> Looking forward to your reply。
> 
> On Mon, May 19, 2025 at 3:55 PM lihua - <
> lihua@huaqin.corp-partner.google.com> wrote:
> 
> > In suspend mode, if the gpio key is not released, two down events will be
> > reported, log as follow:
> >
> > *gpio key keep pressing：*
> > /dev/input/event0: EV_KEY       00fa                 DOWN
> > /dev/input/event0: EV_SYN       SYN_REPORT           00000000
> > /dev/input/event0: EV_KEY       00fa                 UP
> > /dev/input/event0: EV_SYN       SYN_REPORT           00000001

These are not 2 down events. I see one down and one up event here.

> >
> > When the gpio key is pressed, the down and up events are reported
> > immediately.
> > The upper layer monitors the time interval between the down and up events.
> > If the time is greater than 500ms, it is considered a long press and
> > responds to the corresponding function.

OK, so the issue you are having is with timestamp on both EV_KEY events
having the same value? Yes, input_sync() after the synthetic key press
will indeed fix this and you will see distinct timestamps on the press
and release events.

> >
> > But, we found that this phenomenon would not occur in the non-sleeping
> > state.
> >
> > Looking at the code, we think it may be related to follow code:
> >
> > 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);
> > ++ input_sync(bdata->input);
> > ++ return IRQ_HANDLED;

This return is wrong though, you do want to schedule debounced read to
make sure you do not end with key "stuck" if button is released really
quickly.

> > }
> >
> > So ,we try to add this two line and fix it.(maybe it's not correct)
> >
> >
> > In addition, why does gpio key keep pressing report two events in suspend
> > state?

Yo may end up trying to report the press twice (but input core will
"eat" one of them and not report to userspace) in the following
scenario:

- the driver generates synthetic key press when resume() method is
  running
- ISR for the GPIO runs, the button is still pressed, and so the driver
  will report another EV_KEY input event. Because input core knows that
  the key is already pressed it will not deliver this "extra" event to
  userspace
- Key is released, input event is generated.

And please, do not top-post on Linux kernel mailing lists.

> >
> >
> >
> > On Sat, May 17, 2025 at 12:08 AM Dmitry Torokhov <
> > dmitry.torokhov@gmail.com> wrote:
> >
> >> Hi,
> >>
> >> On Fri, May 16, 2025 at 09:39:58AM +0800, lihua - wrote:
> >> > The requirement is to long press the Google key in sleep mode to turn
> >> > on the flashlight, so it is necessary to be able to recognize the long
> >> > press event in sleep mode.
> >> >
> >> > But from the current code design, this requirement cannot be met.
> >>
> >> You have not completed your sentence: "... this requirement cannot be
> >> met" because ...
> >>
> >> You need to explain what exactly is wrong in the current implementation
> >> so that we can evaluate the patch properly. So far I do not really
> >> understand how not calling input_sync() in the GPIO interrupt handler
> >> one more time allegedly allows you to recognize short press but not long
> >> press? There is something missing here.
> >>
> >> As I mentioned before, returning early without scheduling another GPIO
> >> read may result in the key or button being "stuck". Maybe for your
> >> particular userspace it is acceptable, but the driver is used in various
> >> products and we need to be careful not to break them.
> >>
> >> And please do not top-post in your replies.
> >>
> >> >
> >> > On Fri, May 16, 2025 at 2:58 AM Dmitry Torokhov
> >> > <dmitry.torokhov@gmail.com> wrote:
> >> > >
> >> > > On Mon, May 12, 2025 at 11:50:15AM +0300, Andy Shevchenko wrote:
> >> > > > On Mon, May 12, 2025 at 03:57:17PM +0800, lihua - wrote:
> >> > > >
> >> > > > First of all, do not top-post!
> >> > > >
> >> > > > > Hello, All linux team members:
> >> > > > >       Could you please review this modification as soon as
> >> possible?
> >> > > >
> >> > > > You even haven't waited for a full week...
> >> > > >
> >> > > > If it's an (important) fix, made it look so (Fixes: tag, Cc: stable@,
> >> etc).
> >> > >
> >> > > I can see that missing input_sync() might hurt, but the patch
> >> > > description makes no sense. The driver does not really differentiate
> >> > > between long and sort press.
> >> > >
> >> > > Please provide a better explanation how missing sync interferes with
> >> > > your use case.

Thanks.

-- 
Dmitry