Re: [Kernel-janitors] [PATCH] IO-APIC debug message reduction at

[Arnaldo Carvalho de Melo <acme@conectiva.com.br>]

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

Em Tue, Jul 06, 2004 at 09:27:08PM +0100, Mark Broadbent escreveu:
> Hi All,
> 
> Below is a patch that reduces the amount of debug information given out
> by the IO-APIC initialisation.
> 
> Comments welcome, please be gentle as this is my first patch.
> 
> Thanks
> Mark
 
comments: 

> --- linux-2.6.7/arch/i386/kernel/io_apic.c	2004-07-06 21:07:29.000000000 +0100
> +++ linux/arch/i386/kernel/io_apic.c	2004-07-06 21:15:26.000000000 +0100
> @@ -84,6 +84,17 @@
>  #define vector_to_irq(vector)	(vector)
>  #endif
>  
> +#define IOAPIC_QUIET   (0)
> +#define IOAPIC_VERBOSE (1)
> +#define IOAPIC_DEBUG   (2)

What is the point of those parens?

> +
> +/*
> + * Define the default level of output to be very little
> + * This can be turned up by using apic=verbose for more
> + * information and apic=debug for _lots_ of information.
> + */
> +static int ioapic_verbosity = 0;
> +

no need to init static variables to zero, not doing so has the advantage of
moving this variable to the .bss kernel section.

>  /*
>   * The common case is 1:1 IRQ<->pin mappings. Sometimes there are
>   * shared ISA-space IRQs, so we have to support them. We are super
> @@ -734,6 +745,26 @@
>  
>  __setup("noapic", ioapic_setup);
>  
> +static int __init ioapic_set_verbosity(char *str)
> +{
> +	if (strcmp("debug", str) == 0)
> +	{
> +		ioapic_verbosity = IOAPIC_DEBUG;
> +	}

follow kernel coding style:

if (condition) {
	j--;
	bleh();
} else if (condition) {
	i++;
	blah();
}

Also why the braces?

> +	else if (strcmp("verbose", str) == 0)
> +	{
> +		ioapic_verbosity = IOAPIC_VERBOSE;
> +	}
> +	else
> +	{
> +		printk(KERN_WARNING "IO-APIC Verbosity level %s not recognised, use ioapic=verbose or ioapic=debug", str);
> +	}
> +
> +	return 0;
> +}
> +
> +__setup("ioapic=", ioapic_set_verbosity);
> +
>  static int __init ioapic_pirq_setup(char *str)
>  {
>  	int i, max;
> @@ -745,13 +776,16 @@
>  		pirq_entries[i] = -1;
>  
>  	pirqs_enabled = 1;
> -	printk(KERN_INFO "PIRQ redirection, working around broken MP-BIOS.\n");
> +
> +	if (ioapic_verbosity > IOAPIC_QUIET)
> +		printk(KERN_INFO "PIRQ redirection, working around broken MP-BIOS.\n");

Humm, why not having something like ioapic_printk(), just like all the
dprintk in lots of places instead of testing ioapic_verbosity all the time,
polluting kernel sources? Sure, there are some places where directly
testing against ioapic_verbosity is better, like on a function that only
prints information... wait, this is the definition of ioapic_printk() :-)

Stopping here.

Regards,

- Arnaldo

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors