[PATCH] x86: fix apic.h unused but set warnings v2

[PATCH] x86: fix apic.h unused but set warnings v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Fix

linux-2.6/arch/x86/include/asm/apic.h: In function 'native_apic_msr_read':
linux-2.6/arch/x86/include/asm/apic.h:144:11: warning: variable 'high' set but not used [-Wunused-but-set-variable]
linux-2.6/arch/x86/include/asm/apic.h: In function 'x2apic_enabled':
linux-2.6/arch/x86/include/asm/apic.h:184:11: warning: variable 'msr2' set but not used [-Wunused-but-set-variable]

which happen with a gcc 4.6 build

Since this is in a frequently included header these warnings are printed
very frequently and make a gcc 4.6 build very noisy.

v2: Add cast and change types based on review feedback.

Cc: x86@kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/include/asm/apic.h |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 286de34..e5472de 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -141,13 +141,13 @@ static inline void native_apic_msr_write(u32 reg, u32 v)
 
 static inline u32 native_apic_msr_read(u32 reg)
 {
-	u32 low, high;
+	u32 low;
 
 	if (reg == APIC_DFR)
 		return -1;
 
-	rdmsr(APIC_BASE_MSR + (reg >> 4), low, high);
-	return low;
+	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
+	return (u32)low;
 }
 
 static inline void native_x2apic_wait_icr_idle(void)
@@ -181,12 +181,12 @@ extern void enable_x2apic(void);
 extern void x2apic_icr_write(u32 low, u32 id);
 static inline int x2apic_enabled(void)
 {
-	int msr, msr2;
+	u32 msr;
 
 	if (!cpu_has_x2apic)
 		return 0;
 
-	rdmsr(MSR_IA32_APICBASE, msr, msr2);
+	rdmsrl(MSR_IA32_APICBASE, msr);
 	if (msr & X2APIC_ENABLE)
 		return 1;
 	return 0;
-- 
1.7.1

Re: [PATCH] x86: fix apic.h unused but set warnings v2

[Thomas Gleixner]

On Mon, 8 Nov 2010, Andi Kleen wrote:
>  static inline u32 native_apic_msr_read(u32 reg)
>  {
> -	u32 low, high;
> +	u32 low;
>  
>  	if (reg == APIC_DFR)
>  		return -1;
>  
> -	rdmsr(APIC_BASE_MSR + (reg >> 4), low, high);
> -	return low;
> +	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
> +	return (u32)low;

What's the point of casting u32 to u32 ?

Thanks,

	tglx

Re: [PATCH] x86: fix apic.h unused but set warnings v2

[Andi Kleen]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Mon, 8 Nov 2010, Andi Kleen wrote:
>>  static inline u32 native_apic_msr_read(u32 reg)
>>  {
>> -	u32 low, high;
>> +	u32 low;
>>  
>>  	if (reg == APIC_DFR)
>>  		return -1;
>>  
>> -	rdmsr(APIC_BASE_MSR + (reg >> 4), low, high);
>> -	return low;
>> +	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
>> +	return (u32)low;
>
> What's the point of casting u32 to u32 ?

One of the earlier reviewers thought adding an explicit cast would make
the truncation in the code clearer.  I didn't full agree either, but 
still did the change.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] x86: fix apic.h unused but set warnings v2

[Cyrill Gorcunov]

On Mon, Nov 08, 2010 at 10:57:30PM +0100, Andi Kleen wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> 
> > On Mon, 8 Nov 2010, Andi Kleen wrote:
> >>  static inline u32 native_apic_msr_read(u32 reg)
> >>  {
> >> -	u32 low, high;
> >> +	u32 low;
> >>  
> >>  	if (reg == APIC_DFR)
> >>  		return -1;
> >>  
> >> -	rdmsr(APIC_BASE_MSR + (reg >> 4), low, high);
> >> -	return low;
> >> +	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
> >> +	return (u32)low;
> >
> > What's the point of casting u32 to u32 ?
> 
> One of the earlier reviewers thought adding an explicit cast would make
> the truncation in the code clearer.  I didn't full agree either, but 
> still did the change.
> 
> -Andi

Well, Andi I proposed to define variable as u64 and convert it back to u32 at procedure
exit point. That would be clean, and I still think so ;) Though I'm fine with
either way (I just thought about ones who will be reading this code in future,
and since most the rdmsrl callers already use u64 and unsigned long, this would
be consistent as well).

  Cyrill

Re: [PATCH] x86: fix apic.h unused but set warnings v2

[Andi Kleen]

> Well, Andi I proposed to define variable as u64 and convert it back to u32 at procedure
> exit point. That would be clean, and I still think so ;) Though I'm fine with
> either way (I just thought about ones who will be reading this code in future,
> and since most the rdmsrl callers already use u64 and unsigned long, this would
> be consistent as well).

I didn't use u64 because that would use more code on i386

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] x86: fix apic.h unused but set warnings v2

[Cyrill Gorcunov]

On Tue, Nov 09, 2010 at 02:50:51AM +0100, Andi Kleen wrote:
> > Well, Andi I proposed to define variable as u64 and convert it back to u32 at procedure
> > exit point. That would be clean, and I still think so ;) Though I'm fine with
> > either way (I just thought about ones who will be reading this code in future,
> > and since most the rdmsrl callers already use u64 and unsigned long, this would
> > be consistent as well).
> 
> I didn't use u64 because that would use more code on i386
> 
> -Andi

Fair enough. Thanks Andi.

  Cyrill