Interrupt issue in twl4030_keypad

Interrupt issue in twl4030_keypad

[Felipe Contreras]

Hi,

The short version is this: either we revert this patch[1], or we apply
this patch series[2], as well as its essential fixes[3].

The long version is this. There's a synchronization issue with the
current keypad driver and twl core; the irq is marked as handled even
though the thread that is supposed to handle it hasn't run yet, and
since it's clear-on-read, and it hasn't been read, it's detected
again, so they keypad driver receives two interrupt callbacks instead
of one, and in the second one reads nothing from the i2c register, so
a key release is assumed. This makes key-presses as simple as shift+a
impossible.

In other words, it's totally unreliable. This might not be isolated to
the keypard driver, but other "nested" interrupts from twl core that
started using request_threaded_irq prematurely (before it was
supported by the twl core). But at least I haven't tried those.

This patch was applied on 2.6.33, which means all versions before 3.2
are affected, including 3.1.

What do you think about fixing this on stable kernels?

Cheers.

[1] http://article.gmane.org/gmane.linux.kernel/932255
[2] http://article.gmane.org/gmane.linux.ports.arm.omap/59958
[3] http://article.gmane.org/gmane.linux.ports.arm.omap/67333
-- 
Felipe Contreras

Re: Interrupt issue in twl4030_keypad

[Felipe Balbi]

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

Hi,

On Mon, Dec 12, 2011 at 08:30:49PM +0200, Felipe Contreras wrote:
> The short version is this: either we revert this patch[1], or we apply
> this patch series[2], as well as its essential fixes[3].
> 
> The long version is this. There's a synchronization issue with the
> current keypad driver and twl core; the irq is marked as handled even
> though the thread that is supposed to handle it hasn't run yet, and
> since it's clear-on-read, and it hasn't been read, it's detected
> again, so they keypad driver receives two interrupt callbacks instead
> of one, and in the second one reads nothing from the i2c register, so
> a key release is assumed. This makes key-presses as simple as shift+a
> impossible.
> 
> In other words, it's totally unreliable. This might not be isolated to
> the keypard driver, but other "nested" interrupts from twl core that
> started using request_threaded_irq prematurely (before it was
> supported by the twl core). But at least I haven't tried those.
> 
> This patch was applied on 2.6.33, which means all versions before 3.2
> are affected, including 3.1.
> 
> What do you think about fixing this on stable kernels?

I believe Samuel has already applied those to the MFD tree. The funny
part is that those patches were pending on linux-omap for over 2 months
I have refreshed them over and over again and asked for help from other
people to test.

Everything went smooth on my simple beagle board with no keypad and I
couldn't see any issues, unfortunately. Still, 2 months is a whole lot
of time to NAK a patch, but nobody said anything so, of course, Samuel
assumed it was ok and, like I said above, it worked for my simple GPIO
usecase with beagleboard.

Oh well, it won't change anything now, Sam has applied Neil's fixes.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Interrupt issue in twl4030_keypad

[Felipe Contreras]

On Mon, Dec 12, 2011 at 9:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Dec 12, 2011 at 08:30:49PM +0200, Felipe Contreras wrote:
>> The short version is this: either we revert this patch[1], or we apply
>> this patch series[2], as well as its essential fixes[3].
>>
>> The long version is this. There's a synchronization issue with the
>> current keypad driver and twl core; the irq is marked as handled even
>> though the thread that is supposed to handle it hasn't run yet, and
>> since it's clear-on-read, and it hasn't been read, it's detected
>> again, so they keypad driver receives two interrupt callbacks instead
>> of one, and in the second one reads nothing from the i2c register, so
>> a key release is assumed. This makes key-presses as simple as shift+a
>> impossible.
>>
>> In other words, it's totally unreliable. This might not be isolated to
>> the keypard driver, but other "nested" interrupts from twl core that
>> started using request_threaded_irq prematurely (before it was
>> supported by the twl core). But at least I haven't tried those.
>>
>> This patch was applied on 2.6.33, which means all versions before 3.2
>> are affected, including 3.1.
>>
>> What do you think about fixing this on stable kernels?
>
> I believe Samuel has already applied those to the MFD tree.

Yeap, I think Linus' tree is going to be fine (3.2), but I'm trying to
find out what to do with previous kernels: as in, what should be done
for stable trees.

> The funny part is that those patches were pending on linux-omap for over 2
> months I have refreshed them over and over again and asked for help from
> other people to test.

You mean these patches? [1] There's nothing wrong with pushing them
forward, but I'm actually talking about this one [2].

> Everything went smooth on my simple beagle board with no keypad and I
> couldn't see any issues, unfortunately.

That's because you didn't get a single interrupt. You could have added
a BUG() on handle_twl4030_pih() and you still wouldn't have noticed
anything.

> Still, 2 months is a whole lot of time to NAK a patch, but nobody said
> anything so, of course,

It's actually almost 2 years :)

> Samuel assumed it was ok and, like I said above, it worked for my simple GPIO
> usecase with beagleboard.

Well, for 3.2 I think the situation is fine, but that's not what I'm
talking about. About your GPIO tests, are you sure you got the
interrupts through twl4030 irq handling?

Cheers.

[1] http://article.gmane.org/gmane.linux.ports.arm.omap/59958
[2] http://article.gmane.org/gmane.linux.kernel/932255

-- 
Felipe Contreras

Re: Interrupt issue in twl4030_keypad

[Felipe Balbi]

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

Hi,

On Mon, Dec 12, 2011 at 10:55:11PM +0200, Felipe Contreras wrote:
> > Samuel assumed it was ok and, like I said above, it worked for my simple GPIO
> > usecase with beagleboard.
> 
> Well, for 3.2 I think the situation is fine, but that's not what I'm
> talking about. About your GPIO tests, are you sure you got the
> interrupts through twl4030 irq handling?

Card detect GPIO on beagleboard is a TWL GPIO. Checking that
removing/adding the card actually triggers an IRQ, should be enough. No?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Interrupt issue in twl4030_keypad

[Felipe Contreras]

On Mon, Dec 12, 2011 at 11:34 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Dec 12, 2011 at 10:55:11PM +0200, Felipe Contreras wrote:
>> > Samuel assumed it was ok and, like I said above, it worked for my simple GPIO
>> > usecase with beagleboard.
>>
>> Well, for 3.2 I think the situation is fine, but that's not what I'm
>> talking about. About your GPIO tests, are you sure you got the
>> interrupts through twl4030 irq handling?
>
> Card detect GPIO on beagleboard is a TWL GPIO. Checking that
> removing/adding the card actually triggers an IRQ, should be enough. No?

Perhaps the reason that worked is because
drivers/mmc/host/omap_hsmmc.c is not requesting a threaded IRQ, so the
action handler was able to run while in handle_twl4030_pih() was still
running (handle_nested_irq), but everything else that was using
request_threaded_irq() didn't have an action handler, but a
thread_fn(), so they were scheduled to run after handle_twl4030_pih().

That's also why you didn't notice the endless loop while pressing a key.

The lesson to learn from this is; don't mix threaded and non-threaded
irq handling.

-- 
Felipe Contreras

Re: Interrupt issue in twl4030_keypad

[Grazvydas Ignotas]

On Mon, Dec 12, 2011 at 10:55 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Mon, Dec 12, 2011 at 9:12 PM, Felipe Balbi <balbi@ti.com> wrote:
>> On Mon, Dec 12, 2011 at 08:30:49PM +0200, Felipe Contreras wrote:
>>> The short version is this: either we revert this patch[1], or we apply
>>> this patch series[2], as well as its essential fixes[3].
>>>
>>> The long version is this. There's a synchronization issue with the
>>> current keypad driver and twl core; the irq is marked as handled even
>>> though the thread that is supposed to handle it hasn't run yet, and
>>> since it's clear-on-read, and it hasn't been read, it's detected
>>> again, so they keypad driver receives two interrupt callbacks instead
>>> of one, and in the second one reads nothing from the i2c register, so
>>> a key release is assumed. This makes key-presses as simple as shift+a
>>> impossible.
>>>
>>> In other words, it's totally unreliable. This might not be isolated to
>>> the keypard driver, but other "nested" interrupts from twl core that
>>> started using request_threaded_irq prematurely (before it was
>>> supported by the twl core). But at least I haven't tried those.
>>>
>>> This patch was applied on 2.6.33, which means all versions before 3.2
>>> are affected, including 3.1.
>>>
>>> What do you think about fixing this on stable kernels?
>>
>> I believe Samuel has already applied those to the MFD tree.
>
> Yeap, I think Linus' tree is going to be fine (3.2), but I'm trying to
> find out what to do with previous kernels: as in, what should be done
> for stable trees.

I only see this in his -next branch, so 3.2 is still affected.


-- 
Gražvydas
--
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: Interrupt issue in twl4030_keypad

[Greg KH]

On Mon, Dec 12, 2011 at 08:30:49PM +0200, Felipe Contreras wrote:
> Hi,
> 
> The short version is this: either we revert this patch[1], or we apply
> this patch series[2], as well as its essential fixes[3].
> 
> The long version is this. There's a synchronization issue with the
> current keypad driver and twl core; the irq is marked as handled even
> though the thread that is supposed to handle it hasn't run yet, and
> since it's clear-on-read, and it hasn't been read, it's detected
> again, so they keypad driver receives two interrupt callbacks instead
> of one, and in the second one reads nothing from the i2c register, so
> a key release is assumed. This makes key-presses as simple as shift+a
> impossible.
> 
> In other words, it's totally unreliable. This might not be isolated to
> the keypard driver, but other "nested" interrupts from twl core that
> started using request_threaded_irq prematurely (before it was
> supported by the twl core). But at least I haven't tried those.
> 
> This patch was applied on 2.6.33, which means all versions before 3.2
> are affected, including 3.1.
> 
> What do you think about fixing this on stable kernels?

I think you need to tell me exactly what git commit ids in Linus's tree
that you want to see in the stable kernel releases, which you didn't do
here :(

Also, please use stable@vger.kernel.org, stable@kernel.org has been dead
since August.

thanks,

greg k-h

Re: Interrupt issue in twl4030_keypad

[Felipe Contreras]

On Mon, Dec 12, 2011 at 9:23 PM, Greg KH <gregkh@suse.de> wrote:
> On Mon, Dec 12, 2011 at 08:30:49PM +0200, Felipe Contreras wrote:
>> The short version is this: either we revert this patch[1], or we apply
>> this patch series[2], as well as its essential fixes[3].
>>
>> The long version is this. There's a synchronization issue with the
>> current keypad driver and twl core; the irq is marked as handled even
>> though the thread that is supposed to handle it hasn't run yet, and
>> since it's clear-on-read, and it hasn't been read, it's detected
>> again, so they keypad driver receives two interrupt callbacks instead
>> of one, and in the second one reads nothing from the i2c register, so
>> a key release is assumed. This makes key-presses as simple as shift+a
>> impossible.
>>
>> In other words, it's totally unreliable. This might not be isolated to
>> the keypard driver, but other "nested" interrupts from twl core that
>> started using request_threaded_irq prematurely (before it was
>> supported by the twl core). But at least I haven't tried those.
>>
>> This patch was applied on 2.6.33, which means all versions before 3.2
>> are affected, including 3.1.
>>
>> What do you think about fixing this on stable kernels?
>
> I think you need to tell me exactly what git commit ids in Linus's tree
> that you want to see in the stable kernel releases, which you didn't do
> here :(

All right, I guess that rules out the revert option.

Once Linus merges the latest fixes I can send you the commit ids, and
they will work for 3.1, but I'm not sure for all the other versions
since 2.6.33. The surest thing would be to just revert the patch I
mentioned, which is a relatively small change compared to picking all
those commits.

> Also, please use stable@vger.kernel.org, stable@kernel.org has been dead
> since August.

Ok.

-- 
Felipe Contreras

Re: Interrupt issue in twl4030_keypad

[Greg KH]

On Mon, Dec 12, 2011 at 11:04:35PM +0200, Felipe Contreras wrote:
> On Mon, Dec 12, 2011 at 9:23 PM, Greg KH <gregkh@suse.de> wrote:
> > On Mon, Dec 12, 2011 at 08:30:49PM +0200, Felipe Contreras wrote:
> >> The short version is this: either we revert this patch[1], or we apply
> >> this patch series[2], as well as its essential fixes[3].
> >>
> >> The long version is this. There's a synchronization issue with the
> >> current keypad driver and twl core; the irq is marked as handled even
> >> though the thread that is supposed to handle it hasn't run yet, and
> >> since it's clear-on-read, and it hasn't been read, it's detected
> >> again, so they keypad driver receives two interrupt callbacks instead
> >> of one, and in the second one reads nothing from the i2c register, so
> >> a key release is assumed. This makes key-presses as simple as shift+a
> >> impossible.
> >>
> >> In other words, it's totally unreliable. This might not be isolated to
> >> the keypard driver, but other "nested" interrupts from twl core that
> >> started using request_threaded_irq prematurely (before it was
> >> supported by the twl core). But at least I haven't tried those.
> >>
> >> This patch was applied on 2.6.33, which means all versions before 3.2
> >> are affected, including 3.1.
> >>
> >> What do you think about fixing this on stable kernels?
> >
> > I think you need to tell me exactly what git commit ids in Linus's tree
> > that you want to see in the stable kernel releases, which you didn't do
> > here :(
> 
> All right, I guess that rules out the revert option.
> 
> Once Linus merges the latest fixes I can send you the commit ids, and
> they will work for 3.1, but I'm not sure for all the other versions
> since 2.6.33. The surest thing would be to just revert the patch I
> mentioned, which is a relatively small change compared to picking all
> those commits.

Then it also needs to be reverted in Linus's tree, is it?

And again, you never told me what commit id the offending patch is here,
you just pointed at some random urls, and for the majority of today, I
only had email access, no web access, so I have no idea what you are
referring to.

But, odds are, we should also just backport the needed patches from
Linus's tree, please send the git commit ids of them to
stable@vger.kernel.org when they hit his tree, and the backports if
needed.

thanks,

greg k-h