linux-kernel - Re: [PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZpHGSCzckXncmeRv@google.com>
Date: Fri, 12 Jul 2024 17:11:52 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Frank Li <Frank.li@....com>
Cc: Jacky Bai <ping.bai@....com>, Jason Liu <jason.hui.liu@....com>,
	Peng Fan <peng.fan@....com>,
	"open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..." <linux-input@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>, imx@...ts.linux.dev
Subject: Re: [PATCH 1/1] input: bbnsm_pwrkey: Fix missed key press after
 suspend

On Fri, Jul 12, 2024 at 07:17:11PM -0400, Frank Li wrote:
> On Fri, Jul 12, 2024 at 04:04:28PM -0700, Dmitry Torokhov wrote:
> > Hi Frank,
> > 
> > On Fri, Jul 12, 2024 at 06:43:51PM -0400, Frank Li wrote:
> > > From: Jacky Bai <ping.bai@....com>
> > > 
> > > Report input event directly on wakeup to ensure no press event is missed
> > > when resuming from suspend.
> > > 
> > > Signed-off-by: Jacky Bai <ping.bai@....com>
> > > Reviewed-by: Peng Fan <peng.fan@....com>
> > > Acked-by: Jason Liu <jason.hui.liu@....com>
> > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > ---
> > >  drivers/input/misc/nxp-bbnsm-pwrkey.c | 36 +++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/drivers/input/misc/nxp-bbnsm-pwrkey.c b/drivers/input/misc/nxp-bbnsm-pwrkey.c
> > > index 1d99206dd3a8b..9675717ecbdfe 100644
> > > --- a/drivers/input/misc/nxp-bbnsm-pwrkey.c
> > > +++ b/drivers/input/misc/nxp-bbnsm-pwrkey.c
> > > @@ -38,6 +38,7 @@ struct bbnsm_pwrkey {
> > >  	int irq;
> > >  	int keycode;
> > >  	int keystate;  /* 1:pressed */
> > > +	bool suspended;
> > >  	struct timer_list check_timer;
> > >  	struct input_dev *input;
> > >  };
> > > @@ -70,6 +71,7 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id)
> > >  {
> > >  	struct platform_device *pdev = dev_id;
> > >  	struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
> > > +	struct input_dev *input = bbnsm->input;
> > >  	u32 event;
> > >  
> > >  	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event);
> > > @@ -81,6 +83,16 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id)
> > >  	mod_timer(&bbnsm->check_timer,
> > >  		   jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
> > >  
> > > +	/*
> > > +	 * Directly report key event after resume to make sure key press
> > > +	 * event is never missed.
> > > +	 */
> > 
> > How do you know that wakeup was caused by the key press on this device?
> > As far as I can see the driver requests the interrupt as shared, so we
> > could end up in bbnsm_pwrkey_interrupt() even if button was not pressed.
> > 
> 
> In bbnsm_pwrky_interrupt()
> {	...
> 	regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event);                                           
>         	if (!(event & BBNSM_BTN_OFF))                                                               
>                 	return IRQ_NONE;
> 
> 	...
> }
> 
> If wakeup was not caused by pwr key,  irq will do nothing, code will not
> reach to here.

OK, so the case you are trying to fix is when interrupt is delayed so
the key is either already released or it will be released by the time
the normal debouncing interval passes?

I think you want to reset ->suspended in the interrupt handler so that
this block fires at most once per suspend/resume cycle. And maybe move
it before calling mod_timer().

Thanks.

-- 
Dmitry