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