All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch] cpu: remove some dead code in store_cache_disable()
@ 2012-04-19  7:00 ` Dan Carpenter
  0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2012-04-19  7:00 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, Frank Arnold, Borislav Petkov,
	linux-kernel, kernel-janitors

amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
or zero.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed..7b4e294 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 		return -EINVAL;
 
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
-	if (err) {
-		if (err = -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+	if (err)
 		return err;
-	}
+
 	return count;
 }
 

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [patch] cpu: remove some dead code in store_cache_disable()
@ 2012-04-19  7:00 ` Dan Carpenter
  0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2012-04-19  7:00 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, x86, Frank Arnold, Borislav Petkov,
	linux-kernel, kernel-janitors

amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
or zero.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed..7b4e294 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 		return -EINVAL;
 
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
-	if (err) {
-		if (err == -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+	if (err)
 		return err;
-	}
+
 	return count;
 }
 

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
  2012-04-19  7:00 ` Dan Carpenter
@ 2012-04-19  8:30   ` Srivatsa S. Bhat
  -1 siblings, 0 replies; 19+ messages in thread
From: Srivatsa S. Bhat @ 2012-04-19  8:18 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Frank Arnold,
	Borislav Petkov, linux-kernel, kernel-janitors

On 04/19/2012 12:30 PM, Dan Carpenter wrote:

> amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
> or zero.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed..7b4e294 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>  		return -EINVAL;
> 
>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
> -	if (err) {
> -		if (err == -EEXIST)
> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> -					    slot);
> +	if (err)
>  		return err;
> -	}
> +
>  	return count;
>  }
> 


Looking at the comments around the code and the print statement your patch
is trying to remove, I wonder if it would be more appropriate to return
-EEXIST in amd_set_l3_disable_slot(), like this:

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] cpu: Fix error return code in amd_set_l3_disable_slot()

If the L3 disable slot is already in use, return -EEXIST instead of -EINVAL.
The caller, store_cache_disable(), checks this return value to print an
appropriate warning.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/x86/kernel/cpu/intel_cacheinfo.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed..0b49e29 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,7 +433,7 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;


Regards,
Srivatsa S. Bhat


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
@ 2012-04-19  8:30   ` Srivatsa S. Bhat
  0 siblings, 0 replies; 19+ messages in thread
From: Srivatsa S. Bhat @ 2012-04-19  8:30 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Frank Arnold,
	Borislav Petkov, linux-kernel, kernel-janitors

On 04/19/2012 12:30 PM, Dan Carpenter wrote:

> amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
> or zero.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed..7b4e294 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>  		return -EINVAL;
> 
>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
> -	if (err) {
> -		if (err = -EEXIST)
> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> -					    slot);
> +	if (err)
>  		return err;
> -	}
> +
>  	return count;
>  }
> 


Looking at the comments around the code and the print statement your patch
is trying to remove, I wonder if it would be more appropriate to return
-EEXIST in amd_set_l3_disable_slot(), like this:

---
From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] cpu: Fix error return code in amd_set_l3_disable_slot()

If the L3 disable slot is already in use, return -EEXIST instead of -EINVAL.
The caller, store_cache_disable(), checks this return value to print an
appropriate warning.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 arch/x86/kernel/cpu/intel_cacheinfo.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed..0b49e29 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,7 +433,7 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;


Regards,
Srivatsa S. Bhat


^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
  2012-04-19  8:30   ` Srivatsa S. Bhat
@ 2012-04-19 11:02     ` Borislav Petkov
  -1 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-19 11:02 UTC (permalink / raw)
  To: Srivatsa S. Bhat
  Cc: Dan Carpenter, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Frank Arnold, linux-kernel, kernel-janitors

On Thu, Apr 19, 2012 at 01:48:02PM +0530, Srivatsa S. Bhat wrote:
> On 04/19/2012 12:30 PM, Dan Carpenter wrote:
> 
> > amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
> > or zero.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> > index 73d08ed..7b4e294 100644
> > --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> > +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> > @@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
> >  		return -EINVAL;
> > 
> >  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
> > -	if (err) {
> > -		if (err = -EEXIST)
> > -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> > -					    slot);
> > +	if (err)
> >  		return err;
> > -	}
> > +
> >  	return count;
> >  }
> > 
> 
> 
> Looking at the comments around the code and the print statement your patch
> is trying to remove, I wonder if it would be more appropriate to return
> -EEXIST in amd_set_l3_disable_slot(), like this:
> 
> ---
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH] cpu: Fix error return code in amd_set_l3_disable_slot()
> 
> If the L3 disable slot is already in use, return -EEXIST instead of -EINVAL.
> The caller, store_cache_disable(), checks this return value to print an
> appropriate warning.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  arch/x86/kernel/cpu/intel_cacheinfo.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed..0b49e29 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -433,7 +433,7 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>  	/*  check if @slot is already used or the index is already disabled */
>  	ret = amd_get_l3_disable_slot(nb, slot);
>  	if (ret >= 0)
> -		return -EINVAL;
> +		return -EEXIST;
>  
>  	if (index > nb->l3_cache.indices)
>  		return -EINVAL;

Well, let's see, there's 8cc1176e5de534d55cb26ff0cef3fd0d6ad8c3c0 which
was intending at looking at -EEXIST when it gets returned but forgot to
return it at the end. Crap. Somebody should b*tchslap its author... oh,
that's me.

Good catch guys, thanks. I'll take a bit enhanced version of Srivatsa's
patch because it goes in the way of what was originally intended. The
enhanced version returns -EEXIST for the other slot too when we're
disabling the same index, see below:

--
From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
Date: Thu, 19 Apr 2012 12:35:08 +0200
Subject: [PATCH] x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()

If the L3 disable slot is already in use, return -EEXIST instead of
-EINVAL. The caller, store_cache_disable(), checks this return value to
print an appropriate warning.

Also, we want to signal with -EEXIST that the current index we're
disabling has actually been already disabled on the node:

$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
-bash: echo: write error: File exists
$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
-bash: echo: write error: File exists
$ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
-bash: echo: write error: File exists

The old code would say

-bash: echo: write error: Invalid argument

for disable slot 1 when playing the example above with no output in
dmesg, which is clearly misleading.

Cc: <stable@vger.kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
[Boris: add testing for the other index too]
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed98a64..b8f3653dddbc 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;
 
 	/* check whether the other slot has disabled the same index already */
 	if (index = amd_get_l3_disable_slot(nb, !slot))
-		return -EINVAL;
+		return -EEXIST;
 
 	amd_l3_disable_index(nb, cpu, slot, index);
 
@@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
 	if (err) {
 		if (err = -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+			pr_warning("L3 slot %d in use/index already disabled!\n",
+				   slot);
 		return err;
 	}
 	return count;
-- 
1.7.9.3.362.g71319


-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
@ 2012-04-19 11:02     ` Borislav Petkov
  0 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-19 11:02 UTC (permalink / raw)
  To: Srivatsa S. Bhat
  Cc: Dan Carpenter, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Frank Arnold, linux-kernel, kernel-janitors

On Thu, Apr 19, 2012 at 01:48:02PM +0530, Srivatsa S. Bhat wrote:
> On 04/19/2012 12:30 PM, Dan Carpenter wrote:
> 
> > amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
> > or zero.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> > index 73d08ed..7b4e294 100644
> > --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> > +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> > @@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
> >  		return -EINVAL;
> > 
> >  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
> > -	if (err) {
> > -		if (err == -EEXIST)
> > -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> > -					    slot);
> > +	if (err)
> >  		return err;
> > -	}
> > +
> >  	return count;
> >  }
> > 
> 
> 
> Looking at the comments around the code and the print statement your patch
> is trying to remove, I wonder if it would be more appropriate to return
> -EEXIST in amd_set_l3_disable_slot(), like this:
> 
> ---
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH] cpu: Fix error return code in amd_set_l3_disable_slot()
> 
> If the L3 disable slot is already in use, return -EEXIST instead of -EINVAL.
> The caller, store_cache_disable(), checks this return value to print an
> appropriate warning.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  arch/x86/kernel/cpu/intel_cacheinfo.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed..0b49e29 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -433,7 +433,7 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>  	/*  check if @slot is already used or the index is already disabled */
>  	ret = amd_get_l3_disable_slot(nb, slot);
>  	if (ret >= 0)
> -		return -EINVAL;
> +		return -EEXIST;
>  
>  	if (index > nb->l3_cache.indices)
>  		return -EINVAL;

Well, let's see, there's 8cc1176e5de534d55cb26ff0cef3fd0d6ad8c3c0 which
was intending at looking at -EEXIST when it gets returned but forgot to
return it at the end. Crap. Somebody should b*tchslap its author... oh,
that's me.

Good catch guys, thanks. I'll take a bit enhanced version of Srivatsa's
patch because it goes in the way of what was originally intended. The
enhanced version returns -EEXIST for the other slot too when we're
disabling the same index, see below:

--
From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
Date: Thu, 19 Apr 2012 12:35:08 +0200
Subject: [PATCH] x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()

If the L3 disable slot is already in use, return -EEXIST instead of
-EINVAL. The caller, store_cache_disable(), checks this return value to
print an appropriate warning.

Also, we want to signal with -EEXIST that the current index we're
disabling has actually been already disabled on the node:

$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
-bash: echo: write error: File exists
$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
-bash: echo: write error: File exists
$ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
-bash: echo: write error: File exists

The old code would say

-bash: echo: write error: Invalid argument

for disable slot 1 when playing the example above with no output in
dmesg, which is clearly misleading.

Cc: <stable@vger.kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
[Boris: add testing for the other index too]
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed98a64..b8f3653dddbc 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;
 
 	/* check whether the other slot has disabled the same index already */
 	if (index == amd_get_l3_disable_slot(nb, !slot))
-		return -EINVAL;
+		return -EEXIST;
 
 	amd_l3_disable_index(nb, cpu, slot, index);
 
@@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
 	if (err) {
 		if (err == -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+			pr_warning("L3 slot %d in use/index already disabled!\n",
+				   slot);
 		return err;
 	}
 	return count;
-- 
1.7.9.3.362.g71319


-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
  2012-04-19 11:02     ` Borislav Petkov
@ 2012-04-19 11:26       ` Srivatsa S. Bhat
  -1 siblings, 0 replies; 19+ messages in thread
From: Srivatsa S. Bhat @ 2012-04-19 11:14 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Dan Carpenter, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Frank Arnold, linux-kernel, kernel-janitors

On 04/19/2012 04:32 PM, Borislav Petkov wrote:

> On Thu, Apr 19, 2012 at 01:48:02PM +0530, Srivatsa S. Bhat wrote:
>> On 04/19/2012 12:30 PM, Dan Carpenter wrote:
>>
>>> amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
>>> or zero.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
>>> index 73d08ed..7b4e294 100644
>>> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
>>> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
>>> @@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>>>  		return -EINVAL;
>>>
>>>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
>>> -	if (err) {
>>> -		if (err == -EEXIST)
>>> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
>>> -					    slot);
>>> +	if (err)
>>>  		return err;
>>> -	}
>>> +
>>>  	return count;
>>>  }
>>>
>>
>>
>> Looking at the comments around the code and the print statement your patch
>> is trying to remove, I wonder if it would be more appropriate to return
>> -EEXIST in amd_set_l3_disable_slot(), like this:
>>
>> ---
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: [PATCH] cpu: Fix error return code in amd_set_l3_disable_slot()
>>
>> If the L3 disable slot is already in use, return -EEXIST instead of -EINVAL.
>> The caller, store_cache_disable(), checks this return value to print an
>> appropriate warning.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  arch/x86/kernel/cpu/intel_cacheinfo.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
>> index 73d08ed..0b49e29 100644
>> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
>> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
>> @@ -433,7 +433,7 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>>  	/*  check if @slot is already used or the index is already disabled */
>>  	ret = amd_get_l3_disable_slot(nb, slot);
>>  	if (ret >= 0)
>> -		return -EINVAL;
>> +		return -EEXIST;
>>  
>>  	if (index > nb->l3_cache.indices)
>>  		return -EINVAL;
> 
> Well, let's see, there's 8cc1176e5de534d55cb26ff0cef3fd0d6ad8c3c0 which
> was intending at looking at -EEXIST when it gets returned but forgot to
> return it at the end. Crap. Somebody should b*tchslap its author... oh,
> that's me.
> 
> Good catch guys, thanks. I'll take a bit enhanced version of Srivatsa's
> patch because it goes in the way of what was originally intended. The
> enhanced version returns -EEXIST for the other slot too when we're
> disabling the same index, see below:
> 

> --

> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> Date: Thu, 19 Apr 2012 12:35:08 +0200
> Subject: [PATCH] x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
> 
> If the L3 disable slot is already in use, return -EEXIST instead of
> -EINVAL. The caller, store_cache_disable(), checks this return value to
> print an appropriate warning.
> 
> Also, we want to signal with -EEXIST that the current index we're
> disabling has actually been already disabled on the node:
> 
> $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
> $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
> -bash: echo: write error: File exists
> $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
> -bash: echo: write error: File exists
> $ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
> -bash: echo: write error: File exists
> 
> The old code would say
> 
> -bash: echo: write error: Invalid argument
> 
> for disable slot 1 when playing the example above with no output in
> dmesg, which is clearly misleading.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
> [Boris: add testing for the other index too]
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed98a64..b8f3653dddbc 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>  	/*  check if @slot is already used or the index is already disabled */
>  	ret = amd_get_l3_disable_slot(nb, slot);
>  	if (ret >= 0)
> -		return -EINVAL;
> +		return -EEXIST;
> 
>  	if (index > nb->l3_cache.indices)
>  		return -EINVAL;
> 
>  	/* check whether the other slot has disabled the same index already */
>  	if (index == amd_get_l3_disable_slot(nb, !slot))
> -		return -EINVAL;
> +		return -EEXIST;
> 
>  	amd_l3_disable_index(nb, cpu, slot, index);
> 
> @@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
>  	if (err) {
>  		if (err == -EEXIST)
> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> -					    slot);
> +			pr_warning("L3 slot %d in use/index already disabled!\n",
> +				   slot);
>  		return err;
>  	}
>  	return count;


Wow, that's even better! Thanks :-)

Regards,
Srivatsa S. Bhat


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
@ 2012-04-19 11:26       ` Srivatsa S. Bhat
  0 siblings, 0 replies; 19+ messages in thread
From: Srivatsa S. Bhat @ 2012-04-19 11:26 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Dan Carpenter, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	Frank Arnold, linux-kernel, kernel-janitors

On 04/19/2012 04:32 PM, Borislav Petkov wrote:

> On Thu, Apr 19, 2012 at 01:48:02PM +0530, Srivatsa S. Bhat wrote:
>> On 04/19/2012 12:30 PM, Dan Carpenter wrote:
>>
>>> amd_set_l3_disable_slot() never returns -EEXIST, it only returns -EINVAL
>>> or zero.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
>>> index 73d08ed..7b4e294 100644
>>> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
>>> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
>>> @@ -466,12 +466,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>>>  		return -EINVAL;
>>>
>>>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
>>> -	if (err) {
>>> -		if (err = -EEXIST)
>>> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
>>> -					    slot);
>>> +	if (err)
>>>  		return err;
>>> -	}
>>> +
>>>  	return count;
>>>  }
>>>
>>
>>
>> Looking at the comments around the code and the print statement your patch
>> is trying to remove, I wonder if it would be more appropriate to return
>> -EEXIST in amd_set_l3_disable_slot(), like this:
>>
>> ---
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: [PATCH] cpu: Fix error return code in amd_set_l3_disable_slot()
>>
>> If the L3 disable slot is already in use, return -EEXIST instead of -EINVAL.
>> The caller, store_cache_disable(), checks this return value to print an
>> appropriate warning.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>>
>>  arch/x86/kernel/cpu/intel_cacheinfo.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
>> index 73d08ed..0b49e29 100644
>> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
>> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
>> @@ -433,7 +433,7 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>>  	/*  check if @slot is already used or the index is already disabled */
>>  	ret = amd_get_l3_disable_slot(nb, slot);
>>  	if (ret >= 0)
>> -		return -EINVAL;
>> +		return -EEXIST;
>>  
>>  	if (index > nb->l3_cache.indices)
>>  		return -EINVAL;
> 
> Well, let's see, there's 8cc1176e5de534d55cb26ff0cef3fd0d6ad8c3c0 which
> was intending at looking at -EEXIST when it gets returned but forgot to
> return it at the end. Crap. Somebody should b*tchslap its author... oh,
> that's me.
> 
> Good catch guys, thanks. I'll take a bit enhanced version of Srivatsa's
> patch because it goes in the way of what was originally intended. The
> enhanced version returns -EEXIST for the other slot too when we're
> disabling the same index, see below:
> 

> --

> From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
> Date: Thu, 19 Apr 2012 12:35:08 +0200
> Subject: [PATCH] x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
> 
> If the L3 disable slot is already in use, return -EEXIST instead of
> -EINVAL. The caller, store_cache_disable(), checks this return value to
> print an appropriate warning.
> 
> Also, we want to signal with -EEXIST that the current index we're
> disabling has actually been already disabled on the node:
> 
> $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
> $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
> -bash: echo: write error: File exists
> $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
> -bash: echo: write error: File exists
> $ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
> -bash: echo: write error: File exists
> 
> The old code would say
> 
> -bash: echo: write error: Invalid argument
> 
> for disable slot 1 when playing the example above with no output in
> dmesg, which is clearly misleading.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
> [Boris: add testing for the other index too]
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> ---
>  arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed98a64..b8f3653dddbc 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>  	/*  check if @slot is already used or the index is already disabled */
>  	ret = amd_get_l3_disable_slot(nb, slot);
>  	if (ret >= 0)
> -		return -EINVAL;
> +		return -EEXIST;
> 
>  	if (index > nb->l3_cache.indices)
>  		return -EINVAL;
> 
>  	/* check whether the other slot has disabled the same index already */
>  	if (index = amd_get_l3_disable_slot(nb, !slot))
> -		return -EINVAL;
> +		return -EEXIST;
> 
>  	amd_l3_disable_index(nb, cpu, slot, index);
> 
> @@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
>  	if (err) {
>  		if (err = -EEXIST)
> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> -					    slot);
> +			pr_warning("L3 slot %d in use/index already disabled!\n",
> +				   slot);
>  		return err;
>  	}
>  	return count;


Wow, that's even better! Thanks :-)

Regards,
Srivatsa S. Bhat


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
  2012-04-19 11:02     ` Borislav Petkov
@ 2012-04-19 11:44       ` Dan Carpenter
  -1 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2012-04-19 11:44 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Srivatsa S. Bhat, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Frank Arnold, linux-kernel, kernel-janitors

Great.  Thanks.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [patch] cpu: remove some dead code in store_cache_disable()
@ 2012-04-19 11:44       ` Dan Carpenter
  0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2012-04-19 11:44 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Srivatsa S. Bhat, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Frank Arnold, linux-kernel, kernel-janitors

Great.  Thanks.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [GIT PULL] L3 CID fix for 3.5
  2012-04-19 11:44       ` Dan Carpenter
@ 2012-04-19 16:53         ` Borislav Petkov
  -1 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-19 16:53 UTC (permalink / raw)
  To: Ingo Molnar, H. Peter Anvin
  Cc: Dan Carpenter, Borislav Petkov, Srivatsa S. Bhat, Thomas Gleixner,
	x86, Frank Arnold, linux-kernel, kernel-janitors

Hi guys,

please pull the following fix for L3 cache index disable into tip. Since
there's no real breakage but simply a correctness issue, it can wait for
3.5. I've also removed the stable tag I had in the previous version.

Thanks.

--
The following changes since commit e816b57a337ea3b755de72bec38c10c864f23015:

  Linux 3.4-rc3 (2012-04-15 18:28:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tags/l3-fix-for-3.5

for you to fetch changes up to a720b2dd2470a52345df11dca8d6c1466599f812:

  x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot() (2012-04-19 18:30:28 +0200)

----------------------------------------------------------------
A small L3 cache index disable fix from Srivatsa Bhat which unifies the
way the code checks for already disabled indices.

----------------------------------------------------------------
Srivatsa S. Bhat (1):
      x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()

 arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Appending patch here too:

--
commit a720b2dd2470a52345df11dca8d6c1466599f812
Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Date:   Thu Apr 19 12:35:08 2012 +0200

    x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
    
    If the L3 disable slot is already in use, return -EEXIST instead of
    -EINVAL. The caller, store_cache_disable(), checks this return value to
    print an appropriate warning.
    
    Also, we want to signal with -EEXIST that the current index we're
    disabling has actually been already disabled on the node:
    
    $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
    $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
    -bash: echo: write error: File exists
    $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
    -bash: echo: write error: File exists
    $ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
    -bash: echo: write error: File exists
    
    The old code would say
    
    -bash: echo: write error: Invalid argument
    
    for disable slot 1 when playing the example above with no output in
    dmesg, which is clearly misleading.
    
    Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
    Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
    Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
    [Boris: add testing for the other index too]
    Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed98a64..b8f3653dddbc 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;
 
 	/* check whether the other slot has disabled the same index already */
 	if (index = amd_get_l3_disable_slot(nb, !slot))
-		return -EINVAL;
+		return -EEXIST;
 
 	amd_l3_disable_index(nb, cpu, slot, index);
 
@@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
 	if (err) {
 		if (err = -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+			pr_warning("L3 slot %d in use/index already disabled!\n",
+				   slot);
 		return err;
 	}
 	return count;

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* [GIT PULL] L3 CID fix for 3.5
@ 2012-04-19 16:53         ` Borislav Petkov
  0 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-19 16:53 UTC (permalink / raw)
  To: Ingo Molnar, H. Peter Anvin
  Cc: Dan Carpenter, Borislav Petkov, Srivatsa S. Bhat, Thomas Gleixner,
	x86, Frank Arnold, linux-kernel, kernel-janitors

Hi guys,

please pull the following fix for L3 cache index disable into tip. Since
there's no real breakage but simply a correctness issue, it can wait for
3.5. I've also removed the stable tag I had in the previous version.

Thanks.

--
The following changes since commit e816b57a337ea3b755de72bec38c10c864f23015:

  Linux 3.4-rc3 (2012-04-15 18:28:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tags/l3-fix-for-3.5

for you to fetch changes up to a720b2dd2470a52345df11dca8d6c1466599f812:

  x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot() (2012-04-19 18:30:28 +0200)

----------------------------------------------------------------
A small L3 cache index disable fix from Srivatsa Bhat which unifies the
way the code checks for already disabled indices.

----------------------------------------------------------------
Srivatsa S. Bhat (1):
      x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()

 arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Appending patch here too:

--
commit a720b2dd2470a52345df11dca8d6c1466599f812
Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Date:   Thu Apr 19 12:35:08 2012 +0200

    x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
    
    If the L3 disable slot is already in use, return -EEXIST instead of
    -EINVAL. The caller, store_cache_disable(), checks this return value to
    print an appropriate warning.
    
    Also, we want to signal with -EEXIST that the current index we're
    disabling has actually been already disabled on the node:
    
    $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
    $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
    -bash: echo: write error: File exists
    $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
    -bash: echo: write error: File exists
    $ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
    -bash: echo: write error: File exists
    
    The old code would say
    
    -bash: echo: write error: Invalid argument
    
    for disable slot 1 when playing the example above with no output in
    dmesg, which is clearly misleading.
    
    Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
    Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
    Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
    [Boris: add testing for the other index too]
    Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed98a64..b8f3653dddbc 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;
 
 	/* check whether the other slot has disabled the same index already */
 	if (index == amd_get_l3_disable_slot(nb, !slot))
-		return -EINVAL;
+		return -EEXIST;
 
 	amd_l3_disable_index(nb, cpu, slot, index);
 
@@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
 	if (err) {
 		if (err == -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+			pr_warning("L3 slot %d in use/index already disabled!\n",
+				   slot);
 		return err;
 	}
 	return count;

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

^ permalink raw reply related	[flat|nested] 19+ messages in thread
* Re: [GIT PULL] L3 CID fix for 3.5
  2012-04-19 16:53         ` Borislav Petkov
@ 2012-04-25  9:30           ` Borislav Petkov
  -1 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-25  9:30 UTC (permalink / raw)
  To: Ingo Molnar, H. Peter Anvin
  Cc: x86, kernel-janitors, linux-kernel, Srivatsa S. Bhat,
	Frank Arnold, Thomas Gleixner, Dan Carpenter

Ping?

Guys, any issues with this that I should know of?

Thanks.

On Thu, Apr 19, 2012 at 06:53:53PM +0200, Borislav Petkov wrote:
> Hi guys,
> 
> please pull the following fix for L3 cache index disable into tip. Since
> there's no real breakage but simply a correctness issue, it can wait for
> 3.5. I've also removed the stable tag I had in the previous version.
> 
> Thanks.
> 
> --
> The following changes since commit e816b57a337ea3b755de72bec38c10c864f23015:
> 
>   Linux 3.4-rc3 (2012-04-15 18:28:29 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tags/l3-fix-for-3.5
> 
> for you to fetch changes up to a720b2dd2470a52345df11dca8d6c1466599f812:
> 
>   x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot() (2012-04-19 18:30:28 +0200)
> 
> ----------------------------------------------------------------
> A small L3 cache index disable fix from Srivatsa Bhat which unifies the
> way the code checks for already disabled indices.
> 
> ----------------------------------------------------------------
> Srivatsa S. Bhat (1):
>       x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
> 
>  arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Appending patch here too:
> 
> --
> commit a720b2dd2470a52345df11dca8d6c1466599f812
> Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Date:   Thu Apr 19 12:35:08 2012 +0200
> 
>     x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
>     
>     If the L3 disable slot is already in use, return -EEXIST instead of
>     -EINVAL. The caller, store_cache_disable(), checks this return value to
>     print an appropriate warning.
>     
>     Also, we want to signal with -EEXIST that the current index we're
>     disabling has actually been already disabled on the node:
>     
>     $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
>     $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
>     -bash: echo: write error: File exists
>     $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
>     -bash: echo: write error: File exists
>     $ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
>     -bash: echo: write error: File exists
>     
>     The old code would say
>     
>     -bash: echo: write error: Invalid argument
>     
>     for disable slot 1 when playing the example above with no output in
>     dmesg, which is clearly misleading.
>     
>     Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>     Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>     Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
>     [Boris: add testing for the other index too]
>     Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed98a64..b8f3653dddbc 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>  	/*  check if @slot is already used or the index is already disabled */
>  	ret = amd_get_l3_disable_slot(nb, slot);
>  	if (ret >= 0)
> -		return -EINVAL;
> +		return -EEXIST;
>  
>  	if (index > nb->l3_cache.indices)
>  		return -EINVAL;
>  
>  	/* check whether the other slot has disabled the same index already */
>  	if (index = amd_get_l3_disable_slot(nb, !slot))
> -		return -EINVAL;
> +		return -EEXIST;
>  
>  	amd_l3_disable_index(nb, cpu, slot, index);
>  
> @@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
>  	if (err) {
>  		if (err = -EEXIST)
> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> -					    slot);
> +			pr_warning("L3 slot %d in use/index already disabled!\n",
> +				   slot);
>  		return err;
>  	}
>  	return count;
> 
> -- 
> Regards/Gruss,
> Boris.
> 
> Advanced Micro Devices GmbH
> Einsteinring 24, 85609 Dornach
> GM: Alberto Bozzo
> Reg: Dornach, Landkreis Muenchen
> HRB Nr. 43632 WEEE Registernr: 129 19551
> _______________________________________________
> osrc-patches mailing list
> osrc-patches@elbe.amd.com
> https://elbe.amd.com/mailman/listinfo/osrc-patches

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [GIT PULL] L3 CID fix for 3.5
@ 2012-04-25  9:30           ` Borislav Petkov
  0 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-25  9:30 UTC (permalink / raw)
  To: Ingo Molnar, H. Peter Anvin
  Cc: x86, kernel-janitors, linux-kernel, Srivatsa S. Bhat,
	Frank Arnold, Thomas Gleixner, Dan Carpenter

Ping?

Guys, any issues with this that I should know of?

Thanks.

On Thu, Apr 19, 2012 at 06:53:53PM +0200, Borislav Petkov wrote:
> Hi guys,
> 
> please pull the following fix for L3 cache index disable into tip. Since
> there's no real breakage but simply a correctness issue, it can wait for
> 3.5. I've also removed the stable tag I had in the previous version.
> 
> Thanks.
> 
> --
> The following changes since commit e816b57a337ea3b755de72bec38c10c864f23015:
> 
>   Linux 3.4-rc3 (2012-04-15 18:28:29 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tags/l3-fix-for-3.5
> 
> for you to fetch changes up to a720b2dd2470a52345df11dca8d6c1466599f812:
> 
>   x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot() (2012-04-19 18:30:28 +0200)
> 
> ----------------------------------------------------------------
> A small L3 cache index disable fix from Srivatsa Bhat which unifies the
> way the code checks for already disabled indices.
> 
> ----------------------------------------------------------------
> Srivatsa S. Bhat (1):
>       x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
> 
>  arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Appending patch here too:
> 
> --
> commit a720b2dd2470a52345df11dca8d6c1466599f812
> Author: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Date:   Thu Apr 19 12:35:08 2012 +0200
> 
>     x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
>     
>     If the L3 disable slot is already in use, return -EEXIST instead of
>     -EINVAL. The caller, store_cache_disable(), checks this return value to
>     print an appropriate warning.
>     
>     Also, we want to signal with -EEXIST that the current index we're
>     disabling has actually been already disabled on the node:
>     
>     $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
>     $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
>     -bash: echo: write error: File exists
>     $ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
>     -bash: echo: write error: File exists
>     $ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
>     -bash: echo: write error: File exists
>     
>     The old code would say
>     
>     -bash: echo: write error: Invalid argument
>     
>     for disable slot 1 when playing the example above with no output in
>     dmesg, which is clearly misleading.
>     
>     Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>     Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>     Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
>     [Boris: add testing for the other index too]
>     Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> 
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 73d08ed98a64..b8f3653dddbc 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
>  	/*  check if @slot is already used or the index is already disabled */
>  	ret = amd_get_l3_disable_slot(nb, slot);
>  	if (ret >= 0)
> -		return -EINVAL;
> +		return -EEXIST;
>  
>  	if (index > nb->l3_cache.indices)
>  		return -EINVAL;
>  
>  	/* check whether the other slot has disabled the same index already */
>  	if (index == amd_get_l3_disable_slot(nb, !slot))
> -		return -EINVAL;
> +		return -EEXIST;
>  
>  	amd_l3_disable_index(nb, cpu, slot, index);
>  
> @@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
>  	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
>  	if (err) {
>  		if (err == -EEXIST)
> -			printk(KERN_WARNING "L3 disable slot %d in use!\n",
> -					    slot);
> +			pr_warning("L3 slot %d in use/index already disabled!\n",
> +				   slot);
>  		return err;
>  	}
>  	return count;
> 
> -- 
> Regards/Gruss,
> Boris.
> 
> Advanced Micro Devices GmbH
> Einsteinring 24, 85609 Dornach
> GM: Alberto Bozzo
> Reg: Dornach, Landkreis Muenchen
> HRB Nr. 43632 WEEE Registernr: 129 19551
> _______________________________________________
> osrc-patches mailing list
> osrc-patches@elbe.amd.com
> https://elbe.amd.com/mailman/listinfo/osrc-patches

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [GIT PULL] L3 CID fix for 3.5
  2012-04-25  9:30           ` Borislav Petkov
@ 2012-04-25 10:23             ` Ingo Molnar
  -1 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2012-04-25 10:23 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ingo Molnar, H. Peter Anvin, x86, kernel-janitors, linux-kernel,
	Srivatsa S. Bhat, Frank Arnold, Thomas Gleixner, Dan Carpenter


* Borislav Petkov <borislav.petkov@amd.com> wrote:

> Ping?
> 
> Guys, any issues with this that I should know of?

Looks good to me. You marked it v3.5, should I pull it into 
v3.4? Looks small enough to me and we better keep such user 
facing interfaces correct, even if there's no direct regression.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [GIT PULL] L3 CID fix for 3.5
@ 2012-04-25 10:23             ` Ingo Molnar
  0 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2012-04-25 10:23 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ingo Molnar, H. Peter Anvin, x86, kernel-janitors, linux-kernel,
	Srivatsa S. Bhat, Frank Arnold, Thomas Gleixner, Dan Carpenter


* Borislav Petkov <borislav.petkov@amd.com> wrote:

> Ping?
> 
> Guys, any issues with this that I should know of?

Looks good to me. You marked it v3.5, should I pull it into 
v3.4? Looks small enough to me and we better keep such user 
facing interfaces correct, even if there's no direct regression.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [GIT PULL] L3 CID fix for 3.5
  2012-04-25 10:23             ` Ingo Molnar
@ 2012-04-25 10:35               ` Borislav Petkov
  -1 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-25 10:35 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, H. Peter Anvin, x86, kernel-janitors, linux-kernel,
	Srivatsa S. Bhat, Frank Arnold, Thomas Gleixner, Dan Carpenter

On Wed, Apr 25, 2012 at 12:23:58PM +0200, Ingo Molnar wrote:
> Looks good to me. You marked it v3.5, should I pull it into v3.4?
> Looks small enough to me and we better keep such user facing
> interfaces correct, even if there's no direct regression.

I'm fine with whatever you decide,

thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551


^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [GIT PULL] L3 CID fix for 3.5
@ 2012-04-25 10:35               ` Borislav Petkov
  0 siblings, 0 replies; 19+ messages in thread
From: Borislav Petkov @ 2012-04-25 10:35 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ingo Molnar, H. Peter Anvin, x86, kernel-janitors, linux-kernel,
	Srivatsa S. Bhat, Frank Arnold, Thomas Gleixner, Dan Carpenter

On Wed, Apr 25, 2012 at 12:23:58PM +0200, Ingo Molnar wrote:
> Looks good to me. You marked it v3.5, should I pull it into v3.4?
> Looks small enough to me and we better keep such user facing
> interfaces correct, even if there's no direct regression.

I'm fine with whatever you decide,

thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551


^ permalink raw reply	[flat|nested] 19+ messages in thread
* [tip:x86/urgent] x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()
  2012-04-19  7:00 ` Dan Carpenter
  (?)
  (?)
@ 2012-04-25 13:50 ` tip-bot for Srivatsa S. Bhat
  -1 siblings, 0 replies; 19+ messages in thread
From: tip-bot for Srivatsa S. Bhat @ 2012-04-25 13:50 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, srivatsa.bhat, tglx, borislav.petkov,
	dan.carpenter

Commit-ID:  a720b2dd2470a52345df11dca8d6c1466599f812
Gitweb:     http://git.kernel.org/tip/a720b2dd2470a52345df11dca8d6c1466599f812
Author:     Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
AuthorDate: Thu, 19 Apr 2012 12:35:08 +0200
Committer:  Borislav Petkov <borislav.petkov@amd.com>
CommitDate: Thu, 19 Apr 2012 18:30:28 +0200

x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot()

If the L3 disable slot is already in use, return -EEXIST instead of
-EINVAL. The caller, store_cache_disable(), checks this return value to
print an appropriate warning.

Also, we want to signal with -EEXIST that the current index we're
disabling has actually been already disabled on the node:

$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_0
-bash: echo: write error: File exists
$ echo 12 > /sys/devices/system/cpu/cpu3/cache/index3/cache_disable_1
-bash: echo: write error: File exists
$ echo 12 > /sys/devices/system/cpu/cpu5/cache/index3/cache_disable_1
-bash: echo: write error: File exists

The old code would say

-bash: echo: write error: Invalid argument

for disable slot 1 when playing the example above with no output in
dmesg, which is clearly misleading.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Link: http://lkml.kernel.org/r/20120419070053.GB16645@elgon.mountain
[Boris: add testing for the other index too]
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 73d08ed..b8f3653 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -433,14 +433,14 @@ int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu, unsigned slot,
 	/*  check if @slot is already used or the index is already disabled */
 	ret = amd_get_l3_disable_slot(nb, slot);
 	if (ret >= 0)
-		return -EINVAL;
+		return -EEXIST;
 
 	if (index > nb->l3_cache.indices)
 		return -EINVAL;
 
 	/* check whether the other slot has disabled the same index already */
 	if (index == amd_get_l3_disable_slot(nb, !slot))
-		return -EINVAL;
+		return -EEXIST;
 
 	amd_l3_disable_index(nb, cpu, slot, index);
 
@@ -468,8 +468,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	err = amd_set_l3_disable_slot(this_leaf->base.nb, cpu, slot, val);
 	if (err) {
 		if (err == -EEXIST)
-			printk(KERN_WARNING "L3 disable slot %d in use!\n",
-					    slot);
+			pr_warning("L3 slot %d in use/index already disabled!\n",
+				   slot);
 		return err;
 	}
 	return count;

^ permalink raw reply related	[flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-04-25 13:51 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-19  7:00 [patch] cpu: remove some dead code in store_cache_disable() Dan Carpenter
2012-04-19  7:00 ` Dan Carpenter
2012-04-19  8:18 ` Srivatsa S. Bhat
2012-04-19  8:30   ` Srivatsa S. Bhat
2012-04-19 11:02   ` Borislav Petkov
2012-04-19 11:02     ` Borislav Petkov
2012-04-19 11:14     ` Srivatsa S. Bhat
2012-04-19 11:26       ` Srivatsa S. Bhat
2012-04-19 11:44     ` Dan Carpenter
2012-04-19 11:44       ` Dan Carpenter
2012-04-19 16:53       ` [GIT PULL] L3 CID fix for 3.5 Borislav Petkov
2012-04-19 16:53         ` Borislav Petkov
2012-04-25  9:30         ` Borislav Petkov
2012-04-25  9:30           ` Borislav Petkov
2012-04-25 10:23           ` Ingo Molnar
2012-04-25 10:23             ` Ingo Molnar
2012-04-25 10:35             ` Borislav Petkov
2012-04-25 10:35               ` Borislav Petkov
2012-04-25 13:50 ` [tip:x86/urgent] x86, intel_cacheinfo: Fix error return code in amd_set_l3_disable_slot() tip-bot for Srivatsa S. Bhat

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.