Skip to contentSkip to conversation

Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM

PATCH In review Takashi Iwai 4 participants 11 messages Aug 23, 2004 – Aug 26, 2004

Subsystems (5)
Arm (32-bit) MIPS SuperH User-mode Linux (UML) +1 more subsystems
lore
Share & tools
lore

Takashi Iwai

author
#0 lore raw

PATCH 1/1 Fix shared interrupt handling of SA_INTERRUPT and SA_SAMPLE_RANDOM

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 <[email protected]> ALSA Developer - www.alsa-project.org

arch/i386/kernel/irq.c +23 -10
@@ -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;
 	}
arch/x86_64/kernel/irq.c +22 -10
@@ -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;
 	}
author
#1 lore raw

At Mon, 23 Aug 2004 19:10:11 +0200,

Quoted 4 lines · 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

#2 lore raw
Quoted 8 lines · Takashi Iwai

At Mon, 23 Aug 2004 19:10:11 +0200,

Quoted 4 lines · 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.

#3 lore raw
In reply to Takashi Iwai
Quoted 7 lines · Takashi Iwai

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

author
#4 lore raw

At Tue, 24 Aug 2004 20:45:08 -0700,

Quoted 10 lines · Andrew Morton
Quoted 7 lines · Takashi Iwai

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.

Quoted 3 lines

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

arch/i386/kernel/irq.c renamed from arch/i386/kernel/irq.c-dist +5 -3
@@ -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)
#5 lore raw
Quoted 19 lines · Takashi Iwai

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();
Quoted 10 lines

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.

author
#6 lore raw

At Wed, 25 Aug 2004 13:41:12 -0700,

Quoted 5 lines · Andrew Morton
Quoted 3 lines · Takashi Iwai

Anyway, suppressing the unnecessary call of add_interrupt_randomness()
should be still valid. The reduced patch is below.

(snip)

Quoted 2 lines

Shouldn't that be `if (ret == IRQ_HANDLED)'?

Yes, it's more strict.

Quoted 3 lines

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 <[email protected]>
arch/sh/kernel/irq.c renamed from arch/sh/kernel/irq.c-dist +5 -3
@@ -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);
arch/um/kernel/irq.c renamed from arch/um/kernel/irq.c-dist +4 -2
@@ -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)
arch/arm/kernel/irq.c renamed from arch/arm/kernel/irq.c-dist +5 -3
@@ -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);
arch/ppc/kernel/irq.c renamed from arch/ppc/kernel/irq.c-dist +4 -2
@@ -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)
arch/i386/kernel/irq.c renamed from arch/i386/kernel/irq.c-dist +5 -3
@@ -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)
arch/cris/kernel/irq.c renamed from arch/cris/kernel/irq.c-dist +5 -3
@@ -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);
arch/ia64/kernel/irq.c renamed from arch/ia64/kernel/irq.c-dist +5 -3
@@ -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)
arch/mips/baget/irq.c renamed from arch/mips/baget/irq.c-dist +4 -3
@@ -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)
arch/mips/kernel/irq.c renamed from arch/mips/kernel/irq.c-dist +5 -3
@@ -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)
arch/v850/kernel/irq.c renamed from arch/v850/kernel/irq.c-dist +4 -2
@@ -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)
arch/sh64/kernel/irq.c renamed from arch/sh64/kernel/irq.c-dist +4 -2
@@ -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)
arch/alpha/kernel/irq.c renamed from arch/alpha/kernel/irq.c-dist +4 -2
@@ -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)
arch/arm26/kernel/irq.c renamed from arch/arm26/kernel/irq.c-dist +4 -2
@@ -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);
arch/ppc64/kernel/irq.c renamed from arch/ppc64/kernel/irq.c-dist +5 -3
@@ -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)
arch/x86_64/kernel/irq.c renamed from arch/x86_64/kernel/irq.c-dist +4 -2
@@ -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)
#7 lore raw
Quoted 5 lines · Takashi Iwai

At Wed, 25 Aug 2004 13:41:12 -0700,

Quoted 3 lines · Andrew Morton
Quoted 1 line · Takashi Iwai
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

Anyway, suppressing the unnecessary call of add_interrupt_randomness()
should be still valid. The reduced patch is below.

(snip)

Quoted 2 lines

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.

Signature · 4 lines
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
author
#8 lore raw

At Thu, 26 Aug 2004 15:04:04 +0100,

Quoted 28 lines · Russell King
Quoted 5 lines · Takashi Iwai

At Wed, 25 Aug 2004 13:41:12 -0700,

Quoted 3 lines · Andrew Morton
Quoted 1 line · Takashi Iwai
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

Anyway, suppressing the unnecessary call of add_interrupt_randomness()
should be still valid. The reduced patch is below.

(snip)

Quoted 2 lines

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);
        }
Quoted 3 lines

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

#9 lore raw
Quoted 30 lines · Takashi Iwai

At Thu, 26 Aug 2004 15:04:04 +0100,

Quoted 28 lines · Russell King
Quoted 5 lines · Takashi Iwai

At Wed, 25 Aug 2004 13:41:12 -0700,

Quoted 3 lines · Andrew Morton
Quoted 1 line · Takashi Iwai
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

Anyway, suppressing the unnecessary call of add_interrupt_randomness()
should be still valid. The reduced patch is below.

(snip)

Quoted 2 lines

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);
        }
Quoted 7 lines
Quoted 3 lines

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.

Signature · 4 lines
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
author
#10 lore raw

At Thu, 26 Aug 2004 15:18:32 +0100,

Quoted 40 lines · Russell King
Quoted 30 lines · Takashi Iwai

At Thu, 26 Aug 2004 15:04:04 +0100,

Quoted 28 lines · Russell King
Quoted 5 lines · Takashi Iwai

At Wed, 25 Aug 2004 13:41:12 -0700,

Quoted 3 lines · Andrew Morton
Quoted 1 line · Takashi Iwai
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

Anyway, suppressing the unnecessary call of add_interrupt_randomness()
should be still valid. The reduced patch is below.

(snip)

Quoted 2 lines

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);
        }
Quoted 7 lines
Quoted 3 lines

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

Quoted 5 lines

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