Cleanups for APIC

Cleanups for APIC

[Pavel Machek]

Hi!

This cleans up io_apic.c a bit -- I do not really like 4 copies of
same code. Does it look okay to apply?

								Pavel

--- tmp/linux/arch/i386/kernel/io_apic.c	2004-05-20 23:08:04.000000000 +0200
+++ linux/arch/i386/kernel/io_apic.c	2004-05-20 23:10:50.000000000 +0200
@@ -41,8 +42,6 @@
 
 #include "io_ports.h"
 
-#undef APIC_LOCKUP_DEBUG
-
 #define APIC_LOCKUP_DEBUG
 
 static spinlock_t ioapic_lock = SPIN_LOCK_UNLOCKED;
@@ -127,8 +126,7 @@
 	}
 }
 
-/* mask = 1 */
-static void __mask_IO_APIC_irq (unsigned int irq)
+static inline void __modify_IO_APIC_irq (unsigned int irq, unsigned long enable, unsigned long disable)
 {
 	int pin;
 	struct irq_pin_list *entry = irq_2_pin + irq;
@@ -139,71 +137,39 @@
 		if (pin == -1)
 			break;
 		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg |= 0x00010000);
+		reg &= ~disable;
+		reg |= enable;
+		io_apic_modify(entry->apic, 0x10 + pin*2, reg);
 		if (!entry->next)
 			break;
 		entry = irq_2_pin + entry->next;
 	}
-	io_apic_sync(entry->apic);
 }
 
-/* mask = 0 */
-static void __unmask_IO_APIC_irq (unsigned int irq)
+/* mask = 1 */
+static void __mask_IO_APIC_irq (unsigned int irq)
 {
-	int pin;
 	struct irq_pin_list *entry = irq_2_pin + irq;
+	__modify_IO_APIC_irq(irq, 0x00010000, 0);
+	io_apic_sync(entry->apic);	/* Is it needed? Or do others need it too? */
+}
 
-	for (;;) {
-		unsigned int reg;
-		pin = entry->pin;
-		if (pin == -1)
-			break;
-		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg &= 0xfffeffff);
-		if (!entry->next)
-			break;
-		entry = irq_2_pin + entry->next;
-	}
+/* mask = 0 */
+static void __unmask_IO_APIC_irq (unsigned int irq)
+{
+	__modify_IO_APIC_irq(irq, 0, 0x00010000);
 }
 
 /* mask = 1, trigger = 0 */
 static void __mask_and_edge_IO_APIC_irq (unsigned int irq)
 {
-	int pin;
-	struct irq_pin_list *entry = irq_2_pin + irq;
-
-	for (;;) {
-		unsigned int reg;
-		pin = entry->pin;
-		if (pin == -1)
-			break;
-		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		reg = (reg & 0xffff7fff) | 0x00010000;
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg);
-		if (!entry->next)
-			break;
-		entry = irq_2_pin + entry->next;
-	}
+	__modify_IO_APIC_irq(irq, 0x00010000, 0x00008000);
 }
 
 /* mask = 0, trigger = 1 */
 static void __unmask_and_level_IO_APIC_irq (unsigned int irq)
 {
-	int pin;
-	struct irq_pin_list *entry = irq_2_pin + irq;
-
-	for (;;) {
-		unsigned int reg;
-		pin = entry->pin;
-		if (pin == -1)
-			break;
-		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		reg = (reg & 0xfffeffff) | 0x00008000;
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg);
-		if (!entry->next)
-			break;
-		entry = irq_2_pin + entry->next;
-	}
+	__modify_IO_APIC_irq(irq, 0x00008000, 0x00010000);
 }
 
 static void mask_IO_APIC_irq (unsigned int irq)

-- 
934a471f20d6580d5aad759bf0d97ddc

Re: Cleanups for APIC

[Ingo Molnar]

On Tue, 25 May 2004, Pavel Machek wrote:

> This cleans up io_apic.c a bit -- I do not really like 4 copies of same
> code. Does it look okay to apply?

yeah, agreed - i checked & test it, it's ok. I made a small modification
(see the patch below) to uninline the __modify_IO_APIC_irq() function -
shaving 0.5K off the kernel's size ...

(wrt. io_apic_sync(): i added it in 2.1.104 together with some other
changes - i dont this it's necessary anymore - the local APICs had
writearound erratas, but i dont remember this ever being necessary for
IO-APICs. I'll address this in another patch.)

	Ingo

From: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/arch/i386/kernel/io_apic.c.orig	
+++ linux/arch/i386/kernel/io_apic.c	
@@ -41,8 +41,6 @@
 
 #include "io_ports.h"
 
-#undef APIC_LOCKUP_DEBUG
-
 #define APIC_LOCKUP_DEBUG
 
 static spinlock_t ioapic_lock = SPIN_LOCK_UNLOCKED;
@@ -127,83 +125,50 @@ static void __init replace_pin_at_irq(un
 	}
 }
 
-/* mask = 1 */
-static void __mask_IO_APIC_irq (unsigned int irq)
+static void __modify_IO_APIC_irq (unsigned int irq, unsigned long enable, unsigned long disable)
 {
-	int pin;
 	struct irq_pin_list *entry = irq_2_pin + irq;
+	unsigned int pin, reg;
 
 	for (;;) {
-		unsigned int reg;
 		pin = entry->pin;
 		if (pin == -1)
 			break;
 		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg |= 0x00010000);
+		reg &= ~disable;
+		reg |= enable;
+		io_apic_modify(entry->apic, 0x10 + pin*2, reg);
 		if (!entry->next)
 			break;
 		entry = irq_2_pin + entry->next;
 	}
+}
+
+/* mask = 1 */
+static void __mask_IO_APIC_irq (unsigned int irq)
+{
+	struct irq_pin_list *entry = irq_2_pin + irq;
+	__modify_IO_APIC_irq(irq, 0x00010000, 0);
+	/* Is it needed? Or do others need it too? */
 	io_apic_sync(entry->apic);
 }
 
 /* mask = 0 */
 static void __unmask_IO_APIC_irq (unsigned int irq)
 {
-	int pin;
-	struct irq_pin_list *entry = irq_2_pin + irq;
-
-	for (;;) {
-		unsigned int reg;
-		pin = entry->pin;
-		if (pin == -1)
-			break;
-		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg &= 0xfffeffff);
-		if (!entry->next)
-			break;
-		entry = irq_2_pin + entry->next;
-	}
+	__modify_IO_APIC_irq(irq, 0, 0x00010000);
 }
 
 /* mask = 1, trigger = 0 */
 static void __mask_and_edge_IO_APIC_irq (unsigned int irq)
 {
-	int pin;
-	struct irq_pin_list *entry = irq_2_pin + irq;
-
-	for (;;) {
-		unsigned int reg;
-		pin = entry->pin;
-		if (pin == -1)
-			break;
-		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		reg = (reg & 0xffff7fff) | 0x00010000;
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg);
-		if (!entry->next)
-			break;
-		entry = irq_2_pin + entry->next;
-	}
+	__modify_IO_APIC_irq(irq, 0x00010000, 0x00008000);
 }
 
 /* mask = 0, trigger = 1 */
 static void __unmask_and_level_IO_APIC_irq (unsigned int irq)
 {
-	int pin;
-	struct irq_pin_list *entry = irq_2_pin + irq;
-
-	for (;;) {
-		unsigned int reg;
-		pin = entry->pin;
-		if (pin == -1)
-			break;
-		reg = io_apic_read(entry->apic, 0x10 + pin*2);
-		reg = (reg & 0xfffeffff) | 0x00008000;
-		io_apic_modify(entry->apic, 0x10 + pin*2, reg);
-		if (!entry->next)
-			break;
-		entry = irq_2_pin + entry->next;
-	}
+	__modify_IO_APIC_irq(irq, 0x00008000, 0x00010000);
 }
 
 static void mask_IO_APIC_irq (unsigned int irq)

Re: Cleanups for APIC

[Maciej W. Rozycki]

On Thu, 27 May 2004, Ingo Molnar wrote:

> (wrt. io_apic_sync(): i added it in 2.1.104 together with some other
> changes - i dont this it's necessary anymore - the local APICs had
> writearound erratas, but i dont remember this ever being necessary for
> IO-APICs. I'll address this in another patch.)

 Hmm, isn't that needed to make sure the iomem writeback is completed
before exiting the caller?

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: Cleanups for APIC

[Ingo Molnar]

On Thu, 27 May 2004, Maciej W. Rozycki wrote:

> > (wrt. io_apic_sync(): i added it in 2.1.104 together with some other
> > changes - i dont this it's necessary anymore - the local APICs had
> > writearound erratas, but i dont remember this ever being necessary for
> > IO-APICs. I'll address this in another patch.)
> 
> Hmm, isn't that needed to make sure the iomem writeback is completed
> before exiting the caller?

the only thing that could happen is a POST delay in the PCI chipset - but
is that really an issue? Plus we only do the io_apic_sync() for the
masking, not the unmasking - so if it's needed then we dont do it
consistently.

	Ingo

Re: Cleanups for APIC

[Maciej W. Rozycki]

On Thu, 27 May 2004, Ingo Molnar wrote:

> > Hmm, isn't that needed to make sure the iomem writeback is completed
> > before exiting the caller?
> 
> the only thing that could happen is a POST delay in the PCI chipset - but
> is that really an issue? Plus we only do the io_apic_sync() for the
> masking, not the unmasking - so if it's needed then we dont do it
> consistently.

 The I/O APIC need not be hooked to PCI ;-) -- I'm not sure about the
i82093AA, but that's definitely true for the i82489DX.  The call to
io_apic_sync() is needed for masking to make sure interrupts won't be
dispatched after returning from the call -- this is not needed for
unmasking as a delay here is harmless.

 Though, now that we don't mask APIC interrupts during their service
anymore the synchronization may be superfluous indeed -- other cases have
to take late interrupts into account anyway.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: Cleanups for APIC

[Ingo Molnar]

* Maciej W. Rozycki <macro@ds2.pg.gda.pl> wrote:

>  The I/O APIC need not be hooked to PCI ;-) -- I'm not sure about the
> i82093AA, but that's definitely true for the i82489DX.  The call to
> io_apic_sync() is needed for masking to make sure interrupts won't be
> dispatched after returning from the call -- this is not needed for
> unmasking as a delay here is harmless.

well, an APIC message could be on the way to the CPU even with this
synchronization. Does it matter whether it's a newly dispatched one due
to POST delays or an in-fly one due to APIC bus delays?

	Ingo

Re: Cleanups for APIC

[Maciej W. Rozycki]

On Thu, 27 May 2004, Ingo Molnar wrote:

> >  The I/O APIC need not be hooked to PCI ;-) -- I'm not sure about the
> > i82093AA, but that's definitely true for the i82489DX.  The call to
> > io_apic_sync() is needed for masking to make sure interrupts won't be
> > dispatched after returning from the call -- this is not needed for
> > unmasking as a delay here is harmless.
> 
> well, an APIC message could be on the way to the CPU even with this
> synchronization. Does it matter whether it's a newly dispatched one due
> to POST delays or an in-fly one due to APIC bus delays?

 Well, if you'd mask, sync, ack (send EOI) in a handler, then the sync
would assure the ack wouldn't be in effect before masking, so no further
interrupt would arrive till unmasking.  It would work for level-triggered
interrupts and the i82093AA, but OTOH for the i82489DX, which uses
level-deassert messages, it wouldn't.

 Too much hassle for an unreliable result...  Just scrap it.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +