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

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

[Andi Kleen]

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

[Andrew, can you please put this in your tree for submission.
This was submitted some time ago, but x86@ is a blackhole unfortunately and
this warnings really makes gcc 4.6 extremly noisy.]

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.

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

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 286de34..9c4e06b 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -141,12 +141,12 @@ 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);
+	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
 	return low;
 }
 
@@ -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;
+	int 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

[Cyrill Gorcunov]

On Sun, Nov 07, 2010 at 08:53:26PM +0100, Andi Kleen wrote:
... 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 286de34..9c4e06b 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -141,12 +141,12 @@ 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);
> +	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
>  	return low;
>  }

 At least the former code _didn't_ hide the details of which
part of 'long long' value we need. Would not

 u64 val;
 rdmsrl(APIC_BASE_MSR + (reg >> 4), val);
 return (u32)val;

be more clear?

 Cyrill

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

[Andi Kleen]

>  At least the former code _didn't_ hide the details of which
> part of 'long long' value we need. Would not
> 
>  u64 val;
>  rdmsrl(APIC_BASE_MSR + (reg >> 4), val);
>  return (u32)val;
> 
> be more clear?

Maybe, but I doubt it will make much difference either way.

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

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

[Thomas Gleixner]

On Sun, 7 Nov 2010, Andi Kleen wrote:
> 
> [Andrew, can you please put this in your tree for submission.
> This was submitted some time ago, but x86@ is a blackhole unfortunately and

You are about to create a blackhole by earning yourself an entry in my
killfile. I'm really fed up with your sneaky and priggish attacks.

Just for the record: The patch came in while I was traveling. I saw it
when I returned which was a day before I left for Boston and I queued
it in my for-tip mbox, but I did not work much during last week.

So you are artificially inflating a 12 days delay (9 days are caused
by travel and a conference where you were present as well) to an
outrageous incident.

A polite reminder would have been appropriate, but it seems that you
are more interested in confrontation than in collaboration.

Thanks,

	tglx

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

[Andi Kleen]

On 11/8/2010 8:36 PM, Thomas Gleixner wrote:
> On Sun, 7 Nov 2010, Andi Kleen wrote:
>> [Andrew, can you please put this in your tree for submission.
>> This was submitted some time ago, but x86@ is a blackhole unfortunately and
> You are about to create a blackhole by earning yourself an entry in my
> killfile. I'm really fed up with your sneaky and priggish attacks.

Hi Thomas,

It's unfortunately not the only patch to which this happened. Losing patches
is very common for x86@ based on my experience, the rate seems to be far
far above 50%, more often even higher. I don't know what the problem
is either, but it's very visible to me.

I would certainly not make such a comment without having
sufficient data to back it up.

In fact I don't ping most patches, but had to do it for this one
because the problem is just too noticeable.

Anyways it's easy to avoid such comments too: don't lose patches as often
I would be happy if I didn't need to send any of these reminders. However
at this loss rate you cannot expect nice comments.

-Andi

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

[Thomas Gleixner]

B1;2401;0cOn Mon, 8 Nov 2010, Andi Kleen wrote:
> On 11/8/2010 8:36 PM, Thomas Gleixner wrote:
> > On Sun, 7 Nov 2010, Andi Kleen wrote:
> > > [Andrew, can you please put this in your tree for submission.
> > > This was submitted some time ago, but x86@ is a blackhole unfortunately
> > > and
> > You are about to create a blackhole by earning yourself an entry in my
> > killfile. I'm really fed up with your sneaky and priggish attacks.
> 
> Hi Thomas,
> 
> It's unfortunately not the only patch to which this happened. Losing patches
> is very common for x86@ based on my experience, the rate seems to be far
> far above 50%, more often even higher. I don't know what the problem
> is either, but it's very visible to me.
> 
> I would certainly not make such a comment without having
> sufficient data to back it up.

Then let's look at the data. 

All patches which are cc'ed to x86@kernel.org or tglx@linutronix.de
end up in a separate mbox. Here is the list since 01/01/2010 with the
reaction times documented

Subject: [PATCH] [2/9] Add a kernel_address() that works for data too

 Not x86 specific

Subject: [PATCH] x86: mce: Xeon75xx specific interface to get corrected

 Commit-ID:  c773f70fd6b53ee646727f871833e53649907264
 Gitweb:     http://git.kernel.org/tip/c773f70fd6b53ee646727f871833e53649907264
 Author:     Andi Kleen <andi@firstfloor.org>
 AuthorDate: Thu, 21 Jan 2010 23:17:12 +0100
 Committer:  H. Peter Anvin <hpa@zytor.com>
 CommitDate: Thu, 21 Jan 2010 17:38:20 -0800

Subject: [PATCH] Add Xeon 7500 series support to oprofile

 Commit-ID:  e83e452b0692c9c13372540deb88a77d4ae2553d
 Gitweb:     http://git.kernel.org/tip/e83e452b0692c9c13372540deb88a77d4ae2553d
 Author:     Andi Kleen <andi@firstfloor.org>
 AuthorDate: Thu, 21 Jan 2010 23:26:27 +0100
 Committer:  Robert Richter <robert.richter@amd.com>
 CommitDate: Mon, 25 Jan 2010 15:34:53 +0100

Subject: Re: [tip:x86/mce] x86, mce: Rename cpu_specific_poll to mce_cpu_specific_poll

 Commit-ID:  757fd770c649b0dfa6eeefc2d5e2ea3119b6be9c
 Gitweb:     http://git.kernel.org/tip/757fd770c649b0dfa6eeefc2d5e2ea3119b6be9c
 Author:     Andi Kleen <andi@firstfloor.org>
 AuthorDate: Sat, 23 Jan 2010 12:33:59 +0100
 Committer:  H. Peter Anvin <hpa@zytor.com>
 CommitDate: Fri, 5 Feb 2010 14:51:53 -0800

Subject: [PATCH] HPET: Drop WARN_ON for mismatch on HPET CMP readback

 Solved differently after technical discussion with
  commit 8da854cb02156c90028233ae1e85ce46a1d3f82c
  Author: Pallipadi, Venkatesh <venkatesh.pallipadi@intel.com>
  Date:   Thu Feb 25 10:53:48 2010 -0800

Subject: [PATCH] Allow Intel platforms to declare unsynchronized TSC to

  Seems to be missed, but no reminder

Subject: [PATCH] x86: mce: Xeon75xx specific interface to get corrected

  Questioned by Hidetoshi.

Subject: [PATCH] Prevent nested interrupts when the IRQ stack is near

  Solved differently.

Subject: [PATCH] [REGRESSION 2.6.33] x86: Handle overlapping mptables

  Commit-ID:  909fc87b32b3b9e3f0b87dcc5d98319c41900c58
  Gitweb:     http://git.kernel.org/tip/909fc87b32b3b9e3f0b87dcc5d98319c41900c58
  Author:     Andi Kleen <andi@firstfloor.org>
  AuthorDate: Mon, 29 Mar 2010 09:41:11 +0200
  Committer:  H. Peter Anvin <hpa@zytor.com>
  CommitDate: Thu, 1 Apr 2010 13:31:07 -0700

Subject: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu

  Commit-ID:  157a9df3b0f8042f1a6a4fe9e0e494431aaca1ae
  Gitweb:     http://git.kernel.org/tip/157a9df3b0f8042f1a6a4fe9e0e494431aaca1ae
  Author:     Andi Kleen <andi@firstfloor.org>
  AuthorDate: Thu, 10 Jun 2010 13:10:36 +0200
  Committer:  Ingo Molnar <mingo@elte.hu>
  CommitDate: Thu, 10 Jun 2010 14:09:30 +0200

Subject: [PATCH] [2/23] IRQ: Move alloc_desk_mask variables inside ifdef

 Commit-ID:  1813a68457bb45b378d5bbec615b167deff3bcfc
 Gitweb:     http://git.kernel.org/tip/1813a68457bb45b378d5bbec615b167deff3bcfc
 Author:     Andi Kleen <andi@xxxxxxxxxxxxxx>
 AuthorDate: Tue, 20 Jul 2010 15:22:54 -0700
 Committer:  Thomas Gleixner <tglx@xxxxxxxxxxxxx>
 CommitDate: Tue, 27 Jul 2010 20:43:05 +0200

Subject: [PATCH] [3/23] x86: Avoid unused by set variables in rdmsr

 Commit-ID:  5f755293ca61520b70b11afe1b1d6e1635cb6c00
 Gitweb:     http://git.kernel.org/tip/5f755293ca61520b70b11afe1b1d6e1635cb6c00
 Author:     Andi Kleen <andi@firstfloor.org>
 AuthorDate: Tue, 20 Jul 2010 15:19:48 -0700
 Committer:  H. Peter Anvin <hpa@linux.intel.com>
 CommitDate: Tue, 20 Jul 2010 15:38:18 -0700

Subject: [PATCH] [7/23] x86: fix set but not read variables

 Commit-ID:  fa10ba64ac94fec4611b79804023eb087862ffe0
 Gitweb:     http://git.kernel.org/tip/fa10ba64ac94fec4611b79804023eb087862ffe0
 Author:     Andi Kleen <andi@xxxxxxxxxxxxxx>
 AuthorDate: Tue, 20 Jul 2010 15:19:49 -0700
 Committer:  H. Peter Anvin <hpa@xxxxxxxxxxxxxxx>
 CommitDate: Tue, 20 Jul 2010 15:38:30 -0700

Subject: [PATCH] Remove implicit list prefetches for most cases
Subject: [PATCH 1/2] Add for_each_module iterator function
Subject: [PATCH 2/2] Rewrite jump_label.c to use binary search
Subject: [PATCH] Convert preempt_disabled in module.c to rcu read lock
Subject: [PATCH 1/2] Add for_each_module iterator function v2
Subject: [PATCH 2/2] Rewrite jump_label.c to use binary search v2

 Not x86 patches. Separate discussion and not my topic.

Subject: [PATCH 1/2] Encode huge page size for VM_FAULT_HWPOISON errors
Subject: [PATCH 2/2] x86: HWPOISON: Report correct address granuality for huge hwpoison faults

 Missed, no polite reminder. Just a nastigram in the pull request,
 nevertheless acked by Ingo.

Subject: [PATCH] Fix written but not read warnings for x2apic.h

 Delayed by 12 days for known reasons

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

 Nastigram with a changed subject line

To summarize:

   - There is a single patch missed in 2010
     [PATCH] Allow Intel platforms to declare unsynchronized TSC to

   - There were two patches not reviewed, but pushed by yourself and
     acked after Ingo noticed

     Subject: [PATCH 1/2] Encode huge page size for VM_FAULT_HWPOISON errors
     Subject: [PATCH 2/2] x86: HWPOISON: Report correct address granuality for huge \
hwpoison faults

   - There were eight patches applied within a few days after posting

   - There was one patch questioned by Hidetoshi and we never got a
     resolution

   - There were two patches which got not applied because the
     discussion resolved it differently

So that makes _ONE_ patch out of 14 being missed. That's hardly close
to 50%.

> In fact I don't ping most patches, but had to do it for this one
> because the problem is just too noticeable.
>
> Anyways it's easy to avoid such comments too: don't lose patches as often
> I would be happy if I didn't need to send any of these reminders. However

Reminder?

A reminder looks like this: "Hi, this is a resend of the patch I sent
<date>, and was wondering if it got lost during your trip to
KS/Plumbers" or "ping, did this get lost ?"

What you wrote was an attack instead. One in a long series of sneaky
attacks. It's just the last one I'm going to read.

> at this loss rate you cannot expect nice comments.

Consider your loss rate 100% from now on. You managed to be the 3rd
person who made it into my killfile in 15 years. Feel well in the
company of Uwe Bugla and Florian Mickler. That's an achievement!

Thanks,

	tglx

Public apology

[Andi Kleen]

Hi Thomas,

I just wanted to apologize for my earlier comments. As you pointed
out I was wrong regarding the loss rate for 2010. And the comments
about "blackhole" were not justified. And yes I should have
done at least one more ping.

Sorry about that. I'll try to do better in the future.

-Andi

Re: Public apology

[Thomas Gleixner]

Andi,

On Tue, 9 Nov 2010, Andi Kleen wrote:
> Hi Thomas,
> 
> I just wanted to apologize for my earlier comments. As you pointed
> out I was wrong regarding the loss rate for 2010. And the comments

You certainly understand that I'm cautious and that I could read the
above as "... for 2010, but not for the time before."

As an inveterate optimist I assume you meant to include the time
before 2010 as well, right?  If I'm wrong, then you better speak up
now and we sort it out once and forever.

> about "blackhole" were not justified. And yes I should have
> done at least one more ping.
> 
> Sorry about that. I'll try to do better in the future.

Apology accepted. Let's work from here and see how it evolves.

Thanks,

	tglx

Re: Public apology

[Andi Kleen]

[Sorry for the late answer. Was out of the office]

Hi Thomas,

On Tue, Nov 09, 2010 at 09:54:57PM +0100, Thomas Gleixner wrote:
> Andi,
> 
> On Tue, 9 Nov 2010, Andi Kleen wrote:
> > Hi Thomas,
> > 
> > I just wanted to apologize for my earlier comments. As you pointed
> > out I was wrong regarding the loss rate for 2010. And the comments
> 
> You certainly understand that I'm cautious and that I could read the
> above as "... for 2010, but not for the time before."

I frankly don't know (clearly my recollection and accounting on this
is untrustworthy). But I suspect you were right and I was wrong
for before 2010 too.

-Andi

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

[Thomas Gleixner]

On Sun, 7 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);
> +	rdmsrl(APIC_BASE_MSR + (reg >> 4), low);
>  	return low;
>  }
>  
> @@ -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;
> +	int msr;

Please make this an u32 while at it.

Thanks,

	tglx