[PATCH] Input: twl4030 power button: don't lose presses on resume

[PATCH] Input: twl4030 power button: don't lose presses on resume

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]


If we press and release the power button before the press interrupt is
handled - as can happen on resume - we lose the press event so the
release event is ignored and we don't know what happened to cause the
wakeup.

So make sure that each interrupt handled does generate an event.
Because twl4030 queues interrupt events we will see two interrupts
for a press-release even if we handle the first one later.  This means
that such a sequence will be reported as two button presses.  This
is unfortunate but is better than no button presses.
Possibly we could set the PENDDIS_MASK to disable queuing of
interrupts, but that might adversely affect other interrupt sources.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index 38e4b50..7ea0ea8 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -42,7 +42,19 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
 	err = twl_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
 				STS_HW_CONDITIONS);
 	if (!err)  {
-		input_report_key(pwr, KEY_POWER, value & PWR_PWRON_IRQ);
+		int val = !!(value & PWR_PWRON_IRQ);
+
+		/* We got an interrupt, so we must see a change.
+		 * Because the TWL4030 queues pending interrupts to a depth
+		 * of 2, we end up seeing two key presses as there can
+		 * be two interrupts processed while the key appears to
+		 * be up.  This could be fixed by setting PNEDDIS_MASK
+		 * in PWR_SIH_CTRL in twl4030-irq.c.
+		 */
+		input_report_key(pwr, KEY_POWER, !val);
+		input_sync(pwr);
+
+		input_report_key(pwr, KEY_POWER, val);
 		input_sync(pwr);
 	} else {
 		dev_err(pwr->dev.parent, "twl4030: i2c error %d while reading"

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[Dmitry Torokhov]

Hi Neil,

On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
> 
> If we press and release the power button before the press interrupt is
> handled - as can happen on resume - we lose the press event so the
> release event is ignored and we don't know what happened to cause the
> wakeup.

What kind of latency do you observe?

> 
> So make sure that each interrupt handled does generate an event.
> Because twl4030 queues interrupt events we will see two interrupts
> for a press-release even if we handle the first one later.  This means
> that such a sequence will be reported as two button presses.  This
> is unfortunate but is better than no button presses.
> Possibly we could set the PENDDIS_MASK to disable queuing of
> interrupts, but that might adversely affect other interrupt sources.
>

It looks like we'd have to modify every driver to ensure consistent
behavior as we do not have any guarantees on how long resume takes.
Maybe this is something that input core needs to implement?

Thanks.


-- 
Dmitry

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]

On Tue, 24 Apr 2012 22:09:19 -0700 Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Hi Neil,
> 
> On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
> > 
> > If we press and release the power button before the press interrupt is
> > handled - as can happen on resume - we lose the press event so the
> > release event is ignored and we don't know what happened to cause the
> > wakeup.
> 
> What kind of latency do you observe?

When I have debugging enabled, hundreds of milliseconds.

When I don't have debugging enabled ... it doesn't tell me, but I'm fairly
sure it is several tens of milliseconds and the button press can be quicker
than that.

If it will help I can try to instrument the driver and get some timings.

> 
> > 
> > So make sure that each interrupt handled does generate an event.
> > Because twl4030 queues interrupt events we will see two interrupts
> > for a press-release even if we handle the first one later.  This means
> > that such a sequence will be reported as two button presses.  This
> > is unfortunate but is better than no button presses.
> > Possibly we could set the PENDDIS_MASK to disable queuing of
> > interrupts, but that might adversely affect other interrupt sources.
> >
> 
> It looks like we'd have to modify every driver to ensure consistent
> behavior as we do not have any guarantees on how long resume takes.
> Maybe this is something that input core needs to implement?

Well if every driver is buggy....

I don't see how this could be implemented in the input core.  And even if it
was, you'd probably need to change each driver to interact with this new
functionality which would be much the same work as changing them to work with
the current functionality....
But maybe I have no imagination - if you can suggest a way that the input core
could support this without changing the drivers, I'm happy to try it out.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[anish kumar]

On Wed, 2012-04-25 at 15:26 +1000, NeilBrown wrote:
> On Tue, 24 Apr 2012 22:09:19 -0700 Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > Hi Neil,
> > 
> > On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
> > > 
> > > If we press and release the power button before the press interrupt is
> > > handled - as can happen on resume - we lose the press event so the
> > > release event is ignored and we don't know what happened to cause the
> > > wakeup.
I didn't understand this.If power button is waking up the device then
obviously power button interrupt handler is called right?If yes then how
can we lose the press event?Is user space not ready to take the event?
May be I didn't understand it properly.Can you kindly explain?
> > 
> > What kind of latency do you observe?
> 
> When I have debugging enabled, hundreds of milliseconds.
> 
> When I don't have debugging enabled ... it doesn't tell me, but I'm fairly
> sure it is several tens of milliseconds and the button press can be quicker
> than that.
> 
> If it will help I can try to instrument the driver and get some timings.
> 
> > 
> > > 
> > > So make sure that each interrupt handled does generate an event.
> > > Because twl4030 queues interrupt events we will see two interrupts
> > > for a press-release even if we handle the first one later.  This means
> > > that such a sequence will be reported as two button presses.  This
> > > is unfortunate but is better than no button presses.
> > > Possibly we could set the PENDDIS_MASK to disable queuing of
> > > interrupts, but that might adversely affect other interrupt sources.
> > >
> > 
> > It looks like we'd have to modify every driver to ensure consistent
> > behavior as we do not have any guarantees on how long resume takes.
> > Maybe this is something that input core needs to implement?
> 
> Well if every driver is buggy....
> 
> I don't see how this could be implemented in the input core.  And even if it
> was, you'd probably need to change each driver to interact with this new
> functionality which would be much the same work as changing them to work with
> the current functionality....
> But maybe I have no imagination - if you can suggest a way that the input core
> could support this without changing the drivers, I'm happy to try it out.
> 
> Thanks,
> NeilBrown

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]

On Wed, 25 Apr 2012 22:26:05 +0530 anish kumar <anish198519851985@gmail.com>
wrote:

> On Wed, 2012-04-25 at 15:26 +1000, NeilBrown wrote:
> > On Tue, 24 Apr 2012 22:09:19 -0700 Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > 
> > > Hi Neil,
> > > 
> > > On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
> > > > 
> > > > If we press and release the power button before the press interrupt is
> > > > handled - as can happen on resume - we lose the press event so the
> > > > release event is ignored and we don't know what happened to cause the
> > > > wakeup.
> I didn't understand this.If power button is waking up the device then
> obviously power button interrupt handler is called right?If yes then how
> can we lose the press event?Is user space not ready to take the event?
> May be I didn't understand it properly.Can you kindly explain?

Yes, the interrupt handler is running.

However the way the driver is currently written, an interrupt is not enough
to generate an "button press" event.
What happens is when the kernel-thread half of the interrupt handler finally
runs, it samples the state of the button and generates an event based on that
level.  So if the button has been released again by the time the ISR runs,
then only a "button release" event is generated.

When this gets to the input layer, the input later *knows* that the button is
currently released so it suppresses the new "button release" event.
A "sync" event does still get through to the App, but they can come at all
sorts of time even when nothing is happening to the button, so they are best
ignored (when by themselves).

What the driver needs to do is acknowledge that just getting an interrupt
means  that something changed, and to ensure that a change gets passed up to
the input layer.

Is that clearer?

Thanks,
NeilBrown


> > > 
> > > What kind of latency do you observe?
> > 
> > When I have debugging enabled, hundreds of milliseconds.
> > 
> > When I don't have debugging enabled ... it doesn't tell me, but I'm fairly
> > sure it is several tens of milliseconds and the button press can be quicker
> > than that.
> > 
> > If it will help I can try to instrument the driver and get some timings.
> > 
> > > 
> > > > 
> > > > So make sure that each interrupt handled does generate an event.
> > > > Because twl4030 queues interrupt events we will see two interrupts
> > > > for a press-release even if we handle the first one later.  This means
> > > > that such a sequence will be reported as two button presses.  This
> > > > is unfortunate but is better than no button presses.
> > > > Possibly we could set the PENDDIS_MASK to disable queuing of
> > > > interrupts, but that might adversely affect other interrupt sources.
> > > >
> > > 
> > > It looks like we'd have to modify every driver to ensure consistent
> > > behavior as we do not have any guarantees on how long resume takes.
> > > Maybe this is something that input core needs to implement?
> > 
> > Well if every driver is buggy....
> > 
> > I don't see how this could be implemented in the input core.  And even if it
> > was, you'd probably need to change each driver to interact with this new
> > functionality which would be much the same work as changing them to work with
> > the current functionality....
> > But maybe I have no imagination - if you can suggest a way that the input core
> > could support this without changing the drivers, I'm happy to try it out.
> > 
> > Thanks,
> > NeilBrown
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[anish singh]

On Thu, Apr 26, 2012 at 2:28 AM, NeilBrown <neilb@suse.de> wrote:
> On Wed, 25 Apr 2012 22:26:05 +0530 anish kumar <anish198519851985@gmail.com>
> wrote:
>
>> On Wed, 2012-04-25 at 15:26 +1000, NeilBrown wrote:
>> > On Tue, 24 Apr 2012 22:09:19 -0700 Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> >
>> > > Hi Neil,
>> > >
>> > > On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
>> > > >
>> > > > If we press and release the power button before the press interrupt is
>> > > > handled - as can happen on resume - we lose the press event so the
>> > > > release event is ignored and we don't know what happened to cause the
>> > > > wakeup.
>> I didn't understand this.If power button is waking up the device then
>> obviously power button interrupt handler is called right?If yes then how
>> can we lose the press event?Is user space not ready to take the event?
>> May be I didn't understand it properly.Can you kindly explain?
>
> Yes, the interrupt handler is running.
>
> However the way the driver is currently written, an interrupt is not enough
> to generate an "button press" event.
> What happens is when the kernel-thread half of the interrupt handler finally
> runs, it samples the state of the button and generates an event based on that
> level.  So if the button has been released again by the time the ISR runs,
> then only a "button release" event is generated.
>
> When this gets to the input layer, the input later *knows* that the button is
> currently released so it suppresses the new "button release" event.
> A "sync" event does still get through to the App, but they can come at all
> sorts of time even when nothing is happening to the button, so they are best
> ignored (when by themselves).
>
> What the driver needs to do is acknowledge that just getting an interrupt
> means  that something changed, and to ensure that a change gets passed up to
> the input layer.
>
> Is that clearer?
Perfect explanation.Thanks a ton.
>
> Thanks,
> NeilBrown
>
>
>> > >
>> > > What kind of latency do you observe?
>> >
>> > When I have debugging enabled, hundreds of milliseconds.
>> >
>> > When I don't have debugging enabled ... it doesn't tell me, but I'm fairly
>> > sure it is several tens of milliseconds and the button press can be quicker
>> > than that.
>> >
>> > If it will help I can try to instrument the driver and get some timings.
>> >
>> > >
>> > > >
>> > > > So make sure that each interrupt handled does generate an event.
>> > > > Because twl4030 queues interrupt events we will see two interrupts
>> > > > for a press-release even if we handle the first one later.  This means
>> > > > that such a sequence will be reported as two button presses.  This
>> > > > is unfortunate but is better than no button presses.
>> > > > Possibly we could set the PENDDIS_MASK to disable queuing of
>> > > > interrupts, but that might adversely affect other interrupt sources.
>> > > >
>> > >
>> > > It looks like we'd have to modify every driver to ensure consistent
>> > > behavior as we do not have any guarantees on how long resume takes.
>> > > Maybe this is something that input core needs to implement?
>> >
>> > Well if every driver is buggy....
>> >
>> > I don't see how this could be implemented in the input core.  And even if it
>> > was, you'd probably need to change each driver to interact with this new
>> > functionality which would be much the same work as changing them to work with
>> > the current functionality....
>> > But maybe I have no imagination - if you can suggest a way that the input core
>> > could support this without changing the drivers, I'm happy to try it out.
>> >
>> > Thanks,
>> > NeilBrown
>>
>

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[anish singh]

On Thu, Apr 26, 2012 at 2:28 AM, NeilBrown <neilb@suse.de> wrote:
> On Wed, 25 Apr 2012 22:26:05 +0530 anish kumar <anish198519851985@gmail.com>
> wrote:
>
>> On Wed, 2012-04-25 at 15:26 +1000, NeilBrown wrote:
>> > On Tue, 24 Apr 2012 22:09:19 -0700 Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> >
>> > > Hi Neil,
>> > >
>> > > On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
>> > > >
>> > > > If we press and release the power button before the press interrupt is
>> > > > handled - as can happen on resume - we lose the press event so the
>> > > > release event is ignored and we don't know what happened to cause the
>> > > > wakeup.
>> I didn't understand this.If power button is waking up the device then
>> obviously power button interrupt handler is called right?If yes then how
>> can we lose the press event?Is user space not ready to take the event?
>> May be I didn't understand it properly.Can you kindly explain?
>
> Yes, the interrupt handler is running.
>
> However the way the driver is currently written, an interrupt is not enough
> to generate an "button press" event.
> What happens is when the kernel-thread half of the interrupt handler finally
> runs, it samples the state of the button and generates an event based on that
> level.  So if the button has been released again by the time the ISR runs,
> then only a "button release" event is generated.
>
> When this gets to the input layer, the input later *knows* that the button is
> currently released so it suppresses the new "button release" event.
> A "sync" event does still get through to the App, but they can come at all
> sorts of time even when nothing is happening to the button, so they are best
> ignored (when by themselves).
>
> What the driver needs to do is acknowledge that just getting an interrupt
> means  that something changed, and to ensure that a change gets passed up to
> the input layer.
>
> Is that clearer?
Perfect explanation.Thanks a ton.
>
> Thanks,
> NeilBrown
>
>
>> > >
>> > > What kind of latency do you observe?
>> >
>> > When I have debugging enabled, hundreds of milliseconds.
>> >
>> > When I don't have debugging enabled ... it doesn't tell me, but I'm fairly
>> > sure it is several tens of milliseconds and the button press can be quicker
>> > than that.
>> >
>> > If it will help I can try to instrument the driver and get some timings.
>> >
>> > >
>> > > >
>> > > > So make sure that each interrupt handled does generate an event.
>> > > > Because twl4030 queues interrupt events we will see two interrupts
>> > > > for a press-release even if we handle the first one later.  This means
>> > > > that such a sequence will be reported as two button presses.  This
>> > > > is unfortunate but is better than no button presses.
>> > > > Possibly we could set the PENDDIS_MASK to disable queuing of
>> > > > interrupts, but that might adversely affect other interrupt sources.
>> > > >
>> > >
>> > > It looks like we'd have to modify every driver to ensure consistent
>> > > behavior as we do not have any guarantees on how long resume takes.
>> > > Maybe this is something that input core needs to implement?
>> >
>> > Well if every driver is buggy....
>> >
>> > I don't see how this could be implemented in the input core.  And even if it
>> > was, you'd probably need to change each driver to interact with this new
>> > functionality which would be much the same work as changing them to work with
>> > the current functionality....
>> > But maybe I have no imagination - if you can suggest a way that the input core
>> > could support this without changing the drivers, I'm happy to try it out.
>> >
>> > Thanks,
>> > NeilBrown
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: twl4030 power button: don't lose presses on resume

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Wed, 25 Apr 2012 15:26:03 +1000 NeilBrown <neilb@suse.de> wrote:

> On Tue, 24 Apr 2012 22:09:19 -0700 Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > Hi Neil,
> > 
> > On Wed, Apr 25, 2012 at 12:21:39PM +1000, NeilBrown wrote:
> > > 
> > > If we press and release the power button before the press interrupt is
> > > handled - as can happen on resume - we lose the press event so the
> > > release event is ignored and we don't know what happened to cause the
> > > wakeup.
> > 
> > What kind of latency do you observe?
> 
> When I have debugging enabled, hundreds of milliseconds.
> 
> When I don't have debugging enabled ... it doesn't tell me, but I'm fairly
> sure it is several tens of milliseconds and the button press can be quicker
> than that.
> 
> If it will help I can try to instrument the driver and get some timings.

I added a bit of tracing.

It looks like a fast button press typically shows about 120ms between 'press'
and 'release', though I have seen as low as 70ms.
When I don't have PM_DEBUG debugging on, the 'press' interrupt gets handled
in about 14ms.
So it seems likely that the times that I suffered from the problem and so
wrote the patch, I had PM_DEBUG enabled which slows resume down quite a lot.

So I cannot make a strong case for these patches.  However I think it is
safer to have them than not :-)

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]