[PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Bernhard Walle]

On IA64, the timer interrupt is not (always?) zero as it is on x86 platforms.
Also, the timer interrupt is CPU-local. Two things need to be changed to make
the irqpoll option make also working on IA64:

  o Call note_interrupt() also on CPU-local interrupts in __do_IRQ().
  o Set a variable timer_irq to the value of the timer interrupt
    after the timer interrupt has been registered and assigned.

That requires changes in Linux-generic files. The default of timer_irq is 0, so
the patch doesn't break i386/x86_64. However, other platforms also may also
have a timer interrupt non-equal to zero, so they can also use the new
set_timer_interrupt() function.


Signed-off-by: Bernhard Walle <bwalle@suse.de>
Acked-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/ia64/kernel/irq_ia64.c |    6 +++++-
 arch/ia64/kernel/time.c     |    6 +++++-
 include/asm-ia64/hw_irq.h   |    2 +-
 include/linux/irq.h         |    3 +++
 kernel/irq/handle.c         |    2 ++
 kernel/irq/spurious.c       |    9 ++++++++-
 6 files changed, 24 insertions(+), 4 deletions(-)

Index: mainline-msi-init/arch/ia64/kernel/irq_ia64.c
===================================================================
--- mainline-msi-init.orig/arch/ia64/kernel/irq_ia64.c
+++ mainline-msi-init/arch/ia64/kernel/irq_ia64.c
@@ -280,11 +280,12 @@ static struct irqaction resched_irqactio
 };
 #endif
 
-void
+int
 register_percpu_irq (ia64_vector vec, struct irqaction *action)
 {
 	irq_desc_t *desc;
 	unsigned int irq;
+	int first_match = -1;
 
 	for (irq = 0; irq < NR_IRQS; ++irq)
 		if (irq_to_vector(irq) == vec) {
@@ -293,7 +294,10 @@ register_percpu_irq (ia64_vector vec, st
 			desc->chip = &irq_type_ia64_lsapic;
 			if (action)
 				setup_irq(irq, action);
+			first_match = irq;
 		}
+
+	return first_match;
 }
 
 void __init
Index: mainline-msi-init/arch/ia64/kernel/time.c
===================================================================
--- mainline-msi-init.orig/arch/ia64/kernel/time.c
+++ mainline-msi-init/arch/ia64/kernel/time.c
@@ -247,7 +247,11 @@ void __devinit ia64_disable_timer(void)
 void __init
 time_init (void)
 {
-	register_percpu_irq(IA64_TIMER_VECTOR, &timer_irqaction);
+	int timer_irq;
+
+	timer_irq = register_percpu_irq(IA64_TIMER_VECTOR, &timer_irqaction);
+	set_timer_interrupt(timer_irq);
+
 	efi_gettimeofday(&xtime);
 	ia64_init_itm();
 
Index: mainline-msi-init/include/linux/irq.h
===================================================================
--- mainline-msi-init.orig/include/linux/irq.h
+++ mainline-msi-init/include/linux/irq.h
@@ -272,6 +272,9 @@ static inline int irq_balancing_disabled
 /* Handle irq action chains: */
 extern int handle_IRQ_event(unsigned int irq, struct irqaction *action);
 
+/* sets the timer interrupt number for irqpoll handling (kernel/irq/spurious.c) */
+extern void set_timer_interrupt(unsigned int irq);
+
 /*
  * Built-in IRQ handlers for various IRQ types,
  * callable via desc->chip->handle_irq()
Index: mainline-msi-init/kernel/irq/spurious.c
===================================================================
--- mainline-msi-init.orig/kernel/irq/spurious.c
+++ mainline-msi-init/kernel/irq/spurious.c
@@ -12,6 +12,7 @@
 #include <linux/interrupt.h>
 
 static int irqfixup __read_mostly;
+static unsigned int timer_irq __read_mostly;
 
 /*
  * Recovery handler for misrouted interrupts.
@@ -146,7 +147,7 @@ void note_interrupt(unsigned int irq, st
 
 	if (unlikely(irqfixup)) {
 		/* Don't punish working computers */
-		if ((irqfixup == 2 && irq == 0) || action_ret == IRQ_NONE) {
+		if ((irqfixup == 2 && irq == timer_irq) || action_ret == IRQ_NONE) {
 			int ok = misrouted_irq(irq);
 			if (action_ret == IRQ_NONE)
 				desc->irqs_unhandled -= ok;
@@ -174,6 +175,12 @@ void note_interrupt(unsigned int irq, st
 	desc->irqs_unhandled = 0;
 }
 
+void set_timer_interrupt(unsigned int irq)
+{
+	timer_irq = irq;
+}
+EXPORT_SYMBOL_GPL(set_timer_interrupt);
+
 int noirqdebug __read_mostly;
 
 int noirqdebug_setup(char *str)
Index: mainline-msi-init/include/asm-ia64/hw_irq.h
===================================================================
--- mainline-msi-init.orig/include/asm-ia64/hw_irq.h
+++ mainline-msi-init/include/asm-ia64/hw_irq.h
@@ -95,7 +95,7 @@ extern int assign_irq_vector (int irq);	
 extern void free_irq_vector (int vector);
 extern int reserve_irq_vector (int vector);
 extern void ia64_send_ipi (int cpu, int vector, int delivery_mode, int redirect);
-extern void register_percpu_irq (ia64_vector vec, struct irqaction *action);
+extern int register_percpu_irq (ia64_vector vec, struct irqaction *action);
 
 static inline void ia64_resend_irq(unsigned int vector)
 {
Index: mainline-msi-init/kernel/irq/handle.c
===================================================================
--- mainline-msi-init.orig/kernel/irq/handle.c
+++ mainline-msi-init/kernel/irq/handle.c
@@ -180,6 +180,8 @@ 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);
 		desc->chip->end(irq);
 		return 1;
 	}

Re: [PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Andrew Morton]

On Tue, 20 Mar 2007 16:00:27 +0100
Bernhard Walle <bwalle@suse.de> wrote:

> On IA64, the timer interrupt is not (always?) zero as it is on x86 platforms.
> Also, the timer interrupt is CPU-local. Two things need to be changed to make
> the irqpoll option make also working on IA64:
> 
>   o Call note_interrupt() also on CPU-local interrupts in __do_IRQ().
>   o Set a variable timer_irq to the value of the timer interrupt
>     after the timer interrupt has been registered and assigned.
> 
> That requires changes in Linux-generic files. The default of timer_irq is 0, so
> the patch doesn't break i386/x86_64. However, other platforms also may also
> have a timer interrupt non-equal to zero, so they can also use the new
> set_timer_interrupt() function.

Couple of things..

I think the term 'timer_interrupt' is a bit generic-sounding.  Would it be
better to call it irqpoll_interrupt?  After all, some architecture might
want to use, umm, the keyboard interrupt to trigger IRQ polling ;)  

Also, the code presently passes the magic IRQ number into the generic IRQ
code.  I wonder if we'd get a more pleasing result if we were to make the
generic IRQ code call _out_ to the architecture:


diff -puN kernel/irq/spurious.c~a kernel/irq/spurious.c
--- a/kernel/irq/spurious.c~a
+++ a/kernel/irq/spurious.c
@@ -135,6 +135,11 @@ report_bad_irq(unsigned int irq, struct 
 	}
 }
 
+bool __attribute__((weak)) arch_is_irqpoll_irq(unsigned int irq)
+{
+	return false;
+}
+
 void note_interrupt(unsigned int irq, struct irq_desc *desc,
 		    irqreturn_t action_ret)
 {
@@ -146,7 +151,8 @@ void note_interrupt(unsigned int irq, st
 
 	if (unlikely(irqfixup)) {
 		/* Don't punish working computers */
-		if ((irqfixup == 2 && irq == 0) || action_ret == IRQ_NONE) {
+		if ((irqfixup == 2 && arch_is_irqpoll_irq(irq)) ||
+				action_ret == IRQ_NONE) {
 			int ok = misrouted_irq(irq);
 			if (action_ret == IRQ_NONE)
 				desc->irqs_unhandled -= ok;
diff -puN include/linux/irq.h~a include/linux/irq.h
--- a/include/linux/irq.h~a
+++ a/include/linux/irq.h
@@ -314,6 +314,7 @@ static inline void generic_handle_irq(un
 /* Handling of unhandled and spurious interrupts: */
 extern void note_interrupt(unsigned int irq, struct irq_desc *desc,
 			   int action_ret);
+extern bool arch_is_irqpoll_irq(unsigned int irq);
 
 /* Resending of interrupts :*/
 void check_irq_resend(struct irq_desc *desc, unsigned int irq);
_


Then, ia64 can implement arch_is_irqpoll_irq() and it can do whatever it
wants in there.

The __attribute__((weak)) thing adds a little bit of overhead, but I don't
think this is a fastpath?

Re: [PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Thomas Gleixner]

On Thu, 2007-03-22 at 14:09 -0700, Andrew Morton wrote:
> I think the term 'timer_interrupt' is a bit generic-sounding.  Would it be
> better to call it irqpoll_interrupt?  After all, some architecture might
> want to use, umm, the keyboard interrupt to trigger IRQ polling ;)  

Interesting thought, but in general I have to agree.

> Also, the code presently passes the magic IRQ number into the generic IRQ
> code.  I wonder if we'd get a more pleasing result if we were to make the
> generic IRQ code call _out_ to the architecture:

> Then, ia64 can implement arch_is_irqpoll_irq() and it can do whatever it
> wants in there.
> 
> The __attribute__((weak)) thing adds a little bit of overhead, but I don't
> think this is a fastpath?

Well, depends what you consider a fastpath. When noirqdebug == 0, it is
called on every interrupt.

	tglx

Re: [PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Andrew Morton]

On Thu, 22 Mar 2007 22:23:21 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 2007-03-22 at 14:09 -0700, Andrew Morton wrote:
> > I think the term 'timer_interrupt' is a bit generic-sounding.  Would it be
> > better to call it irqpoll_interrupt?  After all, some architecture might
> > want to use, umm, the keyboard interrupt to trigger IRQ polling ;)  
> 
> Interesting thought, but in general I have to agree.
> 
> > Also, the code presently passes the magic IRQ number into the generic IRQ
> > code.  I wonder if we'd get a more pleasing result if we were to make the
> > generic IRQ code call _out_ to the architecture:
> 
> > Then, ia64 can implement arch_is_irqpoll_irq() and it can do whatever it
> > wants in there.
> > 
> > The __attribute__((weak)) thing adds a little bit of overhead, but I don't
> > think this is a fastpath?
> 
> Well, depends what you consider a fastpath. When noirqdebug == 0, it is
> called on every interrupt.
> 

OK, well the alternative is to do

extern bool __arch_irqpoll_irq(unsigned int irq);
#define arch_is_irqpoll_irq(irq)	__arch_is_irqpoll_irq(irq)

in an ia64 header file and then do

#ifndef arch_is_irqpoll_irq
static inline bool arch_is_irqpoll_irq(unsigned irq)
{
	return irq == 0;
}
#endif

in spurious.c

Re: [PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Bernhard Walle]

Hello,

* Thomas Gleixner <tglx@linutronix.de> [2007-03-22 22:23]:
> 
> > Also, the code presently passes the magic IRQ number into the generic IRQ
> > code.  I wonder if we'd get a more pleasing result if we were to make the
> > generic IRQ code call _out_ to the architecture:
> 
> > Then, ia64 can implement arch_is_irqpoll_irq() and it can do whatever it
> > wants in there.
> > 
> > The __attribute__((weak)) thing adds a little bit of overhead, but I don't
> > think this is a fastpath?
> 
> Well, depends what you consider a fastpath. When noirqdebug == 0, it is
> called on every interrupt.

I think that function is only called if irqfixup == 2, because if the
first condition in the expression is wrong, the evaluation of the
expression is aborted in C.


Regards,
  Bernhard

Re: [PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Bernhard Walle]

* Andrew Morton <akpm@linux-foundation.org> [2007-03-22 22:09]:
> On Tue, 20 Mar 2007 16:00:27 +0100
> Bernhard Walle <bwalle@suse.de> wrote:
> 
> > On IA64, the timer interrupt is not (always?) zero as it is on x86 platforms.
> > Also, the timer interrupt is CPU-local. Two things need to be changed to make
> > the irqpoll option make also working on IA64:
> > 
> >   o Call note_interrupt() also on CPU-local interrupts in __do_IRQ().
> >   o Set a variable timer_irq to the value of the timer interrupt
> >     after the timer interrupt has been registered and assigned.
> > 
> > That requires changes in Linux-generic files. The default of timer_irq is 0, so
> > the patch doesn't break i386/x86_64. However, other platforms also may also
> > have a timer interrupt non-equal to zero, so they can also use the new
> > set_timer_interrupt() function.
> 
> Couple of things..
> 
> I think the term 'timer_interrupt' is a bit generic-sounding.  Would it be
> better to call it irqpoll_interrupt?  After all, some architecture might
> want to use, umm, the keyboard interrupt to trigger IRQ polling ;)  

Well, the documentation of irqpoll says that it's called on the timer
interrupt. But maybe also the documentation should be changed. :)

> Also, the code presently passes the magic IRQ number into the generic IRQ
> code.  I wonder if we'd get a more pleasing result if we were to make the
> generic IRQ code call _out_ to the architecture:

I think I have a third solution. There's already IRQF_TIMER, and
that's used and defined in a few architectures already (like Sh), so
why not simply use that here. Maybe we should introduce IRQF_IRQPOLL,
but the concept would be the same. If we don't take care of shared
interrupt handlers (and I think sharing the timer interrupt is a
_very_ bad idea, are there architectures that do this?), the could
would look like this (+ the change in __do_IRQ).

What's your opinion on this approach? Of course, we would have to make
sure that IRQF_TIMER is defined on _every_ architectures, but that
would give us the chance to find out each architecture that also has a
timer interrupt != 0.


Index: mainline-msi-init/arch/ia64/kernel/time.c
===================================================================
--- mainline-msi-init.orig/arch/ia64/kernel/time.c
+++ mainline-msi-init/arch/ia64/kernel/time.c
@@ -235,7 +235,7 @@ ia64_init_itm (void)
 
 static struct irqaction timer_irqaction = {
 	.handler =	timer_interrupt,
-	.flags =	IRQF_DISABLED,
+	.flags =	IRQF_DISABLED | IRQF_TIMER,
 	.name =		"timer"
 };
 
Index: mainline-msi-init/arch/x86_64/kernel/time.c
===================================================================
--- mainline-msi-init.orig/arch/x86_64/kernel/time.c
+++ mainline-msi-init/arch/x86_64/kernel/time.c
@@ -320,7 +320,7 @@ void __init stop_timer_interrupt(void)
 }
 
 static struct irqaction irq0 = {
-	timer_interrupt, IRQF_DISABLED, CPU_MASK_NONE, "timer", NULL, NULL
+	timer_interrupt, IRQF_DISABLED | IRQF_TIMER, CPU_MASK_NONE, "timer", NULL, NULL
 };
 
 void __init time_init(void)
Index: mainline-msi-init/kernel/irq/spurious.c
===================================================================
--- mainline-msi-init.orig/kernel/irq/spurious.c
+++ mainline-msi-init/kernel/irq/spurious.c
@@ -146,7 +146,7 @@ void note_interrupt(unsigned int irq, st
 
 	if (unlikely(irqfixup)) {
 		/* Don't punish working computers */
-		if ((irqfixup == 2 && irq == 0) || action_ret == IRQ_NONE) {
+		if ((irqfixup == 2 && (desc->action->flags & IRQF_TIMER)) || action_ret == IRQ_NONE) {
 			int ok = misrouted_irq(irq);
 			if (action_ret == IRQ_NONE)
 				desc->irqs_unhandled -= ok;

Re: [PATCH] Fix irqpoll on IA64 (timer interrupt != 0)

[Andrew Morton]

On Fri, 23 Mar 2007 00:04:07 +0100
Bernhard Walle <bwalle@suse.de> wrote:

> * Andrew Morton <akpm@linux-foundation.org> [2007-03-22 22:09]:
> > On Tue, 20 Mar 2007 16:00:27 +0100
> > Bernhard Walle <bwalle@suse.de> wrote:
> > 
> > > On IA64, the timer interrupt is not (always?) zero as it is on x86 platforms.
> > > Also, the timer interrupt is CPU-local. Two things need to be changed to make
> > > the irqpoll option make also working on IA64:
> > > 
> > >   o Call note_interrupt() also on CPU-local interrupts in __do_IRQ().
> > >   o Set a variable timer_irq to the value of the timer interrupt
> > >     after the timer interrupt has been registered and assigned.
> > > 
> > > That requires changes in Linux-generic files. The default of timer_irq is 0, so
> > > the patch doesn't break i386/x86_64. However, other platforms also may also
> > > have a timer interrupt non-equal to zero, so they can also use the new
> > > set_timer_interrupt() function.
> > 
> > Couple of things..
> > 
> > I think the term 'timer_interrupt' is a bit generic-sounding.  Would it be
> > better to call it irqpoll_interrupt?  After all, some architecture might
> > want to use, umm, the keyboard interrupt to trigger IRQ polling ;)  
> 
> Well, the documentation of irqpoll says that it's called on the timer
> interrupt. But maybe also the documentation should be changed. :)
> 
> > Also, the code presently passes the magic IRQ number into the generic IRQ
> > code.  I wonder if we'd get a more pleasing result if we were to make the
> > generic IRQ code call _out_ to the architecture:
> 
> I think I have a third solution. There's already IRQF_TIMER, and
> that's used and defined in a few architectures already (like Sh), so
> why not simply use that here. Maybe we should introduce IRQF_IRQPOLL,
> but the concept would be the same. If we don't take care of shared
> interrupt handlers (and I think sharing the timer interrupt is a
> _very_ bad idea, are there architectures that do this?), the could
> would look like this (+ the change in __do_IRQ).
> 
> What's your opinion on this approach? Of course, we would have to make
> sure that IRQF_TIMER is defined on _every_ architectures, but that
> would give us the chance to find out each architecture that also has a
> timer interrupt != 0.
> 
> 
> Index: mainline-msi-init/arch/ia64/kernel/time.c
> ===================================================================
> --- mainline-msi-init.orig/arch/ia64/kernel/time.c
> +++ mainline-msi-init/arch/ia64/kernel/time.c
> @@ -235,7 +235,7 @@ ia64_init_itm (void)
>  
>  static struct irqaction timer_irqaction = {
>  	.handler =	timer_interrupt,
> -	.flags =	IRQF_DISABLED,
> +	.flags =	IRQF_DISABLED | IRQF_TIMER,
>  	.name =		"timer"
>  };
>  
> Index: mainline-msi-init/arch/x86_64/kernel/time.c
> ===================================================================
> --- mainline-msi-init.orig/arch/x86_64/kernel/time.c
> +++ mainline-msi-init/arch/x86_64/kernel/time.c
> @@ -320,7 +320,7 @@ void __init stop_timer_interrupt(void)
>  }
>  
>  static struct irqaction irq0 = {
> -	timer_interrupt, IRQF_DISABLED, CPU_MASK_NONE, "timer", NULL, NULL
> +	timer_interrupt, IRQF_DISABLED | IRQF_TIMER, CPU_MASK_NONE, "timer", NULL, NULL
>  };
>  
>  void __init time_init(void)
> Index: mainline-msi-init/kernel/irq/spurious.c
> ===================================================================
> --- mainline-msi-init.orig/kernel/irq/spurious.c
> +++ mainline-msi-init/kernel/irq/spurious.c
> @@ -146,7 +146,7 @@ void note_interrupt(unsigned int irq, st
>  
>  	if (unlikely(irqfixup)) {
>  		/* Don't punish working computers */
> -		if ((irqfixup == 2 && irq == 0) || action_ret == IRQ_NONE) {
> +		if ((irqfixup == 2 && (desc->action->flags & IRQF_TIMER)) || action_ret == IRQ_NONE) {
>  			int ok = misrouted_irq(irq);
>  			if (action_ret == IRQ_NONE)
>  				desc->irqs_unhandled -= ok;

Seems sane.

Or we could add a new flag specifically for this purpose: IRQF_IRQPOLL_IRQ?