[PATCH] x86: PIC, L-APIC and I/O APIC debug information

[PATCH] x86: PIC, L-APIC and I/O APIC debug information

[Maciej W. Rozycki]

 Dump all the PIC, local APIC and I/O APIC information at the
fs_initcall() level, which is after ACPI (if used) has initialised PCI
information, making the point of invocation consistent across MP-table and
ACPI platforms.  Remove explicit calls to print_IO_APIC() from elsewhere.  
Make the interface of all the functions involved consistent between 32-bit
and 64-bit versions and make them all static by default by the means of a
New-and-Improved(TM) __apicdebuginit() macro.

 Note that like print_IO_APIC() all these only output anything if 
"apic=debug" has been passed to the kernel through the command line.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
patch-next-2.6.26-rc9-20080711-ioapic-debug-2
diff -up --recursive --new-file linux-next-2.6.26-rc9-20080711.macro/arch/x86/kernel/io_apic_32.c linux-next-2.6.26-rc9-20080711/arch/x86/kernel/io_apic_32.c
--- linux-next-2.6.26-rc9-20080711.macro/arch/x86/kernel/io_apic_32.c	2008-07-11 15:56:32.000000000 +0000
+++ linux-next-2.6.26-rc9-20080711/arch/x86/kernel/io_apic_32.c	2008-07-15 23:19:56.000000000 +0000
@@ -50,6 +50,8 @@
 #include <mach_apic.h>
 #include <mach_apicdef.h>
 
+#define __apicdebuginit(type) static type __init
+
 int (*ioapic_renumber_irq)(int ioapic, int irq);
 atomic_t irq_mis_count;
 
@@ -1352,7 +1354,8 @@ static void __init setup_timer_IRQ0_pin(
 	ioapic_write_entry(apic, pin, entry);
 }
 
-void __init print_IO_APIC(void)
+
+__apicdebuginit(void) print_IO_APIC(void)
 {
 	int apic, i;
 	union IO_APIC_reg_00 reg_00;
@@ -1467,9 +1470,7 @@ void __init print_IO_APIC(void)
 	return;
 }
 
-#if 0
-
-static void print_APIC_bitfield(int base)
+__apicdebuginit(void) print_APIC_bitfield(int base)
 {
 	unsigned int v;
 	int i, j;
@@ -1490,7 +1491,7 @@ static void print_APIC_bitfield(int base
 	}
 }
 
-void /*__init*/ print_local_APIC(void *dummy)
+__apicdebuginit(void) print_local_APIC(void *dummy)
 {
 	unsigned int v, ver, maxlvt;
 
@@ -1574,12 +1575,12 @@ void /*__init*/ print_local_APIC(void *d
 	printk("\n");
 }
 
-void print_all_local_APICs(void)
+__apicdebuginit(void) print_all_local_APICs(void)
 {
 	on_each_cpu(print_local_APIC, NULL, 1);
 }
 
-void /*__init*/ print_PIC(void)
+__apicdebuginit(void) print_PIC(void)
 {
 	unsigned int v;
 	unsigned long flags;
@@ -1611,7 +1612,17 @@ void /*__init*/ print_PIC(void)
 	printk(KERN_DEBUG "... PIC ELCR: %04x\n", v);
 }
 
-#endif  /*  0  */
+__apicdebuginit(int) print_all_ICs(void)
+{
+	print_PIC();
+	print_all_local_APICs();
+	print_IO_APIC();
+
+	return 0;
+}
+
+fs_initcall(print_all_ICs);
+
 
 static void __init enable_IO_APIC(void)
 {
@@ -2324,8 +2335,6 @@ void __init setup_IO_APIC(void)
 	setup_IO_APIC_irqs();
 	init_IO_APIC_traps();
 	check_timer();
-	if (!acpi_ioapic)
-		print_IO_APIC();
 }
 
 /*
diff -up --recursive --new-file linux-next-2.6.26-rc9-20080711.macro/arch/x86/kernel/io_apic_64.c linux-next-2.6.26-rc9-20080711/arch/x86/kernel/io_apic_64.c
--- linux-next-2.6.26-rc9-20080711.macro/arch/x86/kernel/io_apic_64.c	2008-07-13 01:24:07.000000000 +0000
+++ linux-next-2.6.26-rc9-20080711/arch/x86/kernel/io_apic_64.c	2008-07-15 23:20:45.000000000 +0000
@@ -52,6 +52,8 @@
 #include <mach_ipi.h>
 #include <mach_apic.h>
 
+#define __apicdebuginit(type) static type __init
+
 struct irq_cfg {
 	cpumask_t domain;
 	cpumask_t old_domain;
@@ -86,8 +88,6 @@ int first_system_vector = 0xfe;
 
 char system_vectors[NR_VECTORS] = { [0 ... NR_VECTORS-1] = SYS_VECTOR_FREE};
 
-#define __apicdebuginit  __init
-
 int sis_apic_bug; /* not actually supported, dummy for compile */
 
 static int no_timer_check;
@@ -971,7 +971,8 @@ static void __init setup_timer_IRQ0_pin(
 	ioapic_write_entry(apic, pin, entry);
 }
 
-void __apicdebuginit print_IO_APIC(void)
+
+__apicdebuginit(void) print_IO_APIC(void)
 {
 	int apic, i;
 	union IO_APIC_reg_00 reg_00;
@@ -1065,9 +1066,7 @@ void __apicdebuginit print_IO_APIC(void)
 	return;
 }
 
-#if 0
-
-static __apicdebuginit void print_APIC_bitfield (int base)
+__apicdebuginit(void) print_APIC_bitfield(int base)
 {
 	unsigned int v;
 	int i, j;
@@ -1088,7 +1087,7 @@ static __apicdebuginit void print_APIC_b
 	}
 }
 
-void __apicdebuginit print_local_APIC(void * dummy)
+__apicdebuginit(void) print_local_APIC(void *dummy)
 {
 	unsigned int v, ver, maxlvt;
 
@@ -1165,12 +1164,12 @@ void __apicdebuginit print_local_APIC(vo
 	printk("\n");
 }
 
-void print_all_local_APICs (void)
+__apicdebuginit(void) print_all_local_APICs(void)
 {
 	on_each_cpu(print_local_APIC, NULL, 1);
 }
 
-void __apicdebuginit print_PIC(void)
+__apicdebuginit(void) print_PIC(void)
 {
 	unsigned int v;
 	unsigned long flags;
@@ -1202,7 +1201,17 @@ void __apicdebuginit print_PIC(void)
 	printk(KERN_DEBUG "... PIC ELCR: %04x\n", v);
 }
 
-#endif  /*  0  */
+__apicdebuginit(int) print_all_ICs(void)
+{
+	print_PIC();
+	print_all_local_APICs();
+	print_IO_APIC();
+
+	return 0;
+}
+
+fs_initcall(print_all_ICs);
+
 
 void __init enable_IO_APIC(void)
 {
@@ -1848,8 +1857,6 @@ void __init setup_IO_APIC(void)
 	setup_IO_APIC_irqs();
 	init_IO_APIC_traps();
 	check_timer();
-	if (!acpi_ioapic)
-		print_IO_APIC();
 }
 
 struct sysfs_ioapic_data {
diff -up --recursive --new-file linux-next-2.6.26-rc9-20080711.macro/arch/x86/pci/acpi.c linux-next-2.6.26-rc9-20080711/arch/x86/pci/acpi.c
--- linux-next-2.6.26-rc9-20080711.macro/arch/x86/pci/acpi.c	2008-07-11 15:56:32.000000000 +0000
+++ linux-next-2.6.26-rc9-20080711/arch/x86/pci/acpi.c	2008-07-15 23:10:13.000000000 +0000
@@ -250,10 +250,5 @@ int __init pci_acpi_init(void)
 			acpi_pci_irq_enable(dev);
 	}
 
-#ifdef CONFIG_X86_IO_APIC
-	if (acpi_ioapic)
-		print_IO_APIC();
-#endif
-
 	return 0;
 }
diff -up --recursive --new-file linux-next-2.6.26-rc9-20080711.macro/include/asm-x86/hw_irq.h linux-next-2.6.26-rc9-20080711/include/asm-x86/hw_irq.h
--- linux-next-2.6.26-rc9-20080711.macro/include/asm-x86/hw_irq.h	2008-07-09 18:01:22.000000000 +0000
+++ linux-next-2.6.26-rc9-20080711/include/asm-x86/hw_irq.h	2008-07-15 23:26:41.000000000 +0000
@@ -64,7 +64,6 @@ extern unsigned long io_apic_irqs;
 extern void init_VISWS_APIC_irqs(void);
 extern void setup_IO_APIC(void);
 extern void disable_IO_APIC(void);
-extern void print_IO_APIC(void);
 extern int IO_APIC_get_PCI_irq_vector(int bus, int slot, int fn);
 extern void setup_ioapic_dest(void);
 

Re: [PATCH] x86: PIC, L-APIC and I/O APIC debug information

[Mikael Pettersson]

Maciej W. Rozycki writes:
 >  Dump all the PIC, local APIC and I/O APIC information at the
 > fs_initcall() level, which is after ACPI (if used) has initialised PCI
 > information, making the point of invocation consistent across MP-table and
 > ACPI platforms.  Remove explicit calls to print_IO_APIC() from elsewhere.  
 > Make the interface of all the functions involved consistent between 32-bit
 > and 64-bit versions and make them all static by default by the means of a
 > New-and-Improved(TM) __apicdebuginit() macro.
...
 > +#define __apicdebuginit(type) static type __init
...
 > -void __init print_IO_APIC(void)
 > +
 > +__apicdebuginit(void) print_IO_APIC(void)
 >  {

I _really_ dislike how this abuses the C macro preprocessor
to create pointless new syntax.

Since you're editing these function definitions anyway why
not just spell out "static void __init" in readable proper C?

/Mikael

Re: [PATCH] x86: PIC, L-APIC and I/O APIC debug information

[Maciej W. Rozycki]

On Mon, 21 Jul 2008, Mikael Pettersson wrote:

>  > +#define __apicdebuginit(type) static type __init
> ...
>  > -void __init print_IO_APIC(void)
>  > +
>  > +__apicdebuginit(void) print_IO_APIC(void)
>  >  {
> 
> I _really_ dislike how this abuses the C macro preprocessor
> to create pointless new syntax.
> 
> Since you're editing these function definitions anyway why
> not just spell out "static void __init" in readable proper C?

 This is so that while debugging you can make all these functions global
with a single change in one place, rather than going through the whole
file and finding all the relevant function headers.  Presumably the
original reason for the existence of the macro.  Unfortunately an
object-like macro cannot be used here, as the "static" keyword has to come
first in a function declaration and the section attribute has to come
after the type designation.

 What's wrong with the syntax in your opinion?

  Maciej

Re: [PATCH] x86: PIC, L-APIC and I/O APIC debug information

[Mikael Pettersson]

Maciej W. Rozycki writes:
 > On Mon, 21 Jul 2008, Mikael Pettersson wrote:
 > 
 > >  > +#define __apicdebuginit(type) static type __init
 > > ...
 > >  > -void __init print_IO_APIC(void)
 > >  > +
 > >  > +__apicdebuginit(void) print_IO_APIC(void)
 > >  >  {
 > > 
 > > I _really_ dislike how this abuses the C macro preprocessor
 > > to create pointless new syntax.
 > > 
 > > Since you're editing these function definitions anyway why
 > > not just spell out "static void __init" in readable proper C?
 > 
 >  This is so that while debugging you can make all these functions global
 > with a single change in one place, rather than going through the whole
 > file and finding all the relevant function headers.  Presumably the
 > original reason for the existence of the macro.  Unfortunately an
 > object-like macro cannot be used here, as the "static" keyword has to come
 > first in a function declaration and the section attribute has to come
 > after the type designation.
 > 
 >  What's wrong with the syntax in your opinion?

It's not even remotely C-like, which will cause confusion
for anyone or anything trying to read and understand the code.

(Basically you're on the path to PL/1 or Bourne C, neither of
which were great from a software engineering point of view.)

Something like the following looks saner to me, would it work for you?

	#ifdef debug
	#define apicdebug	/* empty */
	#define __apicinit	/* empty */
	#else
	#define apicdebug	static
	#define __apicinit	__init
	#endif
	...
	apicdebug void __apicinit print_IO_APIC(void)
	{ .. }

/Mikael

Re: [PATCH] x86: PIC, L-APIC and I/O APIC debug information

[Maciej W. Rozycki]

On Mon, 21 Jul 2008, Mikael Pettersson wrote:

> It's not even remotely C-like, which will cause confusion
> for anyone or anything trying to read and understand the code.

 Not being C-like sounds like an advantage as the reader would be forced 
to check what it really means, but I can see what you mean, and the fact 
we have got worse constructs elsewhere does not look like a terribly good 
argument either.

> Something like the following looks saner to me, would it work for you?
> 
> 	#ifdef debug
> 	#define apicdebug	/* empty */
> 	#define __apicinit	/* empty */
> 	#else
> 	#define apicdebug	static
> 	#define __apicinit	__init
> 	#endif
> 	...
> 	apicdebug void __apicinit print_IO_APIC(void)
> 	{ .. }

 Hmm, suddenly out of a single line, quite a lot of stuff has emerged.  I 
am not convinced and I am now thinking of something else instead.

 However these debugging facilities are not absolutely necessary for the
essential functionality of the patch, so I will simply remove them for
reconsideration and send a regenerated patch containing the rest only.

 Ingo, please drop this one from anywhere you may have it -- I will send a 
replacement shortly.

  Maciej

Re: [PATCH] x86: PIC, L-APIC and I/O APIC debug information

[Ingo Molnar]

* Maciej W. Rozycki <macro@linux-mips.org> wrote:

>  Dump all the PIC, local APIC and I/O APIC information at the 
> fs_initcall() level, which is after ACPI (if used) has initialised PCI 
> information, making the point of invocation consistent across MP-table 
> and ACPI platforms.  Remove explicit calls to print_IO_APIC() from 
> elsewhere.  Make the interface of all the functions involved 
> consistent between 32-bit and 64-bit versions and make them all static 
> by default by the means of a New-and-Improved(TM) __apicdebuginit() 
> macro.
> 
>  Note that like print_IO_APIC() all these only output anything if 
> "apic=debug" has been passed to the kernel through the command line.

applied to tip/x86/apic, thanks Maciej.

	Ingo