Messages in this thread Patch in this message | | | Date | Wed, 23 Dec 1998 01:34:26 +0100 (CET) | From | Andrea Arcangeli <> | Subject | Re: [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/
| |