Good spotting. However, this is actually not a IO-APIC specific bug: it
turns out that this could happen with the old APIC too. Your patch fixes
it only for the IO-APIC case.
The bug also makes it clear that the in-progess disable really is
different from the "explicit" disable. I've got a patch that clearly
separates the two (both for the IO-APIC and the old APIC). For the usual
reasons I haven't tested this yet, but I'll append a patch (relative to
clean 2.1.98) that people can play around with.
Whether we want to nest the explicit disable/enable or not is a secondary
issue. The nesting makes sense for irq sharing, but I have to admit that
I'm still a bit nervous about changing the logic, but at least this patch
is a start towards making the nesting happen the same way on the IO-APIC
as on the old APIC if we decide we do want to nest. I refuse to nest on
one but not the other - that just makes no sense at all.
[ This patch does not yet nest - the disable count is still just a single
bit. But this patch allows for two things: checking "wait_for_irq()" on
a per-irq basis by looking at IRQ_INPROGRESS and thus allowing us a more
fine-grained wait and allowing us to do irq autodetection by looking at
the same bit without actually having to do the ugly irq counting stuff.
The patch actually does neither of these two things, but I'll get around
to doing them soon unless somebody else beats me to it ]
Comments?
Linus
-----
--- v2.1.98/linux/arch/i386/kernel/irq.c Sat Apr 25 18:13:10 1998
+++ linux/arch/i386/kernel/irq.c Wed Apr 29 18:53:09 1998
@@ -70,9 +70,14 @@
static unsigned int irq_events [NR_IRQS] = { -1, };
static int disabled_irq [NR_IRQS] = { 0, };
-#ifdef __SMP__
-static int ipi_pending [NR_IRQS] = { 0, };
-#endif
+
+/*
+ * Reasons for being disabled: somebody has done
+ * a "disable_irq()" or we must not re-enter the
+ * already executing irq..
+ */
+#define IRQ_INPROGRESS 1
+#define IRQ_DISABLED 2
/*
* Not all IRQs can be routed through the IO-APIC, eg. on certain (older)
@@ -177,6 +182,7 @@
void unmask_generic_irq(unsigned int irq)
{
+ disabled_irq[irq] = 0;
if (IO_APIC_IRQ(irq))
enable_IO_APIC_irq(irq);
else {
@@ -651,18 +658,6 @@
return status;
}
-
-void disable_irq(unsigned int irq)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&irq_controller_lock, flags);
- irq_handles[irq]->disable(irq);
- spin_unlock_irqrestore(&irq_controller_lock, flags);
-
- synchronize_irq();
-}
-
/*
* disable/enable_irq() wait for all irq contexts to finish
* executing. Also it's recursive.
@@ -673,55 +668,10 @@
set_8259A_irq_mask(irq);
}
-#ifdef __SMP__
-static void disable_ioapic_irq(unsigned int irq)
-{
- disabled_irq[irq] = 1;
- /*
- * We do not disable IO-APIC irqs in hardware ...
- */
-}
-#endif
-
void enable_8259A_irq (unsigned int irq)
{
- unsigned long flags;
- spin_lock_irqsave(&irq_controller_lock, flags);
cached_irq_mask &= ~(1 << irq);
set_8259A_irq_mask(irq);
- spin_unlock_irqrestore(&irq_controller_lock, flags);
-}
-
-#ifdef __SMP__
-void enable_ioapic_irq (unsigned int irq)
-{
- unsigned long flags, should_handle_irq;
- int cpu = smp_processor_id();
-
- spin_lock_irqsave(&irq_controller_lock, flags);
- disabled_irq[irq] = 0;
-
- /*
- * In the SMP+IOAPIC case it might happen that there are an unspecified
- * number of pending IRQ events unhandled. These cases are very rare,
- * so we 'resend' these IRQs via IPIs, to the same CPU. It's much
- * better to do it this way as thus we dont have to be aware of
- * 'pending' interrupts in the IRQ path, except at this point.
- */
- if (irq_events[irq]) {
- if (!ipi_pending[irq]) {
- ipi_pending[irq] = 1;
- --irq_events[irq];
- send_IPI(cpu,IO_APIC_VECTOR(irq));
- }
- }
- spin_unlock_irqrestore(&irq_controller_lock, flags);
-}
-#endif
-
-void enable_irq(unsigned int irq)
-{
- irq_handles[irq]->enable(irq);
}
void make_8259A_irq (unsigned int irq)
@@ -741,6 +691,7 @@
static inline void mask_and_ack_8259A(unsigned int irq)
{
spin_lock(&irq_controller_lock);
+ disabled_irq[irq] |= IRQ_INPROGRESS;
cached_irq_mask |= 1 << irq;
if (irq & 8) {
inb(0xA1); /* DUMMY */
@@ -763,7 +714,8 @@
if (handle_IRQ_event(irq, regs)) {
spin_lock(&irq_controller_lock);
- unmask_8259A(irq);
+ if (!(disabled_irq[irq] &= IRQ_DISABLED))
+ unmask_8259A(irq);
spin_unlock(&irq_controller_lock);
}
@@ -771,41 +723,115 @@
}
#ifdef __SMP__
+
+static int ipi_pending [NR_IRQS] = { 0, };
+
+/*
+ * In the SMP+IOAPIC case it might happen that there are an unspecified
+ * number of pending IRQ events unhandled. These cases are very rare,
+ * so we 'resend' these IRQs via IPIs, to the same CPU. It's much
+ * better to do it this way as thus we dont have to be aware of
+ * 'pending' interrupts in the IRQ path, except at this point.
+ */
+static void enable_ioapic_irq(unsigned int irq)
+{
+ if (irq_events[irq] && !ipi_pending[irq]) {
+ ipi_pending[irq] = 1;
+ send_IPI(APIC_DEST_SELF, IO_APIC_VECTOR(irq));
+ }
+}
+
+/*
+ * We do not actually disable IO-APIC irqs in hardware ...
+ */
+static void disable_ioapic_irq(unsigned int irq)
+{
+}
+
static void do_ioapic_IRQ(unsigned int irq, int cpu, struct pt_regs * regs)
{
- int should_handle_irq = 0;
+ spin_lock(&irq_controller_lock);
+ /* Ack the irq inside the lock! */
ack_APIC_irq();
+ ipi_pending[irq] = 0;
- spin_lock(&irq_controller_lock);
- if (ipi_pending[irq])
- ipi_pending[irq] = 0;
+ /* If the irq is disabled for whatever reason, just set a flag and return */
+ if (disabled_irq[irq] & (IRQ_DISABLED | IRQ_INPROGRESS)) {
+ irq_events[irq] = 1;
+ spin_unlock(&irq_controller_lock);
+ return;
+ }
- if (!irq_events[irq]++ && !disabled_irq[irq])
- should_handle_irq = 1;
+ disabled_irq[irq] = IRQ_INPROGRESS;
+ irq_events[irq] = 0;
hardirq_enter(cpu);
spin_unlock(&irq_controller_lock);
- if (should_handle_irq) {
- while (test_bit(0,&global_irq_lock)) mb();
-again:
+ while (test_bit(0,&global_irq_lock)) barrier();
+
+ for (;;) {
+ int pending;
+
handle_IRQ_event(irq, regs);
spin_lock(&irq_controller_lock);
- should_handle_irq=0;
- if (--irq_events[irq] && !disabled_irq[irq])
- should_handle_irq=1;
+ pending = irq_events[irq];
+ irq_events[irq] = 0;
+ if (!pending)
+ break;
spin_unlock(&irq_controller_lock);
-
- if (should_handle_irq)
- goto again;
}
+ disabled_irq[irq] &= IRQ_DISABLED;
+ spin_unlock(&irq_controller_lock);
hardirq_exit(cpu);
release_irqlock(cpu);
}
+
#endif
+
+/*
+ * Generic enable/disable code: this just calls
+ * down into the PIC-specific version for the actual
+ * hardware disable after having gotten the irq
+ * controller lock.
+ */
+void disable_irq(unsigned int irq)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&irq_controller_lock, flags);
+ /*
+ * At this point we may actually have a pending interrupt being active
+ * on another CPU. So don't touch the IRQ_INPROGRESS bit..
+ */
+ disabled_irq[irq] |= IRQ_DISABLED;
+ irq_handles[irq]->disable(irq);
+ spin_unlock_irqrestore(&irq_controller_lock, flags);
+
+ synchronize_irq();
+}
+
+void enable_irq(unsigned int irq)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&irq_controller_lock, flags);
+ /*
+ * In contrast to the above, we should _not_ have any concurrent
+ * interrupt activity here, so we just clear both disabled bits.
+ *
+ * This allows us to have IRQ_INPROGRESS set until we actually
+ * install a handler for this interrupt (make irq autodetection
+ * work by just lookinf at the "disabled_irq[]" array)
+ */
+ disabled_irq[irq] = 0;
+ irq_handles[irq]->enable(irq);
+ spin_unlock_irqrestore(&irq_controller_lock, flags);
+}
+
/*
* do_IRQ handles all normal device IRQ's (the special
* SMP cross-CPU interrupts have their own specific
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu