__do_IRQ does not check IRQ_DISABLED when IRQ_PER_CPU is set
PATCH 1/1 __do_IRQ does not check IRQ_DISABLED when IRQ_PER_CPU is set
[patch] __do_IRQ does not check IRQ_DISABLED when IRQ_PER_CPU is set
In __do_IRQ(), the normal case is that IRQ_DISABLED is checked and if set the handler (handle_IRQ_event()) is not called.
Earlier in __do_IRQ(), if IRQ_PER_CPU is set the code does not check IRQ_DISABLED and calls the handler even though IRQ_DISABLED is set. This behavior seems unintentional.
One user encountering this behavior is the CPE handler (in arch/ia64/kernel/mca.c). When the CPE handler encounters too many CPEs (such as a solid single bit error), it sets up a polling timer and disables the CPE interrupt (to avoid excessive overhead logging the stream of single bit errors). disable_irq_nosync() is called which sets IRQ_DISABLED. The IRQ_PER_CPU flag was previously set (in ia64_mca_late_init()). The net result is the CPE handler gets called even though it is marked disabled.
If the behavior of not checking IRQ_DISABLED when IRQ_PER_CPU is set is intentional, it would be worthy of a comment describing the intended behavior. disable_irq_nosync() does call chip->disable() to provide a chipset specifiec interface for disabling the interrupt, which avoids this issue when used.
Comments???
--------------------------------------------------------------------
--- kernel/irq/handle.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Index: linus/kernel/irq/handle.c
=================================--- linus.orig/kernel/irq/handle.c 2007-10-30 09:49:26.000000000 -0500
+++ linus/kernel/irq/handle.c 2007-10-30 10:23:52.436719688 -0500
@@ -178,9 +178,11 @@ fastcall unsigned int __do_IRQ(unsigned
*/
if (desc->chip->ack)
desc->chip->ack(irq);
- action_ret = handle_IRQ_event(irq, desc->action);
- if (!noirqdebug)
- note_interrupt(irq, desc, action_ret);
+ if (likely(!(desc->status & IRQ_DISABLED))) {
+ action_ret = handle_IRQ_event(irq, desc->action);
+ if (!noirqdebug)
+ note_interrupt(irq, desc, action_ret);
+ }
desc->chip->end(irq);
return 1;
}
Signature · 2 lines
Russ Anderson, OS RAS/Partitioning Project Lead SGI - Silicon Graphics Inc [email protected]
Quoted 18 lines · On Tue, 30 Oct 2007 11:26:57 -0500 Russ Anderson <[email protected]> wrote:
[patch]
__do_IRQdoes not checkIRQ_DISABLEDwhenIRQ_PER_CPUis setIn
__do_IRQ(), the normal case is thatIRQ_DISABLEDis checked and if set the handler (handle_IRQ_event()) is not called.Earlier in
__do_IRQ(), ifIRQ_PER_CPUis set the code does not checkIRQ_DISABLEDand calls the handler even thoughIRQ_DISABLEDis set. This behavior seems unintentional.One user encountering this behavior is the CPE handler (in
arch/ia64/kernel/mca.c). When the CPE handler encounters too many CPEs (such as a solid single bit error), it sets up a polling timer and disables the CPE interrupt (to avoid excessive overhead logging the stream of single bit errors).disable_irq_nosync()is called which setsIRQ_DISABLED. TheIRQ_PER_CPUflag was previously set (inia64_mca_late_init()). The net result is the CPE handler gets called even though it is marked disabled.
Quoted 8 lines
If the behavior of not checking
IRQ_DISABLEDwhenIRQ_PER_CPUis set is intentional, it would be worthy of a comment describing the intended behavior.disable_irq_nosync()does callchip->disable()to provide a chipset specifiec interface for disabling the interrupt, which avoids this issue when used.Comments???
It looks right to me.
Quoted 14 lines
--------------------------------------------------------------------
--- kernel/irq/handle.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)Index:
linus/kernel/irq/handle.c
=================================> ---linus.orig/kernel/irq/handle.c2007-10-30 09:49:26.000000000 -0500
+++linus/kernel/irq/handle.c2007-10-30 10:23:52.436719688 -0500(quoted diff) +0
-1@@ -178,9 +178,11 @@ fastcall unsigned int __do_IRQ(unsigned */ if (desc->chip->ack) desc->chip->ack(irq); - action_ret = handle_IRQ_event(irq, desc->action);
Review fastcall unsigned int __do_IRQ(unsigned · line 188
Quoted 10 lines
(quoted diff) +5
-2- if (!noirqdebug) - note_interrupt(irq, desc, action_ret); + if (likely(!(desc->status & IRQ_DISABLED))) { + action_ret = handle_IRQ_event(irq, desc->action); + if (!noirqdebug) + note_interrupt(irq, desc, action_ret); + } desc->chip->end(irq); return 1; }
Alas, I can't remember who wrote (and cares about) the IRQ_PER_CPU support.
Oh well.
Quoted 8 lines
One user encountering this behavior is the CPE handler (in
arch/ia64/kernel/mca.c). When the CPE handler encounters too many CPEs (such as a solid single bit error), it sets up a polling timer and disables the CPE interrupt (to avoid excessive overhead logging the stream of single bit errors).disable_irq_nosync()is called which setsIRQ_DISABLED. TheIRQ_PER_CPUflag was previously set (inia64_mca_late_init()). The net result is the CPE handler gets called even though it is marked disabled.
Presumably we are in this situation because there are still some pending CPE interrupts on some cpus when we disable CPE? Or is there a more serious problem that we aren't manage to disable CPE on all cpus properly?
-Tony
Quoted 9 lines · Tony
Quoted 8 lines
One user encountering this behavior is the CPE handler (in
arch/ia64/kernel/mca.c). When the CPE handler encounters too many CPEs (such as a solid single bit error), it sets up a polling timer and disables the CPE interrupt (to avoid excessive overhead logging the stream of single bit errors).disable_irq_nosync()is called which setsIRQ_DISABLED. TheIRQ_PER_CPUflag was previously set (inia64_mca_late_init()). The net result is the CPE handler gets called even though it is marked disabled.
Quoted 4 lines
Presumably we are in this situation because there are still some pending CPE interrupts on some cpus when we disable CPE? Or is there a more serious problem that we aren't manage to disable CPE on all cpus properly?
The latter. If IRQ_PER_CPU is set, IRQ_DISABLED is not checked
in __do_IRQ(), so the handler is always called. It is not a race condition type thing where a few pended interrupts get handled after IRQ_DISABLED is set.
My assumption is that setting IRQ_PER_CPU should not change the behavior of IRQ_DISABLED.
disable_irq_nosync() does call chip->disable() to provide a chipset specific interface for disabling the interrupt. That avoids the issue by having the chipset not issue the interrupt. If a disable handler is required to disable the interrupt, then setting IRQ_DISABLED is not necessary (and misleading).
I think the intended behavior is for chip->disable() to disable the interrupt in the chipset. If, for some reason, the interrupt cannot be disabled in the hardware, the IRQ_DISABLED would prevent the interrupt handler from being called.
Signature · 2 lines
Russ Anderson, OS RAS/Partitioning Project Lead SGI - Silicon Graphics Inc [email protected]