[patch 1/3] x86: io-apic - use ARRAY_SIZE macro

[patch 1/3] x86: io-apic - use ARRAY_SIZE macro

[Cyrill Gorcunov]

[-- Attachment #1: x86-ioapic-use-ARRAY_SIZE --]
[-- Type: text/plain, Size: 707 bytes --]

Make the code width a bit shorter with ARRAY_SIZE macro.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-04 20:34:14.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-04 21:35:48.000000000 +0400
@@ -166,7 +166,7 @@ static void __init init_work(void *data)
 
 	memcpy(cfg, irq_cfg_legacy, sizeof(irq_cfg_legacy));
 
-	legacy_count = sizeof(irq_cfg_legacy)/sizeof(irq_cfg_legacy[0]);
+	legacy_count = ARRAY_SIZE(irq_cfg_legacy);
 	for (i = legacy_count; i < *da->nr; i++)
 		init_one_irq_cfg(&cfg[i]);
 

-- 

Re: [patch 1/3] x86: io-apic - use ARRAY_SIZE macro

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Make the code width a bit shorter with ARRAY_SIZE macro.

applied to tip/irq/sparseirq (which is hosting the IO-APIC unification 
series) - thanks Cyrill.

	Ingo

[patch 2/3] x86: io-apic - declare irq_cfg_lock for SPARSE_IRQ only

[Cyrill Gorcunov]

[-- Attachment #1: x86-apic-irq_cfg_lock --]
[-- Type: text/plain, Size: 825 bytes --]

We use irq_cfg_lock lock in SPARSE_IRQ only context so
move it under #ifdef and compiler will be happy.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-04 22:06:37.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-04 22:08:06.000000000 +0400
@@ -147,14 +147,15 @@ static void init_one_irq_cfg(struct irq_
 
 static struct irq_cfg *irq_cfgx;
 
+#ifdef CONFIG_HAVE_SPARSE_IRQ
 /*
  * Protect the irq_cfgx_free freelist:
  */
 static DEFINE_SPINLOCK(irq_cfg_lock);
 
-#ifdef CONFIG_HAVE_SPARSE_IRQ
 static struct irq_cfg *irq_cfgx_free;
 #endif
+
 static void __init init_work(void *data)
 {
 	struct dyn_array *da = data;

-- 

Re: [patch 2/3] x86: io-apic - declare irq_cfg_lock for SPARSE_IRQ only

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> We use irq_cfg_lock lock in SPARSE_IRQ only context so
> move it under #ifdef and compiler will be happy.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

applied to tip/irq/sparseirq - thanks Cyrill.

	Ingo

[patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[-- Attachment #1: x86-ioapic-simplify-setup_IO_APIC_irqs --]
[-- Type: text/plain, Size: 2189 bytes --]

Use a nested level for 'for' cycle and break long lines.
For apic_print we should countinue using KERNEL_DEBUG if
we have started to.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-04 22:08:06.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-04 22:08:32.000000000 +0400
@@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
 	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
 
 	for (apic = 0; apic < nr_ioapics; apic++) {
-	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
+		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
 
-		idx = find_irq_entry(apic,pin,mp_INT);
-		if (idx == -1) {
-			if (first_notcon) {
-				apic_printk(APIC_VERBOSE, KERN_DEBUG " IO-APIC (apicid-pin) %d-%d", mp_ioapics[apic].mp_apicid, pin);
-				first_notcon = 0;
-			} else
-				apic_printk(APIC_VERBOSE, ", %d-%d", mp_ioapics[apic].mp_apicid, pin);
-			continue;
-		}
-		if (!first_notcon) {
-			apic_printk(APIC_VERBOSE, " not connected.\n");
-			first_notcon = 1;
-		}
+			idx = find_irq_entry(apic, pin, mp_INT);
+			if (idx == -1) {
+				if (first_notcon) {
+					apic_printk(APIC_VERBOSE, KERN_DEBUG
+							" IO-APIC (apicid-pin) %d-%d",
+							mp_ioapics[apic].mp_apicid, pin);
+					first_notcon = 0;
+				} else
+					apic_printk(APIC_VERBOSE, KERN_DEBUG ", %d-%d",
+							mp_ioapics[apic].mp_apicid, pin);
+				continue;
+			}
+			if (!first_notcon) {
+				apic_printk(APIC_VERBOSE, " not connected.\n");
+				first_notcon = 1;
+			}
 
-		irq = pin_2_irq(idx, apic, pin);
+			irq = pin_2_irq(idx, apic, pin);
 #ifdef CONFIG_X86_32
-                if (multi_timer_check(apic, irq))
-                        continue;
+			if (multi_timer_check(apic, irq))
+				continue;
 #endif
-		add_pin_to_irq(irq, apic, pin);
+			add_pin_to_irq(irq, apic, pin);
 
-		setup_IO_APIC_irq(apic, pin, irq,
-				  irq_trigger(idx), irq_polarity(idx));
-	}
+			setup_IO_APIC_irq(apic, pin, irq,
+					irq_trigger(idx), irq_polarity(idx));
+		}
 	}
 
 	if (!first_notcon)

-- 

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Use a nested level for 'for' cycle and break long lines.
> For apic_print we should countinue using KERNEL_DEBUG if
> we have started to.

> @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
>  	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
>  
>  	for (apic = 0; apic < nr_ioapics; apic++) {
> -	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
> +		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
>  
> +			idx = find_irq_entry(apic, pin, mp_INT);
> +			if (idx == -1) {

hm, i dont really like the super-deep nesting we do here. Could you 
please split out the iterator into a separate function? That makes the 
code a lot easier to understand and saves two extra tabs as well for 
those ugly-looking printk lines.

	Ingo

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > Use a nested level for 'for' cycle and break long lines.
| > For apic_print we should countinue using KERNEL_DEBUG if
| > we have started to.
| 
| > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
| >  	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| >  
| >  	for (apic = 0; apic < nr_ioapics; apic++) {
| > -	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > +		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| >  
| > +			idx = find_irq_entry(apic, pin, mp_INT);
| > +			if (idx == -1) {
| 
| hm, i dont really like the super-deep nesting we do here. Could you 
| please split out the iterator into a separate function? That makes the 
| code a lot easier to understand and saves two extra tabs as well for 
| those ugly-looking printk lines.
| 
| 	Ingo
| 

ok Ingo, will take a look, thanks

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > Use a nested level for 'for' cycle and break long lines.
| > For apic_print we should countinue using KERNEL_DEBUG if
| > we have started to.
| 
| > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
| >  	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| >  
| >  	for (apic = 0; apic < nr_ioapics; apic++) {
| > -	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > +		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| >  
| > +			idx = find_irq_entry(apic, pin, mp_INT);
| > +			if (idx == -1) {
| 
| hm, i dont really like the super-deep nesting we do here. Could you 
| please split out the iterator into a separate function? That makes the 
| code a lot easier to understand and saves two extra tabs as well for 
| those ugly-looking printk lines.
| 
| 	Ingo
| 

You know it seems we use such a 'cycle on every pin on io-apics'
in several places for now:

io_apic.c
---------
	clear_IO_APIC
	save_mask_IO_APIC_setup
	restore_IO_APIC_setup
	IO_APIC_irq_trigger
	setup_IO_APIC_irqs

I've made a one-line macro for this (like for_all_ioapics_pins)
_but_ it looks much more ugly then this two nested for(;;) :)

If you meant me to make a separate iterator over the pins I think
it will not help a lot - this function is simple enought so the only
problem is too-long-printk-form - maybe just print them on separated
lines instead of tracking apicids? Or it was made in a sake to not
scroll screen too much?

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
> | 
> | * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> | 
> | > Use a nested level for 'for' cycle and break long lines.
> | > For apic_print we should countinue using KERNEL_DEBUG if
> | > we have started to.
> | 
> | > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
> | >  	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
> | >  
> | >  	for (apic = 0; apic < nr_ioapics; apic++) {
> | > -	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
> | > +		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
> | >  
> | > +			idx = find_irq_entry(apic, pin, mp_INT);
> | > +			if (idx == -1) {
> | 
> | hm, i dont really like the super-deep nesting we do here. Could you 
> | please split out the iterator into a separate function? That makes the 
> | code a lot easier to understand and saves two extra tabs as well for 
> | those ugly-looking printk lines.
> | 
> | 	Ingo
> | 
> 
> You know it seems we use such a 'cycle on every pin on io-apics'
> in several places for now:
> 
> io_apic.c
> ---------
> 	clear_IO_APIC
> 	save_mask_IO_APIC_setup
> 	restore_IO_APIC_setup
> 	IO_APIC_irq_trigger
> 	setup_IO_APIC_irqs
> 
> I've made a one-line macro for this (like for_all_ioapics_pins)
> _but_ it looks much more ugly then this two nested for(;;) :)
> 
> If you meant me to make a separate iterator over the pins I think
> it will not help a lot - this function is simple enought so the only
> problem is too-long-printk-form - maybe just print them on separated
> lines instead of tracking apicids? Or it was made in a sake to not
> scroll screen too much?

hm, by iterator i meant the body itself. I.e. something like:

static void __init setup_IO_APIC_irqs(void)
{
        int apic, pin, notcon = 1;

        apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");

        for (apic = 0; apic < nr_ioapics; apic++)
		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++)
			notcon = setup_ioapic_irq(apic, pin, notcon);

        if (!notcon)
                apic_printk(APIC_VERBOSE, " not connected.\n");
}

this looks quite a bit cleaner, doesnt it? We lose the 'idx' and 'irq' 
variables and we lose the curly braces as well. The flow looks a lot 
more trivial. And the new setup_ioapic_irq() function will be simpler as 
well - it will only have 'idx' and 'irq' as a local variable, the rest 
comes in as a parameter. It can 'return notcon' instead of 'continue'. 
And it will be 2 levels of tabs aligned to the left, as an added bonus.

Hm?

	Ingo

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Sep 05, 2008 at 08:11:11PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > [Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
| > | 
| > | * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > | 
| > | > Use a nested level for 'for' cycle and break long lines.
| > | > For apic_print we should countinue using KERNEL_DEBUG if
| > | > we have started to.
| > | 
| > | > @@ -1521,32 +1521,35 @@ static void __init setup_IO_APIC_irqs(vo
| > | >  	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| > | >  
| > | >  	for (apic = 0; apic < nr_ioapics; apic++) {
| > | > -	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > | > +		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
| > | >  
| > | > +			idx = find_irq_entry(apic, pin, mp_INT);
| > | > +			if (idx == -1) {
| > | 
| > | hm, i dont really like the super-deep nesting we do here. Could you 
| > | please split out the iterator into a separate function? That makes the 
| > | code a lot easier to understand and saves two extra tabs as well for 
| > | those ugly-looking printk lines.
| > | 
| > | 	Ingo
| > | 
| > 
| > You know it seems we use such a 'cycle on every pin on io-apics'
| > in several places for now:
| > 
| > io_apic.c
| > ---------
| > 	clear_IO_APIC
| > 	save_mask_IO_APIC_setup
| > 	restore_IO_APIC_setup
| > 	IO_APIC_irq_trigger
| > 	setup_IO_APIC_irqs
| > 
| > I've made a one-line macro for this (like for_all_ioapics_pins)
| > _but_ it looks much more ugly then this two nested for(;;) :)
| > 
| > If you meant me to make a separate iterator over the pins I think
| > it will not help a lot - this function is simple enought so the only
| > problem is too-long-printk-form - maybe just print them on separated
| > lines instead of tracking apicids? Or it was made in a sake to not
| > scroll screen too much?
| 
| hm, by iterator i meant the body itself. I.e. something like:
| 
| static void __init setup_IO_APIC_irqs(void)
| {
|         int apic, pin, notcon = 1;
| 
|         apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
| 
|         for (apic = 0; apic < nr_ioapics; apic++)
| 		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++)
| 			notcon = setup_ioapic_irq(apic, pin, notcon);
| 
|         if (!notcon)
|                 apic_printk(APIC_VERBOSE, " not connected.\n");
| }
| 
| this looks quite a bit cleaner, doesnt it? We lose the 'idx' and 'irq' 
| variables and we lose the curly braces as well. The flow looks a lot 
| more trivial. And the new setup_ioapic_irq() function will be simpler as 
| well - it will only have 'idx' and 'irq' as a local variable, the rest 
| comes in as a parameter. It can 'return notcon' instead of 'continue'. 
| And it will be 2 levels of tabs aligned to the left, as an added bonus.
| 
| Hm?
| 
| 	Ingo
| 

Yes Ingo it does look much cleaner _but_ the only thing which bothering
me is that we split 'logical solid' printing into several functions -
i mean we started printing in new setup_ioapic_irq function and finish
it in caller and that is much worser then having long lines printing
in single function i think (but I could be wrong :)

If we just drop original printing (just for a second to get the whole
image) we will get:

---
static void __init setup_IO_APIC_irqs(void)
{
	int apic, pin, idx, irq, first_notcon = 1;

	for (apic = 0; apic < nr_ioapics; apic++) {
		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {

			idx = find_irq_entry(apic, pin, mp_INT);
			if (idx == -1) 
				continue;

			irq = pin_2_irq(idx, apic, pin);
#ifdef CONFIG_X86_32
			if (multi_timer_check(apic, irq))
				continue;
#endif
			add_pin_to_irq(irq, apic, pin);

			setup_IO_APIC_irq(apic, pin, irq,
					irq_trigger(idx), irq_polarity(idx));
		}
	}
---

So as you see it's more then enough self-solid :) So I wouldn't
break it 'cause of printing. If we have enough memory for bit
field - we could just mark there if pin is connected to irq and
print connection map after.
Don't get me wrong please - I just don't want to overload this function
with additional call.

According how many characters have been typed for this message I think
instead of talking I could already have done the patch you supposed :)

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> So as you see it's more then enough self-solid :) So I wouldn't break 
> it 'cause of printing. If we have enough memory for bit field - we 
> could just mark there if pin is connected to irq and print connection 
> map after. Don't get me wrong please - I just don't want to overload 
> this function with additional call.

the compiler will inline that single-called function just fine, as long 
as you declare it static.

spreading printouts over several functions isnt all that bad i think. We 
could even drop the 'not connected' complication - it's evident enough 
from the fact that nothing gets printed.

	Ingo

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Sep 05, 2008 at 08:38:35PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > So as you see it's more then enough self-solid :) So I wouldn't break 
| > it 'cause of printing. If we have enough memory for bit field - we 
| > could just mark there if pin is connected to irq and print connection 
| > map after. Don't get me wrong please - I just don't want to overload 
| > this function with additional call.
| 
| the compiler will inline that single-called function just fine, as long 
| as you declare it static.
| 
| spreading printouts over several functions isnt all that bad i think. We 
| could even drop the 'not connected' complication - it's evident enough 
| from the fact that nothing gets printed.
| 
| 	Ingo
|

Ingo, lets leave it as is for now :)

(we cshouldn't drop these messages since they show us what
exactly is wrong with MP table)
 
		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Fri, Sep 05, 2008 at 08:38:35PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > So as you see it's more then enough self-solid :) So I wouldn't break 
| > it 'cause of printing. If we have enough memory for bit field - we 
| > could just mark there if pin is connected to irq and print connection 
| > map after. Don't get me wrong please - I just don't want to overload 
| > this function with additional call.
| 
| the compiler will inline that single-called function just fine, as long 
| as you declare it static.
| 
| spreading printouts over several functions isnt all that bad i think. We 
| could even drop the 'not connected' complication - it's evident enough 
| from the fact that nothing gets printed.
| 
| 	Ingo
| 

Ingo, how about the following approach? We don't introduce new
functions but rather srink the code by new printout form.

		- Cyrill -
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] x86: io-apic - setup_IO_APIC_irqs code simplification

By changing printout form we are able to shrink code a bit.

Former printout example:

	init IO_APIC IRQs
	 IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
	 IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.

New printout example:

	init IO_APIC IRQs
	 1-1 1-2 1-3 (apicid-pin) not connected
	 2-1 2-2 2-3 (apicid-pin) not connected

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-06 13:46:23.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-06 14:08:13.000000000 +0400
@@ -1516,41 +1516,44 @@ static void setup_IO_APIC_irq(int apic, 
 
 static void __init setup_IO_APIC_irqs(void)
 {
-	int apic, pin, idx, irq, first_notcon = 1;
+	int apic, pin, idx, irq;
+	int notcon = 0;
 
 	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
 
 	for (apic = 0; apic < nr_ioapics; apic++) {
-	for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
+		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
 
-		idx = find_irq_entry(apic,pin,mp_INT);
-		if (idx == -1) {
-			if (first_notcon) {
-				apic_printk(APIC_VERBOSE, KERN_DEBUG " IO-APIC (apicid-pin) %d-%d", mp_ioapics[apic].mp_apicid, pin);
-				first_notcon = 0;
-			} else
-				apic_printk(APIC_VERBOSE, ", %d-%d", mp_ioapics[apic].mp_apicid, pin);
-			continue;
-		}
-		if (!first_notcon) {
-			apic_printk(APIC_VERBOSE, " not connected.\n");
-			first_notcon = 1;
-		}
+			idx = find_irq_entry(apic, pin, mp_INT);
+			if (idx == -1) {
+				apic_printk(APIC_VERBOSE,
+					KERN_DEBUG " %d-%d",
+					mp_ioapics[apic].mp_apicid, pin);
+				if (!notcon)
+					notcon = 1;
+				continue;
+			}
 
-		irq = pin_2_irq(idx, apic, pin);
+			irq = pin_2_irq(idx, apic, pin);
 #ifdef CONFIG_X86_32
-                if (multi_timer_check(apic, irq))
-                        continue;
+			if (multi_timer_check(apic, irq))
+				continue;
 #endif
-		add_pin_to_irq(irq, apic, pin);
+			add_pin_to_irq(irq, apic, pin);
 
-		setup_IO_APIC_irq(apic, pin, irq,
-				  irq_trigger(idx), irq_polarity(idx));
-	}
+			setup_IO_APIC_irq(apic, pin, irq,
+					irq_trigger(idx), irq_polarity(idx));
+		}
+		if (notcon) {
+			apic_printk(APIC_VERBOSE,
+				KERN_DEBUG " (apicid-pin) not connected\n");
+			notcon = 0;
+		}
 	}
 
-	if (!first_notcon)
-		apic_printk(APIC_VERBOSE, " not connected.\n");
+	if (notcon)
+		apic_printk(APIC_VERBOSE,
+			KERN_DEBUG " (apicid-pin) not connected\n");
 }
 
 /*

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> By changing printout form we are able to shrink code a bit.
> 
> Former printout example:
> 
> 	init IO_APIC IRQs
> 	 IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
> 	 IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
> 
> New printout example:
> 
> 	init IO_APIC IRQs
> 	 1-1 1-2 1-3 (apicid-pin) not connected
> 	 2-1 2-2 2-3 (apicid-pin) not connected

applied to tip/irq/sparseirq - thanks Cyrill.

	Ingo

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Yinghai Lu]

On Sat, Sep 6, 2008 at 6:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> By changing printout form we are able to shrink code a bit.
>>
>> Former printout example:
>>
>>       init IO_APIC IRQs
>>        IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
>>        IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
>>
>> New printout example:
>>
>>       init IO_APIC IRQs
>>        1-1 1-2 1-3 (apicid-pin) not connected
>>        2-1 2-2 2-3 (apicid-pin) not connected
>
> applied to tip/irq/sparseirq - thanks Cyrill.

got
 0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7>
(apicid-pin) not connected
 1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7>
1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7>
1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not
connected

can you remove the extra <7>?

YH

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Mon, Sep 8, 2008 at 4:24 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Sat, Sep 6, 2008 at 6:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>
>>> By changing printout form we are able to shrink code a bit.
>>>
>>> Former printout example:
>>>
>>>       init IO_APIC IRQs
>>>        IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
>>>        IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
>>>
>>> New printout example:
>>>
>>>       init IO_APIC IRQs
>>>        1-1 1-2 1-3 (apicid-pin) not connected
>>>        2-1 2-2 2-3 (apicid-pin) not connected
>>
>> applied to tip/irq/sparseirq - thanks Cyrill.
>
> got
>  0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7>
> (apicid-pin) not connected
>  1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7>
> 1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7>
> 1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not
> connected
>
> can you remove the extra <7>?
>
> YH
>

Thanks Yinghai! The patch enveloped.
I'm overzealous with KERN_DEBUG marker. Sorry.
(Can't send it inlined - hope it's not a big problem)

Cyrill

[-- Attachment #2: io-apic.patch --]
[-- Type: application/octet-stream, Size: 1386 bytes --]

From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] x86: io-apic - do not use KERN_DEBUG marker too much

Do not use KERN_DEBUG several times on the same line being printed.
Introduced by mine previous patch, sorry.

Reported-by: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

--- a/arch/x86/kernel/io_apic.c	Mon Sep 08 07:34:16 2008
+++ b/arch/x86/kernel/io_apic.c	Mon Sep 08 08:06:30 2008
@@ -1526,11 +1526,16 @@ static void __init setup_IO_APIC_irqs(vo
 
 			idx = find_irq_entry(apic, pin, mp_INT);
 			if (idx == -1) {
-				apic_printk(APIC_VERBOSE,
-					KERN_DEBUG " %d-%d",
-					mp_ioapics[apic].mp_apicid, pin);
-				if (!notcon)
+				if (!notcon) {
 					notcon = 1;
+					apic_printk(APIC_VERBOSE,
+						KERN_DEBUG " %d-%d",
+						mp_ioapics[apic].mp_apicid,
+						pin);
+				} else
+					apic_printk(APIC_VERBOSE, " %d-%d",
+						mp_ioapics[apic].mp_apicid,
+						pin);
 				continue;
 			}
 
@@ -1546,14 +1551,14 @@ static void __init setup_IO_APIC_irqs(vo
 		}
 		if (notcon) {
 			apic_printk(APIC_VERBOSE,
-				KERN_DEBUG " (apicid-pin) not connected\n");
+				" (apicid-pin) not connected\n");
 			notcon = 0;
 		}
 	}
 
 	if (notcon)
 		apic_printk(APIC_VERBOSE,
-			KERN_DEBUG " (apicid-pin) not connected\n");
+			" (apicid-pin) not connected\n");
 }
 
 /*

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

On Mon, Sep 8, 2008 at 4:24 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Sat, Sep 6, 2008 at 6:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>
>> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>
>>> By changing printout form we are able to shrink code a bit.
>>>
>>> Former printout example:
>>>
>>>       init IO_APIC IRQs
>>>        IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
>>>        IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
>>>
>>> New printout example:
>>>
>>>       init IO_APIC IRQs
>>>        1-1 1-2 1-3 (apicid-pin) not connected
>>>        2-1 2-2 2-3 (apicid-pin) not connected
>>
>> applied to tip/irq/sparseirq - thanks Cyrill.
>
> got
>  0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7>
> (apicid-pin) not connected
>  1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7>
> 1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7>
> 1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not
> connected
>
> can you remove the extra <7>?
>
> YH
>

Btw Yinghai, do you really have a machine with that many unconnected pins?

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Yinghai Lu]

On Sun, Sep 7, 2008 at 9:20 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Mon, Sep 8, 2008 at 4:24 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> On Sat, Sep 6, 2008 at 6:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>>
>>> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>>
>>>> By changing printout form we are able to shrink code a bit.
>>>>
>>>> Former printout example:
>>>>
>>>>       init IO_APIC IRQs
>>>>        IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
>>>>        IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
>>>>
>>>> New printout example:
>>>>
>>>>       init IO_APIC IRQs
>>>>        1-1 1-2 1-3 (apicid-pin) not connected
>>>>        2-1 2-2 2-3 (apicid-pin) not connected
>>>
>>> applied to tip/irq/sparseirq - thanks Cyrill.
>>
>> got
>>  0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7>
>> (apicid-pin) not connected
>>  1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7>
>> 1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7>
>> 1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not
>> connected
>>
>> can you remove the extra <7>?
>>
>> YH
>>
>
> Btw Yinghai, do you really have a machine with that many unconnected pins?

Yes

recent change in sparseirq, we only init irq [0,16) at that point. so
pins on other io apic controller all unconnected...

YH

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Yinghai Lu]

On Sun, Sep 7, 2008 at 9:38 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Sun, Sep 7, 2008 at 9:20 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> On Mon, Sep 8, 2008 at 4:24 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>> On Sat, Sep 6, 2008 at 6:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>>>
>>>> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>>>
>>>>> By changing printout form we are able to shrink code a bit.
>>>>>
>>>>> Former printout example:
>>>>>
>>>>>       init IO_APIC IRQs
>>>>>        IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
>>>>>        IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
>>>>>
>>>>> New printout example:
>>>>>
>>>>>       init IO_APIC IRQs
>>>>>        1-1 1-2 1-3 (apicid-pin) not connected
>>>>>        2-1 2-2 2-3 (apicid-pin) not connected
>>>>
>>>> applied to tip/irq/sparseirq - thanks Cyrill.
>>>
>>> got
>>>  0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7>
>>> (apicid-pin) not connected
>>>  1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7>
>>> 1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7>
>>> 1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not
>>> connected
>>>
>>> can you remove the extra <7>?
>>>
>>> YH
>>>
>>
>> Btw Yinghai, do you really have a machine with that many unconnected pins?
>
> Yes
>
> recent change in sparseirq, we only init irq [0,16) at that point. so
> pins on other io apic controller all unconnected...
>
correction: sparseirq does not change the behavoir...

ENABLING IO-APIC IRQs
init IO_APIC IRQs
 IO-APIC (apicid-pin) 0-0 not connected.
IOAPIC[0]: Set routing entry (0-1 -> 0x31 -> IRQ 1 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-2 -> 0x30 -> IRQ 0 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-3 -> 0x33 -> IRQ 3 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-4 -> 0x34 -> IRQ 4 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-5 -> 0x35 -> IRQ 5 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-6 -> 0x36 -> IRQ 6 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-7 -> 0x37 -> IRQ 7 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-8 -> 0x38 -> IRQ 8 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-9 -> 0x39 -> IRQ 9 Mode:1 Active:0)
IOAPIC[0]: Set routing entry (0-10 -> 0x3a -> IRQ 10 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-11 -> 0x3b -> IRQ 11 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-12 -> 0x3c -> IRQ 12 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-13 -> 0x3d -> IRQ 13 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-14 -> 0x3e -> IRQ 14 Mode:0 Active:0)
IOAPIC[0]: Set routing entry (0-15 -> 0x3f -> IRQ 15 Mode:0 Active:0)
 IO-APIC (apicid-pin) 0-16, 0-17, 0-18, 0-19, 0-20, 0-21, 0-22, 0-23,
1-0, 1-1, 1-2, 1-3, 1-4, 1-5, 1-6, 2-0, 2-1, 2-2, 2-3, 2-4, 2-5, 2-6,
3-0, 3-1, 3-2, 3-3, 3-4, 3-5, 3-6, 3-7, 3-8, 3-9, 3-10, 3-11, 3-12,
3-13, 3-14, 3-15, 3-16, 3-17, 3-18, 3-19, 3-20, 3-21, 3-22, 3-23 not
connected.

there is some difference between using acpi or mptable...

when mpatble is used, mp_irqs is all filled after mptable is parsed.

when acpi is used, one [0-15) is filled..., later it will fill entries
when acpi add entries...mp_register_gsi/mp_config_acpi_gsi

YH

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

On Mon, Sep 8, 2008 at 9:07 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Sun, Sep 7, 2008 at 9:38 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>> On Sun, Sep 7, 2008 at 9:20 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>> On Mon, Sep 8, 2008 at 4:24 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>>>> On Sat, Sep 6, 2008 at 6:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
>>>>>
>>>>> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>>>>
>>>>>> By changing printout form we are able to shrink code a bit.
>>>>>>
>>>>>> Former printout example:
>>>>>>
>>>>>>       init IO_APIC IRQs
>>>>>>        IO-APIC (apicid-pin) 1-1, 1-2, 1-3 not connected.
>>>>>>        IO-APIC (apicid-pin) 2-1, 2-2, 2-3 not connected.
>>>>>>
>>>>>> New printout example:
>>>>>>
>>>>>>       init IO_APIC IRQs
>>>>>>        1-1 1-2 1-3 (apicid-pin) not connected
>>>>>>        2-1 2-2 2-3 (apicid-pin) not connected
>>>>>
>>>>> applied to tip/irq/sparseirq - thanks Cyrill.
>>>>
>>>> got
>>>>  0-16<7> 0-17<7> 0-18<7> 0-19<7> 0-20<7> 0-21<7> 0-22<7> 0-23<7>
>>>> (apicid-pin) not connected
>>>>  1-0<7> 1-1<7> 1-2<7> 1-3<7> 1-4<7> 1-5<7> 1-6<7> 1-7<7> 1-8<7> 1-9<7>
>>>> 1-10<7> 1-11<7> 1-12<7> 1-13<7> 1-14<7> 1-15<7> 1-16<7> 1-17<7>
>>>> 1-18<7> 1-19<7> 1-20<7> 1-21<7> 1-22<7> 1-23<7> (apicid-pin) not
>>>> connected
>>>>
>>>> can you remove the extra <7>?
>>>>
>>>> YH
>>>>
>>>
>>> Btw Yinghai, do you really have a machine with that many unconnected pins?
>>
>> Yes
>>
>> recent change in sparseirq, we only init irq [0,16) at that point. so
>> pins on other io apic controller all unconnected...
>>
> correction: sparseirq does not change the behavoir...
>
> ENABLING IO-APIC IRQs
> init IO_APIC IRQs
>  IO-APIC (apicid-pin) 0-0 not connected.
> IOAPIC[0]: Set routing entry (0-1 -> 0x31 -> IRQ 1 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-2 -> 0x30 -> IRQ 0 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-3 -> 0x33 -> IRQ 3 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-4 -> 0x34 -> IRQ 4 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-5 -> 0x35 -> IRQ 5 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-6 -> 0x36 -> IRQ 6 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-7 -> 0x37 -> IRQ 7 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-8 -> 0x38 -> IRQ 8 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-9 -> 0x39 -> IRQ 9 Mode:1 Active:0)
> IOAPIC[0]: Set routing entry (0-10 -> 0x3a -> IRQ 10 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-11 -> 0x3b -> IRQ 11 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-12 -> 0x3c -> IRQ 12 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-13 -> 0x3d -> IRQ 13 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-14 -> 0x3e -> IRQ 14 Mode:0 Active:0)
> IOAPIC[0]: Set routing entry (0-15 -> 0x3f -> IRQ 15 Mode:0 Active:0)
>  IO-APIC (apicid-pin) 0-16, 0-17, 0-18, 0-19, 0-20, 0-21, 0-22, 0-23,
> 1-0, 1-1, 1-2, 1-3, 1-4, 1-5, 1-6, 2-0, 2-1, 2-2, 2-3, 2-4, 2-5, 2-6,
> 3-0, 3-1, 3-2, 3-3, 3-4, 3-5, 3-6, 3-7, 3-8, 3-9, 3-10, 3-11, 3-12,
> 3-13, 3-14, 3-15, 3-16, 3-17, 3-18, 3-19, 3-20, 3-21, 3-22, 3-23 not
> connected.
>
> there is some difference between using acpi or mptable...
>
> when mpatble is used, mp_irqs is all filled after mptable is parsed.
>
> when acpi is used, one [0-15) is filled..., later it will fill entries
> when acpi add entries...mp_register_gsi/mp_config_acpi_gsi
>
> YH
>

thanks for explanation Yinghai, i was reffering to MP and didn't take
a look on ACPI facility (that is why I've asked you). Thanks!

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Maciej W. Rozycki]

On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:

> Ingo, how about the following approach? We don't introduce new
> functions but rather srink the code by new printout form.

 Honestly, this one should probably use sprintf() or suchlike to avoid the
mess of printk() calls building a line of output from pieces.  It's quite
easy to calculate here what the maximum size of the buffer required could
be and automatic arrays can have variable size, so no need for the hassle
of heap management.  Calls to printk() without a trailing newline should
be avoided where possible as it messes up logging priority if a message
pops up from an interrupt inbetween.

  Maciej

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Ingo Molnar]

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

> On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
> 
> > Ingo, how about the following approach? We don't introduce new
> > functions but rather srink the code by new printout form.
> 
>  Honestly, this one should probably use sprintf() or suchlike to avoid the
> mess of printk() calls building a line of output from pieces.  It's quite
> easy to calculate here what the maximum size of the buffer required could
> be and automatic arrays can have variable size, so no need for the hassle
> of heap management.  Calls to printk() without a trailing newline should
> be avoided where possible as it messes up logging priority if a message
> pops up from an interrupt inbetween.

hm, is it worth the trouble? This is during very early init.

	Ingo

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Maciej W. Rozycki]

On Sat, 6 Sep 2008, Ingo Molnar wrote:

> >  Honestly, this one should probably use sprintf() or suchlike to avoid the
> > mess of printk() calls building a line of output from pieces.  It's quite
> > easy to calculate here what the maximum size of the buffer required could
> > be and automatic arrays can have variable size, so no need for the hassle
> > of heap management.  Calls to printk() without a trailing newline should
> > be avoided where possible as it messes up logging priority if a message
> > pops up from an interrupt inbetween.
> 
> hm, is it worth the trouble? This is during very early init.

 But is it really a trouble?  With an auto array the additional code is
minuscule -- mostly printk() calls replaced with sprintf() plus a variable
or two to maintain a pointer to the buffer which will go into registers
anyway.  Plus a pr_info("%s", buffer) or suchlike at the end.  I know 
laziness is a virtue, but you have to draw a line somewhere. ;)

 The benefit, apart from what I wrote above, is less chance of propagating
bad style to newcomers with each piece of such old code removed.

  Maciej

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Sat, Sep 06, 2008 at 08:49:14PM +0200]
| 
| * Maciej W. Rozycki <macro@linux-mips.org> wrote:
| 
| > On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| > 
| > > Ingo, how about the following approach? We don't introduce new
| > > functions but rather srink the code by new printout form.
| > 
| >  Honestly, this one should probably use sprintf() or suchlike to avoid the
| > mess of printk() calls building a line of output from pieces.  It's quite
| > easy to calculate here what the maximum size of the buffer required could
| > be and automatic arrays can have variable size, so no need for the hassle
| > of heap management.  Calls to printk() without a trailing newline should
| > be avoided where possible as it messes up logging priority if a message
| > pops up from an interrupt inbetween.
| 
| hm, is it worth the trouble? This is during very early init.
| 
| 	Ingo
| 

Ingo, Maciej,

here is an another attempt to satisfy the requirements :)
Please review and comment if it looks better or worse now.

		- Cyrill -

---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: [PATCH] x86: io-apic - cleanup of setup_IO_APIC_irqs v3

Use local buffer to accumulate all not connected pin info
on each io-apic and printout it if needed.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---


Index: linux-2.6.git/arch/x86/kernel/io_apic.c
===================================================================
--- linux-2.6.git.orig/arch/x86/kernel/io_apic.c	2008-09-07 10:10:15.000000000 +0400
+++ linux-2.6.git/arch/x86/kernel/io_apic.c	2008-09-07 13:50:46.000000000 +0400
@@ -1514,46 +1514,69 @@ static void setup_IO_APIC_irq(int apic, 
 	ioapic_write_entry(apic, pin, entry);
 }
 
+/* check and setup io-apic irq */
+static int __init setup_ioapic_irq(int apic, int pin)
+{
+	int idx, irq;
+
+	idx = find_irq_entry(apic, pin, mp_INT);
+	if (idx == -1)
+		return -1;
+
+	irq = pin_2_irq(idx, apic, pin);
+
+#ifdef CONFIG_X86_32
+	if (multi_timer_check(apic, irq))
+		return 0;
+#endif
+
+	add_pin_to_irq(irq, apic, pin);
+	setup_IO_APIC_irq(apic, pin, irq,
+			irq_trigger(idx), irq_polarity(idx));
+	return 0;
+}
+
 static void __init setup_IO_APIC_irqs(void)
 {
-	int apic, pin, idx, irq;
-	int notcon = 0;
+	int apic, pin;
+	char buf[64], *p;
+	int len;
 
 	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
 
+	p = buf, buf[0] = 0, len = 0;
+
 	for (apic = 0; apic < nr_ioapics; apic++) {
 		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
 
-			idx = find_irq_entry(apic, pin, mp_INT);
-			if (idx == -1) {
-				apic_printk(APIC_VERBOSE,
-					KERN_DEBUG " %d-%d",
-					mp_ioapics[apic].mp_apicid, pin);
-				if (!notcon)
-					notcon = 1;
+			if (!setup_ioapic_irq(apic, pin))
 				continue;
-			}
 
-			irq = pin_2_irq(idx, apic, pin);
-#ifdef CONFIG_X86_32
-			if (multi_timer_check(apic, irq))
+			/*
+			 * pin is not connected
+			 *  - we try to save this info in the buffer
+			 *    and print it out later
+			 *  - we don't expect too much not connected
+			 *    pins (as MP specification says the maximum
+			 *    value is 2 not connected pins) but if we
+			 *    have buggy BIOS or just get too many of them -
+			 *    truncate output with "..."
+			 */
+			if (!p)
 				continue;
-#endif
-			add_pin_to_irq(irq, apic, pin);
-
-			setup_IO_APIC_irq(apic, pin, irq,
-					irq_trigger(idx), irq_polarity(idx));
+			len += snprintf(p + len, sizeof(buf) - len, " %d-%d",
+					mp_ioapics[apic].mp_apicid, pin);
+			if (len >= sizeof(buf)) {
+				memcpy(&buf[sizeof(buf) - 4], "...", 3);
+				p = NULL;
+			}
 		}
-		if (notcon) {
-			apic_printk(APIC_VERBOSE,
-				KERN_DEBUG " (apicid-pin) not connected\n");
-			notcon = 0;
+		if (buf[0]) {
+			apic_printk(APIC_VERBOSE, KERN_DEBUG
+				" (apicid-pin) not connected:%s\n", buf);
+			p = buf, buf[0] = 0, len = 0;
 		}
 	}
-
-	if (notcon)
-		apic_printk(APIC_VERBOSE,
-			KERN_DEBUG " (apicid-pin) not connected\n");
 }
 
 /*

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> here is an another attempt to satisfy the requirements :)
> Please review and comment if it looks better or worse now.

sigh, i think the extra buffering is clearly worse. These are early init 
printks and we already do many other cases of multi-line printouts and 
even KERN_CONT printouts. We have a perfectly fine built-in buffer 
already: the printk buffer.

	Ingo

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Ingo Molnar - Sun, Sep 07, 2008 at 05:47:21PM +0200]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > here is an another attempt to satisfy the requirements :)
| > Please review and comment if it looks better or worse now.
| 
| sigh, i think the extra buffering is clearly worse. These are early init 
| printks and we already do many other cases of multi-line printouts and 
| even KERN_CONT printouts. We have a perfectly fine built-in buffer 
| already: the printk buffer.
| 
| 	Ingo
| 

ok Ingo, i think we could drop this idea then.

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100]
| On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| 
| > Ingo, how about the following approach? We don't introduce new
| > functions but rather srink the code by new printout form.
| 
|  Honestly, this one should probably use sprintf() or suchlike to avoid the
| mess of printk() calls building a line of output from pieces.  It's quite
| easy to calculate here what the maximum size of the buffer required could
| be and automatic arrays can have variable size, so no need for the hassle
| of heap management.  Calls to printk() without a trailing newline should
| be avoided where possible as it messes up logging priority if a message
| pops up from an interrupt inbetween.
| 
|   Maciej
| 

The easiest way would be just print this info on separate
lines like

	IO-APIC (apicid-pin) 1-1 not connected

and just drop all this troubles :)

I'm not sure how much memory we need for every io-apic
pins - iirc there only 32 redirection entry so it could
be about 32 bytes from stack would be enough. Will take
a look. Thanks Maciej! Ingo?

Btw, i think the problem is rather with (as Ingo mentoined)
too nesting cycles - so maybe we could use an iterator
for this? Maybe rcu based? As I see these global variables
are not supposed to be changed during iterations and if that
happens in future we could achieve rcu complains about it
and this prevent us for possible bug. Opinions? (just
a thoughts)

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Yinghai Lu]

On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100]
> | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
> |
> | > Ingo, how about the following approach? We don't introduce new
> | > functions but rather srink the code by new printout form.
> |
> |  Honestly, this one should probably use sprintf() or suchlike to avoid the
> | mess of printk() calls building a line of output from pieces.  It's quite
> | easy to calculate here what the maximum size of the buffer required could
> | be and automatic arrays can have variable size, so no need for the hassle
> | of heap management.  Calls to printk() without a trailing newline should
> | be avoided where possible as it messes up logging priority if a message
> | pops up from an interrupt inbetween.
> |
> |   Maciej
> |
>
> The easiest way would be just print this info on separate
> lines like
>
>        IO-APIC (apicid-pin) 1-1 not connected
>
> and just drop all this troubles :)
>
> I'm not sure how much memory we need for every io-apic
> pins - iirc there only 32 redirection entry so it could
> be about 32 bytes from stack would be enough. Will take
> a look. Thanks Maciej! Ingo?

no. some system could have 3 or 4 ioapic controller, and every one
have 24...(like three mcp55/io55)
4*24

or old system have 1 8111 and 7 8132. will have 32 + 7*2*7

YH

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Yinghai Lu - Sat, Sep 06, 2008 at 12:16:28PM -0700]
| On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100]
| > | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| > |
| > | > Ingo, how about the following approach? We don't introduce new
| > | > functions but rather srink the code by new printout form.
| > |
| > |  Honestly, this one should probably use sprintf() or suchlike to avoid the
| > | mess of printk() calls building a line of output from pieces.  It's quite
| > | easy to calculate here what the maximum size of the buffer required could
| > | be and automatic arrays can have variable size, so no need for the hassle
| > | of heap management.  Calls to printk() without a trailing newline should
| > | be avoided where possible as it messes up logging priority if a message
| > | pops up from an interrupt inbetween.
| > |
| > |   Maciej
| > |
| >
| > The easiest way would be just print this info on separate
| > lines like
| >
| >        IO-APIC (apicid-pin) 1-1 not connected
| >
| > and just drop all this troubles :)
| >
| > I'm not sure how much memory we need for every io-apic
| > pins - iirc there only 32 redirection entry so it could
| > be about 32 bytes from stack would be enough. Will take
| > a look. Thanks Maciej! Ingo?
| 
| no. some system could have 3 or 4 ioapic controller, and every one
| have 24...(like three mcp55/io55)
| 4*24
| 
| or old system have 1 8111 and 7 8132. will have 32 + 7*2*7
| 
| YH
| 

yep, I just calculated - we've to allocate too much for
early init.

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Yinghai Lu - Sat, Sep 06, 2008 at 12:16:28PM -0700]
| On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| > [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100]
| > | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| > |
| > | > Ingo, how about the following approach? We don't introduce new
| > | > functions but rather srink the code by new printout form.
| > |
| > |  Honestly, this one should probably use sprintf() or suchlike to avoid the
| > | mess of printk() calls building a line of output from pieces.  It's quite
| > | easy to calculate here what the maximum size of the buffer required could
| > | be and automatic arrays can have variable size, so no need for the hassle
| > | of heap management.  Calls to printk() without a trailing newline should
| > | be avoided where possible as it messes up logging priority if a message
| > | pops up from an interrupt inbetween.
| > |
| > |   Maciej
| > |
| >
| > The easiest way would be just print this info on separate
| > lines like
| >
| >        IO-APIC (apicid-pin) 1-1 not connected
| >
| > and just drop all this troubles :)
| >
| > I'm not sure how much memory we need for every io-apic
| > pins - iirc there only 32 redirection entry so it could
| > be about 32 bytes from stack would be enough. Will take
| > a look. Thanks Maciej! Ingo?
| 
| no. some system could have 3 or 4 ioapic controller, and every one
| have 24...(like three mcp55/io55)
| 4*24
| 
| or old system have 1 8111 and 7 8132. will have 32 + 7*2*7
| 
| YH
|

Didn't really understand all numbers :)
We have
	24	io-apic route entries
	128	io-apic maximum in quantity

so the worst (hardly possible if ever) is when on last io-apic
all pins are wrong. So for this case we would need

	(3 + 1) * ((10 + 1) + (13 + 1)) = 100

where
	3 - strlen("127") - max io-apic number
	10 - Sum(strlen(0)+...+strlen(9)) - pin numbers
	13 - same sum for two-digit numbers
	+1 number - either "-" sign or " "

100 bytes to print not connected pins. Ugh :-)

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Cyrill Gorcunov - Sat, Sep 06, 2008 at 11:38:20PM +0400]
| [Yinghai Lu - Sat, Sep 06, 2008 at 12:16:28PM -0700]
| | On Sat, Sep 6, 2008 at 12:04 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| | > [Maciej W. Rozycki - Sat, Sep 06, 2008 at 07:45:08PM +0100]
| | > | On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| | > |
| | > | > Ingo, how about the following approach? We don't introduce new
| | > | > functions but rather srink the code by new printout form.
| | > |
| | > |  Honestly, this one should probably use sprintf() or suchlike to avoid the
| | > | mess of printk() calls building a line of output from pieces.  It's quite
| | > | easy to calculate here what the maximum size of the buffer required could
| | > | be and automatic arrays can have variable size, so no need for the hassle
| | > | of heap management.  Calls to printk() without a trailing newline should
| | > | be avoided where possible as it messes up logging priority if a message
| | > | pops up from an interrupt inbetween.
| | > |
| | > |   Maciej
| | > |
| | >
| | > The easiest way would be just print this info on separate
| | > lines like
| | >
| | >        IO-APIC (apicid-pin) 1-1 not connected
| | >
| | > and just drop all this troubles :)
| | >
| | > I'm not sure how much memory we need for every io-apic
| | > pins - iirc there only 32 redirection entry so it could
| | > be about 32 bytes from stack would be enough. Will take
| | > a look. Thanks Maciej! Ingo?
| | 
| | no. some system could have 3 or 4 ioapic controller, and every one
| | have 24...(like three mcp55/io55)
| | 4*24
| | 
| | or old system have 1 8111 and 7 8132. will have 32 + 7*2*7
| | 
| | YH
| |
| 
| Didn't really understand all numbers :)
| We have
| 	24	io-apic route entries
| 	128	io-apic maximum in quantity
| 
| so the worst (hardly possible if ever) is when on last io-apic
| all pins are wrong. So for this case we would need
| 
| 	(3 + 1) * ((10 + 1) + (13 + 1)) = 100
| 
| where
| 	3 - strlen("127") - max io-apic number
| 	10 - Sum(strlen(0)+...+strlen(9)) - pin numbers
| 	13 - same sum for two-digit numbers
| 	+1 number - either "-" sign or " "
| 
| 100 bytes to print not connected pins. Ugh :-)
| 
| 		- Cyrill -

But if one day hardware changed - we'll be in troubles :)
So I think better would be just use Ingo's suggestion
about to separate apic/pin iterator and function body.

		- Cyrill -

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Maciej W. Rozycki]

On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:

> So I think better would be just use Ingo's suggestion
> about to separate apic/pin iterator and function body.

 No doubt about it.  Then if the number of entries was to be huge, then
lines output to the log buffer could be limited to 80 characters.  With
the use of sprintf() it would be rather trivial.  And you would avoid any
concerns about stack consumption.

 Please note that with MPS 1.1 systems the number of "unconnected" I/O 
APIC inputs can be considerable.

  Maciej

Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sat, Sep 06, 2008 at 09:08:51PM +0100]
| On Sat, 6 Sep 2008, Cyrill Gorcunov wrote:
| 
| > So I think better would be just use Ingo's suggestion
| > about to separate apic/pin iterator and function body.
| 
|  No doubt about it.  Then if the number of entries was to be huge, then
| lines output to the log buffer could be limited to 80 characters.  With
| the use of sprintf() it would be rather trivial.  And you would avoid any
| concerns about stack consumption.
| 
|  Please note that with MPS 1.1 systems the number of "unconnected" I/O 
| APIC inputs can be considerable.
| 
|   Maciej
| 

Thanks Maciej, will continue tomorrow.

		- Cyrill -