Message-ID: <20130205094033.7ab830fb@notabene.brown>
Date:	Tue, 5 Feb 2013 09:40:33 +1100
From:	NeilBrown <neilb@...e.de>
To:	"ivan.khoronzhuk" <ivan.khoronzhuk@...com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	<linux-kernel@...r.kernel.org>, <linux-input@...r.kernel.org>,
	Bengt Jonsson <bengt.g.jonsson@...ricsson.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Bill Pemberton <wfp5p@...ginia.edu>
Subject: Re: [RFC PATCH] Input: gpio_keys: Fix suspend/resume press event
 lost

On Thu, 31 Jan 2013 15:25:43 +0200 "ivan.khoronzhuk" <ivan.khoronzhuk@...com>
wrote:

> On 01/28/2013 08:51 PM, NeilBrown wrote:
> > On Mon, 28 Jan 2013 18:13:14 +0200 "ivan.khoronzhuk" <ivan.khoronzhuk@...com>
> > wrote:
> >
> >> On 01/22/2013 07:24 AM, NeilBrown wrote:
> >>> On Mon, 21 Jan 2013 15:57:18 -0800 Dmitry Torokhov
> >>> <dmitry.torokhov@...il.com> wrote:
> >>>
> >>>> Hi Ivan,
> >>>>
> >>>> On Mon, Jan 21, 2013 at 03:15:14PM +0200, Ivan Khoronzhuk wrote:
> >>>>> Rebased on linux_omap/master.
> >>>>>
> >>>>> During suspend/resume the key press can be lost if time of resume
> >>>>> sequence is significant.
> >>>>>
> >>>>> If press event cannot be remembered then the driver can read the
> >>>>> current button state only in time of interrupt handling. But in some
> >>>>> cases when time between IRQ and IRQ handler is significant we can
> >>>>> read incorrect state. As a particular case, when device is in suspend
> >>>>> we press wakupable key and up it back in a jiffy, the interrupt
> >>>>> handler read the state of up but the interrupt source is press indeed.
> >>>>> As a result, in a OS like android, we resume then suspend right away
> >>>>> because the key state is not changed.
> >>>>>
> >>>>> This patch add to gpio_keys framework opportunity to recover lost of
> >>>>> press key event at resuming. The variable "key_pressed" from
> >>>>> gpio_button_data structure is not used for gpio keys, it is only used
> >>>>> for gpio irq keys, so it is logically used to remember press lost
> >>>>> while resuming.
> >>>> The same could happen if you delay processing of interrupt long enough
> >>>> during normal operation. If key is released by the time you get around
> >>>> to reading it you will not see a key press.
> >>>>
> >>>> To me this sounds like you need to speed up your resume process so that
> >>>> you can start serving interrupts quicker.
> >>>>
> >>> Agreed.  When I was looking at this I found that any genuine button press
> >>> would have at least 70msec between press and release, while the device could
> >>> wake up to the point of being able to handle interrupts in about 14msec.
> >>> That is enough of a gap to make it pointless to try to 'fix' the code.
> >>>
> >>> With enough verbose debugging enabled that 14msec can easily grow to
> >>> hundreds, but then if  you have debugging enabled to can discipline yourself
> >>> to hold the button for longer.
> >>>
> >>> Ivan: What sort of delay are you seeing between the button press and the
> >>> interrupt routine running?  And can you measure how long the button is
> >>> typically down for?
> >>>
> >>> NeilBrown
> >> In my case I have the delay between the button press and the ISR
> >> about 145ms. If the button down for 120ms the IRQ press event is
> >> lost and if 160ms event is captured. I cannot speed up resume
> >> process enough to guarantee correct work, so I wrote this fix.
> >>
> >
> > 145ms does sound like a long time.
> > You should be able to get precise timings by putting a printk in various
> > parts of the code and ensuring the kernel logs are getting precise timestamps.
> >
> > Then you could see if the delay is between resume starting and the ISR
> > running, or between the ISR and the gpio_work_func getting scheduled.
> >
> > I assume that you don't have ->debounce_interval set....
> >
> > If the ISR is running soon enough, it might be possible to read the GPIO from
> > the ISR - I think that would be a cleaner solution.
> >
> > If not, then you will need something that interpolates an extra key press.
> > In that case I would suggest my original patch - it seems simpler than yours
> > and doesn't make a special case out of suspend.  As Dmitry said, any delay
> > could cause a problem.
> > My patch simply ensures that there is at least one button event for each
> > interrupt by generating an extra event for the inverse of the current button
> > position.  If that state had already been noticed, the input subsystem will
> > filter the extra event out so it won't be visible elsewhere.
> >
> > For reference, my original patch is below.
> >
> > NeilBrown
> >
> > [PATCH] Input: gpio_keys - ensure we don't miss key-presses during resume.
> >
> > If the latency of resume means we don't poll the key status until
> > after it has been released, we can lose the keypress which woke the
> > device.
> >
> > So on each interrupt, record that a press is pending, and in that
> > case, report both the up and down event, ordered such that the second
> > event is that one that reflects the current state.
> >
> > One event will normally be swallowed by the input layer if there was
> > no change, but the result will be that every interrupt will produce at
> > least one event.
> >
> > Signed-off-by: NeilBrown <neilb@...e.de>
> >
> > diff --git a/drivers/input/keyboard/gpio_keys.c
> > b/drivers/input/keyboard/gpio_keys.c index 62bfce4..961e5e1 100644
> > --- a/drivers/input/keyboard/gpio_keys.c
> > +++ b/drivers/input/keyboard/gpio_keys.c
> > @@ -40,6 +40,7 @@ struct gpio_button_data {
> >   	spinlock_t lock;
> >   	bool disabled;
> >   	bool key_pressed;
> > +	bool pending;
> >   };
> >   
> >   struct gpio_keys_drvdata {
> > @@ -335,6 +336,14 @@ static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata) if (state)
> >   			input_event(input, type, button->code, button->value); } else {
> > +		if (type == EV_KEY && bdata->pending) {
> > +			/* Before reporting the observed state, report the
> > +			 * alternate to be sure that a change is seen.
> > +			 */
> > +			bdata->pending = 0;
> > +			input_event(input, type, button->code, !state);
> > +			input_sync(input);
> > +		}
> >   		input_event(input, type, button->code, !!state);
> >   	}Yes it si
> >   	input_sync(input);
> > @@ -361,6 +370,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
> >   	BUG_ON(irq != bdata->irq);
> >   
> > +	bdata->pending = true;
> >   	if (bdata->timer_debounce)
> >   		mod_timer(&bdata->timer,
> >   			jiffies + msecs_to_jiffies(bdata->timer_debounce));
> 
> The delay is between resume starting and the ISR running,
> It is measured with oscilloscope between signal on button pin and
> before irq enabling. And I doubly checked it by printk() right in
> the ISR. So I cannot read the button state right in the ISR.

Wow.  That is a big delay then.

> 
> Your patch is simpler but it doesn't take into account one situation.
> It is unlikely but what if the event is lost at 16:00 and reproduced
> at 17:30, in some cases it is better to lose the event than recover it
> out of time.

Is that at all a realistic scenario?  Interrupts disabled for 90 minutes!  If
that happened you would have more to worry about than one stray button press
(which is still a valid button press, but is a bit late).

I think you are over-complicating things.  Just keep it simple:  Generate at
least one button event on every interrupt.

NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)

Powered by blists - more mailing lists