> MOLNAR Ingo <mingo@chiara.csoma.elte.hu> writes:
>
> > On 20 Apr 1998, Claus-Justus Heine wrote:
> >
> > > I think I have found a bug in enable_ioapic_irq() in arch/i3886/kernel/irq.c
> >
> > nicely spotted, the current enable_ioapic_irq() code is badly broken. I've
>
> I didn't want to hurt anybody. It is generally a stupid idea to run
> interrupt service routines in a non-interrupt level the way that
> enable_ioapic_irq() did it.
>
> BTW, the enable_ioapic_irq only "enables" the interrupt if
> "disable_irq[irq]" is zero. The UP version of enable_irq()
> unconditionally enables the interrupt. I think these two should match
> -- i.e. behave as much the same as is possible. So, maybe the
> disable_irq() routines shouldn't increment that disable_irq[irq]
> counter, but simply set a flag?
Nice to see that the above was implemented in 2.1.98
I have one further remark, talking about 2.1.98:
If more than a single (i.e. 1) interrupt has accumulated during the
period that disable_ioapic_irq() was active, then do_ioapic_IRQ() will
NEVER AGAIN execute that interrupt.
This is because do_ioapic_IRQ() checks whether irq_events[irq] is zero:
if (!irq_events[irq]++ && !disabled_irq[irq])
should_handle_irq = 1;
otherwise it will not handle the irq but simply increment the variable
irq_events[irq]. Now, if, say, two interrupts have accumulated while
an IRQ was disabled, then irq_events[irq] will have the value
"2". Occasionally enable_ioapic_irq() is called. It then resends the
IRQ and decrements irq_events[irq]:
if (irq_events[irq]) {
if (!ipi_pending[irq]) {
ipi_pending[irq] = 1;
--irq_events[irq];
send_IPI(cpu,IO_APIC_VECTOR(irq));
}
}
That means irq_events[irq] is still different from zero when
do_ioapic_IRQ() is called next time. do_ioapic_IRQ() then still will
not handle the IRQ, and even worse, it NEVER will handle the IRQ
except in the unlikely case that someone calls enable_ioapic_irq()
until irq_events[irq] finally is zero (and if that someone calls the
enable_ioapic_irq()s fast enough so that the interrupt count decreases
faster than the other CPU increases it by calling do_ioapic_IRQ() :-)
My suggestion is to unconditionally handle the IRQ if the
ipi_pending[irq] flag is set which means that somebody tried to enable
the IRQ again by calling enable_ioapic_IRQ(). Patch follows:
--- linux-2.1/arch/i386/kernel/irq.c~ Sat Apr 25 13:52:51 1998
+++ linux-2.1/arch/i386/kernel/irq.c Sat Apr 25 21:19:14 1998
@@ -779,11 +779,15 @@
ack_APIC_irq();
spin_lock(&irq_controller_lock);
- if (ipi_pending[irq])
- ipi_pending[irq] = 0;
if (!irq_events[irq]++ && !disabled_irq[irq])
should_handle_irq = 1;
+
+ if (ipi_pending[irq]) {
+ ipi_pending[irq] = 0;
+ should_handle_irq = !disabled_irq[irq];
+ }
+
hardirq_enter(cpu);
spin_unlock(&irq_controller_lock);
Maybe not very elegant, but should work.
Cheers
Claus
-- Claus-Justus Heine claus@momo.math.rwth-aachen.de http://www-math.math.rwth-aachen.de/~LBFM/clausPGP Public Key: http://www-math.math.rwth-aachen.de/~LBFM/claus/claus-public-key.asc
Ftape - the Linux Floppy Tape Project WWW : http://www-math.math.rwth-aachen.de/~LBFM/claus/ftape Mailing-list: linux-tape@vger.rutgers.edu
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu