linux-kernel - Re: *PING* [PATCH]: x86: mce: fix mce warning with disabled lapic

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]

Date:	Wed, 12 Aug 2009 13:59:20 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Andi Kleen <andi@...stfloor.org>
Cc:	Johannes Stezenbach <js@...21.net>, x86@...nel.org,
	linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rjw@...k.pl>
Subject: Re: *PING* [PATCH]: x86: mce: fix mce warning with disabled lapic

* Andi Kleen <andi@...stfloor.org> wrote:

> *PING* can someone please take the patch in there and process the 
> other suggestions?

Firstly, a basic patch submission technical matter: could you please 
stop spamming maintainers of the x86/MCE code with such '*PING*' 
private mails, for patches you never properly submitted to begin 
with?

The proper way to submit an upstream kernel fix, as you should well 
be aware of, is to send a patch with a proper title and to Cc: it to 
lkml and the maintainers affected. You never did that, you only 
posted a for-testing patch into a discussion. Please stop this 
self-important posturing, it's somewhat annoying.

Also, another, patch log quality issue, please credit Johannes 
properly. You put this into the changelog:

> Originally reported by Johannes Stezenbach 
> 
> This is a 2.6.31 candidate because it fixes a regression.
> 
> Signed-off-by: Andi Kleen <ak@...ux.intel.com>

The proper way is to put this into the changelog:

  Reported-by: Johannes Stezenbach <js@...21.net>

And given that Johannes also tested the patch, another line of:

  Tested-by: Johannes Stezenbach <js@...21.net>

Would be appropriate as well.

Thirdly, we can do better with the fix itself too:

> +++ linux/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -236,6 +236,9 @@ void intel_init_thermal(struct cpuinfo_x
>  	int tm2 = 0;
>  	u32 l, h;
>  
> +	if (!cpu_has_apic || disable_apic)
> +		return;
> +
>  	/* Thermal monitoring depends on ACPI and clock modulation*/
>  	if (!cpu_has(c, X86_FEATURE_ACPI) || !cpu_has(c, X86_FEATURE_ACC))
>  		return;

we already have that X86_FEATURE_ACPI and X86_FEATURE_ACC check and 
a return statement. Would be better to expand that with the APIC 
checks. Plus update the comment to also mention APIC as a 
requirement plus fix the small error in the comment too while at it.

If these problems are fixed i'll apply the fix to tip:x86/urgent.

Thanks,

	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/