Messages in this thread | | | Date | Fri, 5 Sep 2008 22:33:47 +0400 | From | Cyrill Gorcunov <> | Subject | Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC_irqs |
| |
[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 -
| |