[PATCH] fix return value of i8042_aux_test_irq

[PATCH] fix return value of i8042_aux_test_irq

[Fernando Luis Vázquez Cao]

I made an interesting finding while testing the two patches below.

http://lkml.org/lkml/2007/7/19/685
http://lkml.org/lkml/2007/7/19/687

These patches modify the traditional CONFIG_DEBUG_KERNEL in such a way
that the request_irq prints a warning if after calling the handler it
returned IRQ_HANDLED .

The code looks like this:

int request_irq(unsigned int irq, irq_handler_t handler,
                unsigned long irqflags, const char *devname, void
*dev_id)
.....
        if (irqflags & IRQF_DISABLED) {
                unsigned long flags;

                local_irq_save(flags);
                retval = handler(irq, dev_id);
                local_irq_restore(flags);
        } else
                retval = handler(irq, dev_id);
        if (retval == IRQ_HANDLED) {
                printk(KERN_WARNING
                       "%s (IRQ %d) handled a spurious interrupt\n",
                       devname, irq);
        }
.....

I discovered that i8042_aux_test_irq handles the "fake" interrupt,
which, in principle, is not correct because it obviously isn't a real
interrupt and it could have been a spurious interrupt as well.

The problem is that the interrupt handler unconditionally returns IRQ
handled, which does not seem correct. Anyway I am not very familiar with
this code so I may be missing the whole point. I would appreciate your
comments on this.

Thank you in advance.

Fernando

--

Do not return IRQ_HANDLED when we haven't handle the interrupt.

Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---

diff -urNp linux-2.6.23-rc1-mm1-orig/drivers/input/serio/i8042.c linux-2.6.23-rc1-mm1/drivers/input/serio/i8042.c
--- linux-2.6.23-rc1-mm1-orig/drivers/input/serio/i8042.c	2007-07-26 16:10:11.000000000 +0900
+++ linux-2.6.23-rc1-mm1/drivers/input/serio/i8042.c	2007-07-26 16:05:19.000000000 +0900
@@ -516,6 +516,7 @@ static irqreturn_t __devinit i8042_aux_t
 {
 	unsigned long flags;
 	unsigned char str, data;
+	int ret;
 
 	spin_lock_irqsave(&i8042_lock, flags);
 	str = i8042_read_status();
@@ -524,10 +525,13 @@ static irqreturn_t __devinit i8042_aux_t
 		if (i8042_irq_being_tested &&
 		    data == 0xa5 && (str & I8042_STR_AUXDATA))
 			complete(&i8042_aux_irq_delivered);
+		ret = 1;
 	}
+	else
+		ret = 0;
 	spin_unlock_irqrestore(&i8042_lock, flags);
 
-	return IRQ_HANDLED;
+	return IRQ_RETVAL(ret);
 }
 
 /*

Re: [PATCH] fix return value of i8042_aux_test_irq

[Dmitry Torokhov]

Hi,

On 7/26/07, Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
> I made an interesting finding while testing the two patches below.
>
> http://lkml.org/lkml/2007/7/19/685
> http://lkml.org/lkml/2007/7/19/687
>
> These patches modify the traditional CONFIG_DEBUG_KERNEL in such a way
> that the request_irq prints a warning if after calling the handler it
> returned IRQ_HANDLED .
>
> The code looks like this:
>
> int request_irq(unsigned int irq, irq_handler_t handler,
>                unsigned long irqflags, const char *devname, void
> *dev_id)
> .....
>        if (irqflags & IRQF_DISABLED) {
>                unsigned long flags;
>
>                local_irq_save(flags);
>                retval = handler(irq, dev_id);
>                local_irq_restore(flags);
>        } else
>                retval = handler(irq, dev_id);
>        if (retval == IRQ_HANDLED) {
>                printk(KERN_WARNING
>                       "%s (IRQ %d) handled a spurious interrupt\n",
>                       devname, irq);
>        }
> .....
>
> I discovered that i8042_aux_test_irq handles the "fake" interrupt,
> which, in principle, is not correct because it obviously isn't a real
> interrupt and it could have been a spurious interrupt as well.
>
> The problem is that the interrupt handler unconditionally returns IRQ
> handled, which does not seem correct. Anyway I am not very familiar with
> this code so I may be missing the whole point. I would appreciate your
> comments on this.
>

The handler does handle the interrupt - both status and data registers
are read so from the keyboard controller point of view the interrupt
has been handled even if we happen to discard the data. As far as I
know IRQ12 is never shared by BIOS... Vojtech, do you remember why we
request IRQ12 with IRQF_SHARED?

-- 
Dmitry

Re: [PATCH] fix return value of i8042_aux_test_irq

[Alan Cox]

> The handler does handle the interrupt - both status and data registers
> are read so from the keyboard controller point of view the interrupt
> has been handled even if we happen to discard the data. As far as I
> know IRQ12 is never shared by BIOS... Vojtech, do you remember why we
> request IRQ12 with IRQF_SHARED?

A small number of boxes do share IRQ12 and it was switched to shared for
them. 

The debug check is an *aid* not a definitive proof of a problem

Re: [PATCH] fix return value of i8042_aux_test_irq

[fernando]

On Thu, July 26, 2007 11:10 pm, Alan Cox wrote:
>> The handler does handle the interrupt - both status and data registers
>> are read so from the keyboard controller point of view the interrupt
>> has been handled even if we happen to discard the data. As far as I
>> know IRQ12 is never shared by BIOS... Vojtech, do you remember why we
>> request IRQ12 with IRQF_SHARED?
>
> A small number of boxes do share IRQ12 and it was switched to shared for
> them.
If that is the case interrupt handlers should be able to determine whether
a certain interrupt comes from their respective devices, and return
IRQ_HANDLED or IRQ_NONE accordingly. Returning IRQ_HANDLED unconditionally
when IRQF_SHARED is set seems strange. Is this behavior intended?

Re: [PATCH] fix return value of i8042_aux_test_irq

[Alan Cox]

> > A small number of boxes do share IRQ12 and it was switched to shared for
> > them.
> If that is the case interrupt handlers should be able to determine whether
> a certain interrupt comes from their respective devices, and return
> IRQ_HANDLED or IRQ_NONE accordingly. Returning IRQ_HANDLED unconditionally
> when IRQF_SHARED is set seems strange. Is this behavior intended?

Sometimes you simple can't tell and in those cases you have no choice.

Alan

Re: [PATCH] fix return value of i8042_aux_test_irq

[fernando]

On Fri, July 27, 2007 12:29 am, Alan Cox wrote:
>> > A small number of boxes do share IRQ12 and it was switched to shared
>> for
>> > them.
>> If that is the case interrupt handlers should be able to determine
>> whether
>> a certain interrupt comes from their respective devices, and return
>> IRQ_HANDLED or IRQ_NONE accordingly. Returning IRQ_HANDLED
>> unconditionally
>> when IRQF_SHARED is set seems strange. Is this behavior intended?
>
> Sometimes you simple can't tell and in those cases you have no choice.
As I mentioned in a previous email, i8042_interrupt considers that it
should not handle an interrupt when there is no data to read and,
accordingly, it returns IRQ_NONE in such cases. I was just wondering if we
could follow the same approach to make i8042_aux_test_irq more
IRQF_SHARED-friendly.

 - Fernando

Re: [PATCH] fix return value of i8042_aux_test_irq

[Dmitry Torokhov]

On Thursday 26 July 2007 11:57, fernando@oss.ntt.co.jp wrote:
> On Fri, July 27, 2007 12:29 am, Alan Cox wrote:
> >> > A small number of boxes do share IRQ12 and it was switched to shared
> >> for
> >> > them.
> >> If that is the case interrupt handlers should be able to determine
> >> whether
> >> a certain interrupt comes from their respective devices, and return
> >> IRQ_HANDLED or IRQ_NONE accordingly. Returning IRQ_HANDLED
> >> unconditionally
> >> when IRQF_SHARED is set seems strange. Is this behavior intended?
> >
> > Sometimes you simple can't tell and in those cases you have no choice.
> As I mentioned in a previous email, i8042_interrupt considers that it
> should not handle an interrupt when there is no data to read and,
> accordingly, it returns IRQ_NONE in such cases. I was just wondering if we
> could follow the same approach to make i8042_aux_test_irq more
> IRQF_SHARED-friendly.
>

Yes, you are right. Patch applied to 'for-linus' branch of input tree.

Thank you.

-- 
Dmitry

Re: [PATCH] fix return value of i8042_aux_test_irq

[Vojtech Pavlik]

On Thu, Jul 26, 2007 at 09:54:50AM -0400, Dmitry Torokhov wrote:
> Hi,
> 
> On 7/26/07, Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
> >I made an interesting finding while testing the two patches below.
> >
> >http://lkml.org/lkml/2007/7/19/685
> >http://lkml.org/lkml/2007/7/19/687
> >
> >These patches modify the traditional CONFIG_DEBUG_KERNEL in such a way
> >that the request_irq prints a warning if after calling the handler it
> >returned IRQ_HANDLED .
> >
> >The code looks like this:
> >
> >int request_irq(unsigned int irq, irq_handler_t handler,
> >               unsigned long irqflags, const char *devname, void
> >*dev_id)
> >.....
> >       if (irqflags & IRQF_DISABLED) {
> >               unsigned long flags;
> >
> >               local_irq_save(flags);
> >               retval = handler(irq, dev_id);
> >               local_irq_restore(flags);
> >       } else
> >               retval = handler(irq, dev_id);
> >       if (retval == IRQ_HANDLED) {
> >               printk(KERN_WARNING
> >                      "%s (IRQ %d) handled a spurious interrupt\n",
> >                      devname, irq);
> >       }
> >.....
> >
> >I discovered that i8042_aux_test_irq handles the "fake" interrupt,
> >which, in principle, is not correct because it obviously isn't a real
> >interrupt and it could have been a spurious interrupt as well.
> >
> >The problem is that the interrupt handler unconditionally returns IRQ
> >handled, which does not seem correct. Anyway I am not very familiar with
> >this code so I may be missing the whole point. I would appreciate your
> >comments on this.
> >
> 
> The handler does handle the interrupt - both status and data registers
> are read so from the keyboard controller point of view the interrupt
> has been handled even if we happen to discard the data. As far as I
> know IRQ12 is never shared by BIOS... Vojtech, do you remember why we
> request IRQ12 with IRQF_SHARED?
 
I believe it was needed on PPC.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [PATCH] fix return value of i8042_aux_test_irq

[fernando]

On Thu, July 26, 2007 10:54 pm, Dmitry Torokhov wrote:
> Hi,
>
> On 7/26/07, Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
>> I made an interesting finding while testing the two patches below.
>>
>> http://lkml.org/lkml/2007/7/19/685
>> http://lkml.org/lkml/2007/7/19/687
>>
>> These patches modify the traditional CONFIG_DEBUG_KERNEL in such a way
>> that the request_irq prints a warning if after calling the handler it
>> returned IRQ_HANDLED .
>>
>> The code looks like this:
>>
>> int request_irq(unsigned int irq, irq_handler_t handler,
>>                unsigned long irqflags, const char *devname, void
>> *dev_id)
>> .....
>>        if (irqflags & IRQF_DISABLED) {
>>                unsigned long flags;
>>
>>                local_irq_save(flags);
>>                retval = handler(irq, dev_id);
>>                local_irq_restore(flags);
>>        } else
>>                retval = handler(irq, dev_id);
>>        if (retval == IRQ_HANDLED) {
>>                printk(KERN_WARNING
>>                       "%s (IRQ %d) handled a spurious interrupt\n",
>>                       devname, irq);
>>        }
>> .....
>>
>> I discovered that i8042_aux_test_irq handles the "fake" interrupt,
>> which, in principle, is not correct because it obviously isn't a real
>> interrupt and it could have been a spurious interrupt as well.
>>
>> The problem is that the interrupt handler unconditionally returns IRQ
>> handled, which does not seem correct. Anyway I am not very familiar with
>> this code so I may be missing the whole point. I would appreciate your
>> comments on this.
>>
>
> The handler does handle the interrupt - both status and data registers
> are read so from the keyboard controller point of view the interrupt
> has been handled even if we happen to discard the data. As far as I
> know IRQ12 is never shared by BIOS... Vojtech, do you remember why we
> request IRQ12 with IRQF_SHARED?
Hi Dmitry,

Thank you for the feedback.

Isn't there a way to tell whether the interrupt came from a different
source? If it is not possible the IRQF_SHARED flag does not seem
appropriate to me. If we return IRQ_HANDLED unconditionally we may end up
preventing the right interrupt handler from executing. Am I missing
something?

Fernando

Re: [PATCH] fix return value of i8042_aux_test_irq

[fernando]

On Thu, July 26, 2007 10:54 pm, Dmitry Torokhov wrote:
> Hi,
>
> On 7/26/07, Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> wrote:
>> I made an interesting finding while testing the two patches below.
>>
>> http://lkml.org/lkml/2007/7/19/685
>> http://lkml.org/lkml/2007/7/19/687
>>
>> These patches modify the traditional CONFIG_DEBUG_KERNEL in such a way
>> that the request_irq prints a warning if after calling the handler it
>> returned IRQ_HANDLED .
>>
>> The code looks like this:
>>
>> int request_irq(unsigned int irq, irq_handler_t handler,
>>                unsigned long irqflags, const char *devname, void
>> *dev_id)
>> .....
>>        if (irqflags & IRQF_DISABLED) {
>>                unsigned long flags;
>>
>>                local_irq_save(flags);
>>                retval = handler(irq, dev_id);
>>                local_irq_restore(flags);
>>        } else
>>                retval = handler(irq, dev_id);
>>        if (retval == IRQ_HANDLED) {
>>                printk(KERN_WARNING
>>                       "%s (IRQ %d) handled a spurious interrupt\n",
>>                       devname, irq);
>>        }
>> .....
>>
>> I discovered that i8042_aux_test_irq handles the "fake" interrupt,
>> which, in principle, is not correct because it obviously isn't a real
>> interrupt and it could have been a spurious interrupt as well.
>>
>> The problem is that the interrupt handler unconditionally returns IRQ
>> handled, which does not seem correct. Anyway I am not very familiar with
>> this code so I may be missing the whole point. I would appreciate your
>> comments on this.
>>
>
> The handler does handle the interrupt - both status and data registers
> are read so from the keyboard controller point of view the interrupt
> has been handled even if we happen to discard the data. As far as I
> know IRQ12 is never shared by BIOS... Vojtech, do you remember why we
> request IRQ12 with IRQF_SHARED?
i8042_interrupt returns IRQ_NONE when there is no data to read, which is
considerate towards other devices that may sharing the interrupt line.
Would it make sense to do something similar for i8042_aux_test_irq?

 - Fernando