lkml.org 
[lkml]   [1998]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch] IRQ_INPROGRESS bugfix
On Tue, 22 Dec 1998, Linus Torvalds wrote:

>On Tue, 22 Dec 1998, Andrea Arcangeli wrote:
>>
>> The i386 port had a bug in the IRQ_INPROGRESS flag handling. It was
>> setting the flag even if there was no action and so even if the irq
>> handler would be no executed and so even if the IRQ_INPROGRESS flag will
>> remain set forever.
>
>That's not a bug, it's a feature!

Ah and more bad there was also a bug in my patch ;)...

I tried to find a case in the current code that would race with the
trivial fix applyed, but I can' t, but _only_ because _now_ it' s
completly illegal to do an enable_irq() if there' s not been a
disable_irq() a bit before. Otherwise do_ioapic_edge_IRQ() would race
easily with the old code since the enable_irq() would allow two irq
handlers to run at the same time since it would cleanup the IRQ_INPROGRESS
flag in the irq_desc struct allowing a new shared irq handler to run at
the same time.

The reason I fixed the bug (and in pre-2.1.132-4 there' s really a bug) in
my way is because I consider it the right one. I was aware that clearing
out IRQ_INPROGRESS in enable_irq() was fixing the thing, it' s the first
thing I done some seconds I noticed that the ethernet card was not getting
irqs anymore.

I think my fix is more correct and make more sense to me, because I think
it's the natural way to handle IRQ_INPROGRESS. When there's an irq running
IRQ_INPROGRESS it' s set and when there aren' t it's clear. And it's
set/unset from the irqhandler itself. I see this way more clean. So far
the updated patch runs fine here.

I repost the fixed patch in the case you'll eventually agree with my
arguments.

Index: linux/arch/i386/kernel/irq.c
diff -u linux/arch/i386/kernel/irq.c:1.1.1.3 linux/arch/i386/kernel/irq.c:1.1.1.1.2.10
--- linux/arch/i386/kernel/irq.c:1.1.1.3 Sun Dec 20 16:26:03 1998
+++ linux/arch/i386/kernel/irq.c Wed Dec 23 01:29:43 1998
@@ -15,6 +15,12 @@
* Naturally it's not a 1:1 relation, but there are similarities.
*/

+/*
+ * Cleaned up the IRQ_INPROGRESS handling. IRQ_INPROGRESS now is set only
+ * if there's really an irq handler running or we are probing for irqs.
+ * Copyright (C) 1998 Andrea Arcangeli
+ */
+
#include <linux/ptrace.h>
#include <linux/errno.h>
#include <linux/kernel_stat.h>
@@ -659,8 +665,8 @@
status = desc->status & ~IRQ_REPLAY;
action = NULL;
if (!(status & (IRQ_DISABLED | IRQ_INPROGRESS)))
- action = desc->action;
- desc->status = status | IRQ_INPROGRESS;
+ if ((action = desc->action) || status & IRQ_AUTODETECT)
+ desc->status = status | IRQ_INPROGRESS;
}
spin_unlock(&irq_controller_lock);

@@ -891,7 +897,7 @@
for (i = NR_IRQS-1; i > 0; i--) {
if (!irq_desc[i].action) {
unsigned int status = irq_desc[i].status | IRQ_AUTODETECT;
- irq_desc[i].status = status & ~IRQ_INPROGRESS;
+ irq_desc[i].status = status & ~(IRQ_INPROGRESS|IRQ_DISABLED);
irq_desc[i].handler->startup(i);
}
}
@@ -915,7 +921,7 @@

/* It triggered already - consider it spurious. */
if (status & IRQ_INPROGRESS) {
- irq_desc[i].status = status & ~IRQ_AUTODETECT;
+ irq_desc[i].status = (status & ~IRQ_AUTODETECT) | IRQ_DISABLED;
irq_desc[i].handler->shutdown(i);
}
}
@@ -944,8 +950,9 @@
if (!nr_irqs)
irq_found = i;
nr_irqs++;
+ status &= ~IRQ_INPROGRESS;
}
- irq_desc[i].status = status & ~IRQ_AUTODETECT;
+ irq_desc[i].status = (status & ~IRQ_AUTODETECT) | IRQ_DISABLED;
irq_desc[i].handler->shutdown(i);
}
spin_unlock_irq(&irq_controller_lock);
Index: linux/arch/i386/kernel/io_apic.c
diff -u linux/arch/i386/kernel/io_apic.c:1.1.1.2 linux/arch/i386/kernel/io_apic.c:1.1.1.1.2.10
--- linux/arch/i386/kernel/io_apic.c:1.1.1.2 Sun Dec 20 16:26:03 1998
+++ linux/arch/i386/kernel/io_apic.c Wed Dec 23 01:29:43 1998
@@ -7,6 +7,12 @@
* patches and reporting/debugging problems patiently!
*/

+/*
+ * Cleaned up the IRQ_INPROGRESS handling. IRQ_INPROGRESS now is set only
+ * if there's really an irq handler running or we are probing for irqs.
+ * Copyright (C) 1998 Andrea Arcangeli
+ */
+
#include <linux/sched.h>
#include <linux/smp_lock.h>
#include <linux/init.h>
@@ -1026,10 +1032,13 @@
*/
action = NULL;
if (!(status & (IRQ_DISABLED | IRQ_INPROGRESS))) {
- action = desc->action;
- status &= ~IRQ_PENDING;
+ if ((action = desc->action) || status & IRQ_AUTODETECT)
+ {
+ status |= IRQ_INPROGRESS;
+ status &= ~IRQ_PENDING;
+ }
}
- desc->status = status | IRQ_INPROGRESS;
+ desc->status = status;
spin_unlock(&irq_controller_lock);

/*
@@ -1079,9 +1088,9 @@
*/
action = NULL;
if (!(status & (IRQ_DISABLED | IRQ_INPROGRESS))) {
- action = desc->action;
+ if ((action = desc->action) || status & IRQ_AUTODETECT)
+ desc->status = status | IRQ_INPROGRESS;
}
- desc->status = status | IRQ_INPROGRESS;

ack_APIC_irq();
spin_unlock(&irq_controller_lock);

Andrea Arcangeli




-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:46    [W:0.310 / U:3.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site