linux-kernel - Re: [PATCH 1/6] ACPI: Minimize X2APIC initial messages

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: <alpine.DEB.2.00.1102191637290.27722@chino.kir.corp.google.com>
Date:	Sat, 19 Feb 2011 17:50:51 -0800 (PST)
From:	David Rientjes <rientjes@...gle.com>
To:	Mike Travis <travis@....com>
cc:	Ingo Molnar <mingo@...e.hu>, Jack Steiner <steiner@....com>,
	Robin Holt <holt@....com>, Len Brown <len.brown@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Yinghai Lu <yhlu.kernel@...il.com>, linux-acpi@...r.kernel.org,
	x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] ACPI: Minimize X2APIC initial messages

On Fri, 18 Feb 2011, Mike Travis wrote:

> Minimize X2APIC messages by printing 8 per line and dropping
> the "enabled" flag since that's assumed.  It will still print
> "disabled" if necessary.
> 
> v2: updated to apply to x86-tip
> 

For each patch in this series, it would be tremendously helpful to show 
what format the current output is in and what the format is after the 
patch is applied.

> Signed-off-by: Mike Travis <travis@....com>
> Reviewed-by: Jack Steiner <steiner@....com>
> Reviewed-by: Robin Holt <holt@....com>
> ---
>  arch/x86/kernel/acpi/boot.c |    3 +++
>  drivers/acpi/tables.c       |   16 +++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> --- linux.orig/arch/x86/kernel/acpi/boot.c
> +++ linux/arch/x86/kernel/acpi/boot.c
> @@ -903,6 +903,9 @@ static int __init acpi_parse_madt_lapic_
>  	if (!count) {
>  		x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
>  					acpi_parse_x2apic, MAX_LOCAL_APIC);
> +		/* insure trailing newline is output */

s/insure/ensure/

> +		pr_cont("\n");

I know that this is the only code that passes ACPI_MADT_TYPE_LOCAL_X2APIC.  
That said, this line really has no place in the caller.

> +
>  		count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
>  					acpi_parse_lapic, MAX_LOCAL_APIC);
>  	}
> --- linux.orig/drivers/acpi/tables.c
> +++ linux/drivers/acpi/tables.c
> @@ -66,11 +66,17 @@ void acpi_table_print_madt_entry(struct
>  		{
>  			struct acpi_madt_local_x2apic *p =
>  			    (struct acpi_madt_local_x2apic *)header;
> -			printk(KERN_INFO PREFIX
> -			       "X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
> -			       p->local_apic_id, p->uid,
> -			       (p->lapic_flags & ACPI_MADT_ENABLED) ?
> -			       "enabled" : "disabled");
> +
> +			if ((p->uid & 7) == 0)
> +				pr_info(PREFIX "X2APIC apic_id=uid:");
> +
> +			pr_cont(" %02x=%02x%s%s",
> +				p->local_apic_id, p->uid,
> +				/* assume "enabled" unless "disabled" */
> +				(p->lapic_flags & ACPI_MADT_ENABLED) ?
> +					"" : " disabled",

Because you're printing only " disabled" when ACPI_MADT_ENABLED is not 
set, this seems like the format of the line would be ambiguous with regard 
to which entry it applies to.  I could imagine a line such as

	X2APIC apic_id=uid: 01=01 disabled 02=02

and then we're left wondering which entry is actually disabled.  I'd 
prefer "01=01(disabled) 02=02" instead.

Also, why did you drop the "0x" prefixes from the current format?

> +				/* nl every 8th item */
> +				(p->uid & 7) == 7 ? "\n" : "");
>  		}
>  		break;
>  
--
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/