linux-kernel - Re: [patch 3/3] x86: io-apic - code style cleaning for setup_IO_APIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080905181111.GG27395@elte.hu>
Date:	Fri, 5 Sep 2008 20:11:11 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Cyrill Gorcunov <gorcunov@...il.com>
Cc:	hpa@...or.com, linux-kernel@...r.kernel.org, tglx@...utronix.de,
	yhlu.kernel@...il.com, macro@...ux-mips.org
Subject: Re: [patch 3/3] x86: io-apic - code style cleaning for
	setup_IO_APIC_irqs


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

> [Ingo Molnar - Fri, Sep 05, 2008 at 10:04:47AM +0200]
> | 
> | * Cyrill Gorcunov <gorcunov@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/