[patch] __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

[Russ Anderson]

[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???

Signed-off-by: Russ Anderson (rja@sgi.com)

--------------------------------------------------------------------
---
 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;
 	}
-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] __do_IRQ does not check IRQ_DISABLED when IRQ_PER_CPU is set

[Andrew Morton]

On Tue, 30 Oct 2007 11:26:57 -0500
Russ Anderson <rja@sgi.com> wrote:

> [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???
> 

It looks right to me.

> 
> --------------------------------------------------------------------
> ---
>  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;
>  	}

Alas, I can't remember who wrote (and cares about) the IRQ_PER_CPU support.
 Oh well.

RE: [patch] __do_IRQ does not check IRQ_DISABLED when IRQ_PER_CPU is set

[Luck, Tony]

> 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.

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

Re: [patch] __do_IRQ does not check IRQ_DISABLED when IRQ_PER_CPU is set

[Russ Anderson]

On Wed, Oct 31, 2007 at 09:20:27AM -0700, Luck, Tony wrote:
> > 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.
> 
> 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.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com