* [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM
@ 2004-08-23 17:10 Takashi Iwai
2004-08-23 17:29 ` Takashi Iwai
2004-08-25 3:45 ` Andrew Morton
0 siblings, 2 replies; 11+ messages in thread
From: Takashi Iwai @ 2004-08-23 17:10 UTC (permalink / raw)
To: linux-kernel; +Cc: Thomas Charbonnel
Hi,
while the recent investation of latency issues, Thomas Charbonne
suggested that there is a long-standing bug in the irq handler.
When the irq is shared, SA_INTERRUPT flag is checked only for the
first registered handler. When it's without SA_INTERRUPT, always
local_irq_enable() is called even if the second or later handler has
SA_INTERRUPT.
Also, handle_IRQ_event() always calls add_interrupt_randomness()
even if the irq is not for the handler with SA_SAMPLE_RANDOM.
This is a performance loss.
The patch below fixes these problems by adding the SA_INTERRUPT
handler always to the head of the irq list, and by checking the return
value of each handler.
The patch is for i386 and x86-64 only. Similar patches will be needed
for other architectures, too (or more better, making an
arch-independent generic handler as in voluntary-preemptive patch).
--
Takashi Iwai <tiwai@suse.de> ALSA Developer - www.alsa-project.org
--- linux/arch/i386/kernel/irq.c 2004-08-18 15:15:18.000000000 +0200
+++ linux/arch/i386/kernel/irq.c 2004-08-20 14:54:14.000000000 +0200
@@ -221,13 +221,21 @@ asmlinkage int handle_IRQ_event(unsigned
{
int status = 1; /* Force the "do bottom halves" bit */
int retval = 0;
-
- if (!(action->flags & SA_INTERRUPT))
- local_irq_enable();
+ int irq_off = 1;
do {
- status |= action->flags;
- retval |= action->handler(irq, action->dev_id, regs);
+ int ret;
+ /* Assume that all SA_INTERRUPT handlers are at the head
+ * of the irq queue
+ */
+ if (irq_off && ! (action->flags & SA_INTERRUPT)) {
+ local_irq_enable();
+ irq_off = 0;
+ }
+ ret = action->handler(irq, action->dev_id, regs);
+ if (ret)
+ status |= action->flags;
+ retval |= ret;
action = action->next;
} while (action);
if (status & SA_SAMPLE_RANDOM)
@@ -955,11 +963,16 @@ int setup_irq(unsigned int irq, struct i
return -EBUSY;
}
- /* add new interrupt at end of irq queue */
- do {
- p = &old->next;
- old = *p;
- } while (old);
+ if (new->flags & SA_INTERRUPT)
+ /* add interrupt at the start of the queue */
+ new->next = old;
+ else
+ /* add new interrupt at end of irq queue */
+ do {
+ p = &old->next;
+ old = *p;
+ } while (old);
+
shared = 1;
}
--- linux/arch/x86_64/kernel/irq.c 2004-08-20 15:04:26.000000000 +0200
+++ linux/arch/x86_64/kernel/irq.c 2004-08-20 15:07:06.000000000 +0200
@@ -213,13 +213,20 @@ inline void synchronize_irq(unsigned int
int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action)
{
int status = 1; /* Force the "do bottom halves" bit */
-
- if (!(action->flags & SA_INTERRUPT))
- local_irq_enable();
+ int irq_off = 1;
do {
- status |= action->flags;
- action->handler(irq, action->dev_id, regs);
+ int ret;
+ /* Assume that all SA_INTERRUPT handlers are at the head
+ * of the irq queue
+ */
+ if (irq_off && ! (action->flags & SA_INTERRUPT)) {
+ local_irq_enable();
+ irq_off = 0;
+ }
+ ret = action->handler(irq, action->dev_id, regs);
+ if (ret)
+ status |= action->flags;
action = action->next;
} while (action);
if (status & SA_SAMPLE_RANDOM)
@@ -794,11 +801,16 @@ int setup_irq(unsigned int irq, struct i
return -EBUSY;
}
- /* add new interrupt at end of irq queue */
- do {
- p = &old->next;
- old = *p;
- } while (old);
+ if (new->flags & SA_INTERRUPT)
+ /* add interrupt at the start of the queue */
+ new->next = old;
+ else
+ /* add new interrupt at end of irq queue */
+ do {
+ p = &old->next;
+ old = *p;
+ } while (old);
+
shared = 1;
}
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-23 17:10 [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM Takashi Iwai @ 2004-08-23 17:29 ` Takashi Iwai 2004-08-23 18:09 ` Zwane Mwaikambo 2004-08-25 3:45 ` Andrew Morton 1 sibling, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2004-08-23 17:29 UTC (permalink / raw) To: linux-kernel; +Cc: Thomas Charbonnel At Mon, 23 Aug 2004 19:10:11 +0200, I wrote: > > The patch below fixes these problems by adding the SA_INTERRUPT > handler always to the head of the irq list, and by checking the return > value of each handler. Forgot to mention that it's to 2.6.8.1. Takashi ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-23 17:29 ` Takashi Iwai @ 2004-08-23 18:09 ` Zwane Mwaikambo 0 siblings, 0 replies; 11+ messages in thread From: Zwane Mwaikambo @ 2004-08-23 18:09 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel, Thomas Charbonnel On Mon, 23 Aug 2004, Takashi Iwai wrote: > At Mon, 23 Aug 2004 19:10:11 +0200, > I wrote: > > > > The patch below fixes these problems by adding the SA_INTERRUPT > > handler always to the head of the irq list, and by checking the return > > value of each handler. > > Forgot to mention that it's to 2.6.8.1. I recall Andrea having it in his 2.4 tree ages ago, i'm not quite sure why it didn't get pushed. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-23 17:10 [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM Takashi Iwai 2004-08-23 17:29 ` Takashi Iwai @ 2004-08-25 3:45 ` Andrew Morton 2004-08-25 11:17 ` Takashi Iwai 1 sibling, 1 reply; 11+ messages in thread From: Andrew Morton @ 2004-08-25 3:45 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel, thomas Takashi Iwai <tiwai@suse.de> wrote: > > while the recent investation of latency issues, Thomas Charbonne > suggested that there is a long-standing bug in the irq handler. > When the irq is shared, SA_INTERRUPT flag is checked only for the > first registered handler. When it's without SA_INTERRUPT, always > local_irq_enable() is called even if the second or later handler has > SA_INTERRUPT. That's because SA_INTERRUPT interrupts shouldn't be shared. The grey cell which remembered why this is so seems to have died, but I've put the email thread here: http://www.zip.com.au/~akpm/linux/patches/stuff/x.txt ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-25 3:45 ` Andrew Morton @ 2004-08-25 11:17 ` Takashi Iwai 2004-08-25 20:41 ` Andrew Morton 0 siblings, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2004-08-25 11:17 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, thomas At Tue, 24 Aug 2004 20:45:08 -0700, Andrew Morton wrote: > > Takashi Iwai <tiwai@suse.de> wrote: > > > > while the recent investation of latency issues, Thomas Charbonne > > suggested that there is a long-standing bug in the irq handler. > > When the irq is shared, SA_INTERRUPT flag is checked only for the > > first registered handler. When it's without SA_INTERRUPT, always > > local_irq_enable() is called even if the second or later handler has > > SA_INTERRUPT. > > That's because SA_INTERRUPT interrupts shouldn't be shared. The grey cell > which remembered why this is so seems to have died, but I've put the email > thread here: http://www.zip.com.au/~akpm/linux/patches/stuff/x.txt Oh thanks that helps to understand what happened. (BTW, regarding the atomicity in that discussion: could the code like rtc.c (assuming it has also SA_SHIRQ) really cause deadlock?) Anyway, suppressing the unnecessary call of add_interrupt_randomness() should be still valid. The reduced patch is below. Takashi --- linux-2.6.8.1/arch/i386/kernel/irq.c-dist 2004-08-25 13:13:05.153227112 +0200 +++ linux-2.6.8.1/arch/i386/kernel/irq.c 2004-08-25 13:13:34.760726088 +0200 @@ -220,14 +220,16 @@ asmlinkage int handle_IRQ_event(unsigned struct pt_regs *regs, struct irqaction *action) { int status = 1; /* Force the "do bottom halves" bit */ - int retval = 0; + int ret, retval = 0; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret) + status |= action->flags; + retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-25 11:17 ` Takashi Iwai @ 2004-08-25 20:41 ` Andrew Morton 2004-08-26 12:50 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Andrew Morton @ 2004-08-25 20:41 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-kernel, thomas Takashi Iwai <tiwai@suse.de> wrote: > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > should be still valid. The reduced patch is below. > > > Takashi > > --- linux-2.6.8.1/arch/i386/kernel/irq.c-dist 2004-08-25 13:13:05.153227112 +0200 > +++ linux-2.6.8.1/arch/i386/kernel/irq.c 2004-08-25 13:13:34.760726088 +0200 > @@ -220,14 +220,16 @@ asmlinkage int handle_IRQ_event(unsigned > struct pt_regs *regs, struct irqaction *action) > { > int status = 1; /* Force the "do bottom halves" bit */ > - int retval = 0; > + int ret, retval = 0; > > if (!(action->flags & SA_INTERRUPT)) > local_irq_enable(); > > do { > - status |= action->flags; > - retval |= action->handler(irq, action->dev_id, regs); > + ret = action->handler(irq, action->dev_id, regs); > + if (ret) > + status |= action->flags; > + retval |= ret; > action = action->next; > } while (action); > if (status & SA_SAMPLE_RANDOM) Shouldn't that be `if (ret == IRQ_HANDLED)'? Probably I'll need to pass this to lots of other architecture maintainers to make the same change. Please write a nice changelog so they understand what your patch does. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-25 20:41 ` Andrew Morton @ 2004-08-26 12:50 ` Takashi Iwai 2004-08-26 14:04 ` Russell King 0 siblings, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2004-08-26 12:50 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, thomas At Wed, 25 Aug 2004 13:41:12 -0700, Andrew Morton wrote: > > Takashi Iwai <tiwai@suse.de> wrote: > > > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > > should be still valid. The reduced patch is below. (snip) > > Shouldn't that be `if (ret == IRQ_HANDLED)'? Yes, it's more strict. > Probably I'll need to pass this to lots of other architecture maintainers > to make the same change. Please write a nice changelog so they understand > what your patch does. Here I patched more architectures. Please pass to arch maintainers if you thinks it's ok. Takashi == [PATCH] Fix the unnecessary entropy call in the irq handler Currently add_interrupt_randomness() is called at each interrupt when one of the handlers has SA_SAMPLE_RANDOM flag, regardless whether the interrupt is processed by that handler or not. This results in the higher latency and perfomance loss. The patch fixes this behavior to avoid the unnecessary call by checking the return value from each handler. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- linux-2.6.8.1/arch/sh/kernel/irq.c-dist 2004-08-26 14:20:41.536919578 +0200 +++ linux-2.6.8.1/arch/sh/kernel/irq.c 2004-08-26 14:21:00.638579701 +0200 @@ -137,14 +137,16 @@ unlock: int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action) { int status = 1; /* Force the "do bottom halves" bit */ - int retval = 0; + int ret, retval = 0; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; + retval |= ret; action = action->next; } while (action); --- linux-2.6.8.1/arch/um/kernel/irq.c-dist 2004-08-26 14:22:40.692851759 +0200 +++ linux-2.6.8.1/arch/um/kernel/irq.c 2004-08-26 14:23:03.695627509 +0200 @@ -155,13 +155,15 @@ int handle_IRQ_event(unsigned int irq, s struct irqaction * action) { int status = 1; /* Force the "do bottom halves" bit */ + int ret; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/arm/kernel/irq.c-dist 2004-08-26 12:52:34.666247196 +0200 +++ linux-2.6.8.1/arch/arm/kernel/irq.c 2004-08-26 12:53:31.595279800 +0200 @@ -261,7 +261,7 @@ static int __do_irq(unsigned int irq, struct irqaction *action, struct pt_regs *regs) { unsigned int status; - int retval = 0; + int ret, retval = 0; spin_unlock(&irq_controller_lock); @@ -270,8 +270,10 @@ __do_irq(unsigned int irq, struct irqact status = 0; do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; + retval |= ret; action = action->next; } while (action); --- linux-2.6.8.1/arch/ppc/kernel/irq.c-dist 2004-08-26 14:19:28.275567074 +0200 +++ linux-2.6.8.1/arch/ppc/kernel/irq.c 2004-08-26 14:19:50.988405487 +0200 @@ -414,13 +414,15 @@ static inline void handle_irq_event(int irq, struct pt_regs *regs, struct irqaction *action) { int status = 0; + int ret; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/i386/kernel/irq.c-dist 2004-08-25 13:13:05.000000000 +0200 +++ linux-2.6.8.1/arch/i386/kernel/irq.c 2004-08-26 12:48:25.203108557 +0200 @@ -220,14 +220,16 @@ asmlinkage int handle_IRQ_event(unsigned struct pt_regs *regs, struct irqaction *action) { int status = 1; /* Force the "do bottom halves" bit */ - int retval = 0; + int ret, retval = 0; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; + retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/cris/kernel/irq.c-dist 2004-08-26 14:14:40.384855612 +0200 +++ linux-2.6.8.1/arch/cris/kernel/irq.c 2004-08-26 14:15:14.701072531 +0200 @@ -125,7 +125,7 @@ asmlinkage void do_IRQ(int irq, struct p { struct irqaction *action; int do_random, cpu; - int retval = 0; + int ret, retval = 0; cpu = smp_processor_id(); irq_enter(); @@ -137,8 +137,10 @@ asmlinkage void do_IRQ(int irq, struct p local_irq_enable(); do_random = 0; do { - do_random |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + do_random |= action->flags; + retval |= ret; action = action->next; } while (action); --- linux-2.6.8.1/arch/ia64/kernel/irq.c-dist 2004-08-26 12:50:25.147796632 +0200 +++ linux-2.6.8.1/arch/ia64/kernel/irq.c 2004-08-26 12:51:06.708311749 +0200 @@ -255,14 +255,16 @@ int handle_IRQ_event(unsigned int irq, struct pt_regs *regs, struct irqaction *action) { int status = 1; /* Force the "do bottom halves" bit */ - int retval = 0; + int ret, retval = 0; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; + retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/mips/baget/irq.c-dist 2004-08-26 14:17:55.830532962 +0200 +++ linux-2.6.8.1/arch/mips/baget/irq.c 2004-08-26 14:18:34.502764418 +0200 @@ -180,7 +180,7 @@ skip: static void do_IRQ(int irq, struct pt_regs * regs) { struct irqaction *action; - int do_random, cpu; + int ret, do_random, cpu; cpu = smp_processor_id(); irq_enter(); @@ -194,8 +194,9 @@ static void do_IRQ(int irq, struct pt_re action = *(irq + irq_action); do_random = 0; do { - do_random |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + do_random |= action->flags; action = action->next; } while (action); if (do_random & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/mips/kernel/irq.c-dist 2004-08-26 14:18:41.770116666 +0200 +++ linux-2.6.8.1/arch/mips/kernel/irq.c 2004-08-26 14:19:08.196125140 +0200 @@ -144,14 +144,16 @@ inline void synchronize_irq(unsigned int int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action) { int status = 1; /* Force the "do bottom halves" bit */ - int retval = 0; + int ret, retval = 0; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; + retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/v850/kernel/irq.c-dist 2004-08-26 14:23:14.792107468 +0200 +++ linux-2.6.8.1/arch/v850/kernel/irq.c 2004-08-26 14:23:34.635601155 +0200 @@ -141,13 +141,15 @@ skip: int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action) { int status = 1; /* Force the "do bottom halves" bit */ + int ret; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/sh64/kernel/irq.c-dist 2004-08-26 14:21:14.716381411 +0200 +++ linux-2.6.8.1/arch/sh64/kernel/irq.c 2004-08-26 14:21:36.941332490 +0200 @@ -148,6 +148,7 @@ asmlinkage void do_NMI(unsigned long vec int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action) { int status; + int ret; status = 1; /* Force the "do bottom halves" bit */ @@ -155,8 +156,9 @@ int handle_IRQ_event(unsigned int irq, s local_irq_enable(); do { - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/alpha/kernel/irq.c-dist 2004-08-26 12:51:34.133057570 +0200 +++ linux-2.6.8.1/arch/alpha/kernel/irq.c 2004-08-26 12:52:02.341618752 +0200 @@ -83,6 +83,7 @@ handle_IRQ_event(unsigned int irq, struc struct irqaction *action) { int status = 1; /* Force the "do bottom halves" bit */ + int ret; do { if (!(action->flags & SA_INTERRUPT)) @@ -90,8 +91,9 @@ handle_IRQ_event(unsigned int irq, struc else local_irq_disable(); - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/arm26/kernel/irq.c-dist 2004-08-26 14:13:44.808461520 +0200 +++ linux-2.6.8.1/arch/arm26/kernel/irq.c 2004-08-26 14:14:08.377115505 +0200 @@ -187,6 +187,7 @@ static void __do_irq(unsigned int irq, struct irqaction *action, struct pt_regs *regs) { unsigned int status; + int ret; spin_unlock(&irq_controller_lock); if (!(action->flags & SA_INTERRUPT)) @@ -194,8 +195,9 @@ __do_irq(unsigned int irq, struct irqact status = 0; do { - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); --- linux-2.6.8.1/arch/ppc64/kernel/irq.c-dist 2004-08-26 14:20:00.603220604 +0200 +++ linux-2.6.8.1/arch/ppc64/kernel/irq.c 2004-08-26 14:20:24.840713164 +0200 @@ -362,14 +362,16 @@ skip: int handle_irq_event(int irq, struct pt_regs *regs, struct irqaction *action) { int status = 0; - int retval = 0; + int ret, retval = 0; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - retval |= action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; + retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) --- linux-2.6.8.1/arch/x86_64/kernel/irq.c-dist 2004-08-26 12:48:38.518091949 +0200 +++ linux-2.6.8.1/arch/x86_64/kernel/irq.c 2004-08-26 12:51:19.752337402 +0200 @@ -213,13 +213,15 @@ inline void synchronize_irq(unsigned int int handle_IRQ_event(unsigned int irq, struct pt_regs * regs, struct irqaction * action) { int status = 1; /* Force the "do bottom halves" bit */ + int ret; if (!(action->flags & SA_INTERRUPT)) local_irq_enable(); do { - status |= action->flags; - action->handler(irq, action->dev_id, regs); + ret = action->handler(irq, action->dev_id, regs); + if (ret == IRQ_HANDLED) + status |= action->flags; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-26 12:50 ` Takashi Iwai @ 2004-08-26 14:04 ` Russell King 2004-08-26 14:10 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Russell King @ 2004-08-26 14:04 UTC (permalink / raw) To: Takashi Iwai; +Cc: Andrew Morton, linux-kernel, thomas On Thu, Aug 26, 2004 at 02:50:52PM +0200, Takashi Iwai wrote: > At Wed, 25 Aug 2004 13:41:12 -0700, > Andrew Morton wrote: > > > > Takashi Iwai <tiwai@suse.de> wrote: > > > > > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > > > should be still valid. The reduced patch is below. > (snip) > > > > Shouldn't that be `if (ret == IRQ_HANDLED)'? > > Yes, it's more strict. I don't think so. Look at what's going on. If "ret" is IRQ_HANDLED all well and fine. However, look at how "retval" is being used: static void __report_bad_irq(int irq, irq_desc_t *desc, irqreturn_t action_ret) { ... if (action_ret != IRQ_HANDLED && action_ret != IRQ_NONE) { printk(KERN_ERR "irq event %d: bogus return value %x\n", irq, action_ret); } else { printk(KERN_ERR "irq %d: nobody cared!\n", irq); } So, we're looking to see not only if a handler returned IRQ_HANDLED, but also if a handler returned _some other value_ other than IRQ_HANDLED or IRQ_NONE. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/ 2.6 Serial core ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-26 14:04 ` Russell King @ 2004-08-26 14:10 ` Takashi Iwai 2004-08-26 14:18 ` Russell King 0 siblings, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2004-08-26 14:10 UTC (permalink / raw) To: Russell King; +Cc: Andrew Morton, linux-kernel, thomas At Thu, 26 Aug 2004 15:04:04 +0100, Russell King wrote: > > On Thu, Aug 26, 2004 at 02:50:52PM +0200, Takashi Iwai wrote: > > At Wed, 25 Aug 2004 13:41:12 -0700, > > Andrew Morton wrote: > > > > > > Takashi Iwai <tiwai@suse.de> wrote: > > > > > > > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > > > > should be still valid. The reduced patch is below. > > (snip) > > > > > > Shouldn't that be `if (ret == IRQ_HANDLED)'? > > > > Yes, it's more strict. > > I don't think so. Look at what's going on. If "ret" is IRQ_HANDLED > all well and fine. However, look at how "retval" is being used: > > static void __report_bad_irq(int irq, irq_desc_t *desc, irqreturn_t action_ret) > { > ... > if (action_ret != IRQ_HANDLED && action_ret != IRQ_NONE) { > printk(KERN_ERR "irq event %d: bogus return value %x\n", > irq, action_ret); > } else { > printk(KERN_ERR "irq %d: nobody cared!\n", irq); > } > > So, we're looking to see not only if a handler returned IRQ_HANDLED, > but also if a handler returned _some other value_ other than IRQ_HANDLED > or IRQ_NONE. But obviously any other value is invalid as shown above, so we shouldn't take it seriously as the correct return value for triggering add_random_interrupt(). Takashi ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-26 14:10 ` Takashi Iwai @ 2004-08-26 14:18 ` Russell King 2004-08-26 14:27 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Russell King @ 2004-08-26 14:18 UTC (permalink / raw) To: Takashi Iwai; +Cc: Andrew Morton, linux-kernel, thomas On Thu, Aug 26, 2004 at 04:10:54PM +0200, Takashi Iwai wrote: > At Thu, 26 Aug 2004 15:04:04 +0100, > Russell King wrote: > > > > On Thu, Aug 26, 2004 at 02:50:52PM +0200, Takashi Iwai wrote: > > > At Wed, 25 Aug 2004 13:41:12 -0700, > > > Andrew Morton wrote: > > > > > > > > Takashi Iwai <tiwai@suse.de> wrote: > > > > > > > > > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > > > > > should be still valid. The reduced patch is below. > > > (snip) > > > > > > > > Shouldn't that be `if (ret == IRQ_HANDLED)'? > > > > > > Yes, it's more strict. > > > > I don't think so. Look at what's going on. If "ret" is IRQ_HANDLED > > all well and fine. However, look at how "retval" is being used: > > > > static void __report_bad_irq(int irq, irq_desc_t *desc, irqreturn_t action_ret) > > { > > ... > > if (action_ret != IRQ_HANDLED && action_ret != IRQ_NONE) { > > printk(KERN_ERR "irq event %d: bogus return value %x\n", > > irq, action_ret); > > } else { > > printk(KERN_ERR "irq %d: nobody cared!\n", irq); > > } > > > > So, we're looking to see not only if a handler returned IRQ_HANDLED, > > but also if a handler returned _some other value_ other than IRQ_HANDLED > > or IRQ_NONE. > > But obviously any other value is invalid as shown above, so we > shouldn't take it seriously as the correct return value for > triggering add_random_interrupt(). Point is: you're killing the method for detecting when IRQ handlers return bogus return codes since you only look at them when they respond with IRQ_HANDLED. So, you're disabling the "bogus return value" check. Completely. The whole point of the check is to identify bad IRQ handlers so they can be fixed up. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/ 2.6 Serial core ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM 2004-08-26 14:18 ` Russell King @ 2004-08-26 14:27 ` Takashi Iwai 0 siblings, 0 replies; 11+ messages in thread From: Takashi Iwai @ 2004-08-26 14:27 UTC (permalink / raw) To: Russell King; +Cc: Andrew Morton, linux-kernel, thomas At Thu, 26 Aug 2004 15:18:32 +0100, Russell King wrote: > > On Thu, Aug 26, 2004 at 04:10:54PM +0200, Takashi Iwai wrote: > > At Thu, 26 Aug 2004 15:04:04 +0100, > > Russell King wrote: > > > > > > On Thu, Aug 26, 2004 at 02:50:52PM +0200, Takashi Iwai wrote: > > > > At Wed, 25 Aug 2004 13:41:12 -0700, > > > > Andrew Morton wrote: > > > > > > > > > > Takashi Iwai <tiwai@suse.de> wrote: > > > > > > > > > > > > Anyway, suppressing the unnecessary call of add_interrupt_randomness() > > > > > > should be still valid. The reduced patch is below. > > > > (snip) > > > > > > > > > > Shouldn't that be `if (ret == IRQ_HANDLED)'? > > > > > > > > Yes, it's more strict. > > > > > > I don't think so. Look at what's going on. If "ret" is IRQ_HANDLED > > > all well and fine. However, look at how "retval" is being used: > > > > > > static void __report_bad_irq(int irq, irq_desc_t *desc, irqreturn_t action_ret) > > > { > > > ... > > > if (action_ret != IRQ_HANDLED && action_ret != IRQ_NONE) { > > > printk(KERN_ERR "irq event %d: bogus return value %x\n", > > > irq, action_ret); > > > } else { > > > printk(KERN_ERR "irq %d: nobody cared!\n", irq); > > > } > > > > > > So, we're looking to see not only if a handler returned IRQ_HANDLED, > > > but also if a handler returned _some other value_ other than IRQ_HANDLED > > > or IRQ_NONE. > > > > But obviously any other value is invalid as shown above, so we > > shouldn't take it seriously as the correct return value for > > triggering add_random_interrupt(). > > Point is: you're killing the method for detecting when IRQ handlers > return bogus return codes since you only look at them when they > respond with IRQ_HANDLED. > > So, you're disabling the "bogus return value" check. Completely. No, the return value from handle_IRQ_event won't be changed at all by my patch. asmlinkage int handle_IRQ_event(unsigned int irq, ... { ... do { ret = action->handler(irq, action->dev_id, regs); ==> if (ret == IRQ_HANDLED) ==> status |= action->flags; ==> retval |= ret; action = action->next; } while (action); if (status & SA_SAMPLE_RANDOM) add_interrupt_randomness(irq); local_irq_disable(); return retval; } The patch adds the additional check for another variable 'status' which is used only for checking SA_SAMPLE_RANDOM. Takashi ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2004-08-26 14:29 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-08-23 17:10 [PATCH] Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM Takashi Iwai 2004-08-23 17:29 ` Takashi Iwai 2004-08-23 18:09 ` Zwane Mwaikambo 2004-08-25 3:45 ` Andrew Morton 2004-08-25 11:17 ` Takashi Iwai 2004-08-25 20:41 ` Andrew Morton 2004-08-26 12:50 ` Takashi Iwai 2004-08-26 14:04 ` Russell King 2004-08-26 14:10 ` Takashi Iwai 2004-08-26 14:18 ` Russell King 2004-08-26 14:27 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox