[PATCH] MIPS: unsigned result is always greater than 0

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] MIPS: unsigned result is always greater than 0
@ 2009-01-02 15:09 Roel Kluin
  2009-01-06 21:46   ` David VomLehn (dvomlehn)
  0 siblings, 1 reply; 4+ messages in thread
From: Roel Kluin @ 2009-01-02 15:09 UTC (permalink / raw)
  To: ralf; +Cc: linux-mips

unsigned result is always greater than 0

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
I cannot determine whether the same bug occurs as well in assembly.
Also shouldn't similar checks occur in atomic64_sub_return and in
atomic64_add_return for negative values of i?

diff --git a/arch/mips/include/asm/atomic.h b/arch/mips/include/asm/atomic.h
index 1232be3..3cd07a9 100644
--- a/arch/mips/include/asm/atomic.h
+++ b/arch/mips/include/asm/atomic.h
@@ -296,9 +296,10 @@ static __inline__ int atomic_sub_if_positive(int i, atomic_t * v)
 
 		raw_local_irq_save(flags);
 		result = v->counter;
-		result -= i;
-		if (result >= 0)
+		if (i <= result) {
+			result -= i;
 			v->counter = result;
+		}
 		raw_local_irq_restore(flags);
 	}
 
@@ -677,9 +678,10 @@ static __inline__ long atomic64_sub_if_positive(long i, atomic64_t * v)
 
 		raw_local_irq_save(flags);
 		result = v->counter;
-		result -= i;
-		if (result >= 0)
+		if (i >= result) {
+			result -= i;
 			v->counter = result;
+		}
 		raw_local_irq_restore(flags);
 	}
 

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* RE: [PATCH] MIPS: unsigned result is always greater than 0
@ 2009-01-06 21:46   ` David VomLehn (dvomlehn)
  0 siblings, 0 replies; 4+ messages in thread
From: David VomLehn (dvomlehn) @ 2009-01-06 21:46 UTC (permalink / raw)
  To: Roel Kluin, ralf; +Cc: linux-mips

 

> -----Original Message-----
> From: linux-mips-bounce@linux-mips.org 
> [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Roel Kluin
> Sent: Friday, January 02, 2009 7:10 AM
> To: ralf@linux-mips.org
> Cc: linux-mips@linux-mips.org
> Subject: [PATCH] MIPS: unsigned result is always greater than 0
> 
> unsigned result is always greater than 0
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> I cannot determine whether the same bug occurs as well in assembly.
> Also shouldn't similar checks occur in atomic64_sub_return and in
> atomic64_add_return for negative values of i?
> 
> diff --git a/arch/mips/include/asm/atomic.h 
> b/arch/mips/include/asm/atomic.h
> index 1232be3..3cd07a9 100644
> --- a/arch/mips/include/asm/atomic.h
> +++ b/arch/mips/include/asm/atomic.h
> @@ -296,9 +296,10 @@ static __inline__ int 
> atomic_sub_if_positive(int i, atomic_t * v)
>  
>  		raw_local_irq_save(flags);
>  		result = v->counter;
> -		result -= i;
> -		if (result >= 0)
> +		if (i <= result) {
> +			result -= i;
>  			v->counter = result;
> +		}
>  		raw_local_irq_restore(flags);
>  	}
>  
> @@ -677,9 +678,10 @@ static __inline__ long 
> atomic64_sub_if_positive(long i, atomic64_t * v)
>  
>  		raw_local_irq_save(flags);
>  		result = v->counter;
> -		result -= i;
> -		if (result >= 0)
> +		if (i >= result) {
> +			result -= i;
>  			v->counter = result;
> +		}
>  		raw_local_irq_restore(flags);
>  	}

I agree that the code as it exists is wrong, but, as I see it, the
problem is that the type of result should be changed from unsigned long
to int. This fixes the comparison so it works correctly. In addition,
such a change means that result would be the same type as the counter
element of atomic_t, avoiding possible surprises should longs be larger
than ints.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] MIPS: unsigned result is always greater than 0
@ 2009-01-06 21:46   ` David VomLehn (dvomlehn)
  0 siblings, 0 replies; 4+ messages in thread
From: David VomLehn (dvomlehn) @ 2009-01-06 21:46 UTC (permalink / raw)
  To: Roel Kluin, ralf; +Cc: linux-mips

 

> -----Original Message-----
> From: linux-mips-bounce@linux-mips.org 
> [mailto:linux-mips-bounce@linux-mips.org] On Behalf Of Roel Kluin
> Sent: Friday, January 02, 2009 7:10 AM
> To: ralf@linux-mips.org
> Cc: linux-mips@linux-mips.org
> Subject: [PATCH] MIPS: unsigned result is always greater than 0
> 
> unsigned result is always greater than 0
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> I cannot determine whether the same bug occurs as well in assembly.
> Also shouldn't similar checks occur in atomic64_sub_return and in
> atomic64_add_return for negative values of i?
> 
> diff --git a/arch/mips/include/asm/atomic.h 
> b/arch/mips/include/asm/atomic.h
> index 1232be3..3cd07a9 100644
> --- a/arch/mips/include/asm/atomic.h
> +++ b/arch/mips/include/asm/atomic.h
> @@ -296,9 +296,10 @@ static __inline__ int 
> atomic_sub_if_positive(int i, atomic_t * v)
>  
>  		raw_local_irq_save(flags);
>  		result = v->counter;
> -		result -= i;
> -		if (result >= 0)
> +		if (i <= result) {
> +			result -= i;
>  			v->counter = result;
> +		}
>  		raw_local_irq_restore(flags);
>  	}
>  
> @@ -677,9 +678,10 @@ static __inline__ long 
> atomic64_sub_if_positive(long i, atomic64_t * v)
>  
>  		raw_local_irq_save(flags);
>  		result = v->counter;
> -		result -= i;
> -		if (result >= 0)
> +		if (i >= result) {
> +			result -= i;
>  			v->counter = result;
> +		}
>  		raw_local_irq_restore(flags);
>  	}

I agree that the code as it exists is wrong, but, as I see it, the
problem is that the type of result should be changed from unsigned long
to int. This fixes the comparison so it works correctly. In addition,
such a change means that result would be the same type as the counter
element of atomic_t, avoiding possible surprises should longs be larger
than ints.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] MIPS: unsigned result is always greater than 0
  2009-01-06 21:46   ` David VomLehn (dvomlehn)
  (?)
@ 2009-01-12  1:26   ` Ralf Baechle
  -1 siblings, 0 replies; 4+ messages in thread
From: Ralf Baechle @ 2009-01-12  1:26 UTC (permalink / raw)
  To: David VomLehn (dvomlehn); +Cc: Roel Kluin, linux-mips

On Tue, Jan 06, 2009 at 04:46:19PM -0500, David VomLehn (dvomlehn) wrote:

> I agree that the code as it exists is wrong, but, as I see it, the
> problem is that the type of result should be changed from unsigned long
> to int. This fixes the comparison so it works correctly. In addition,
> such a change means that result would be the same type as the counter
> element of atomic_t, avoiding possible surprises should longs be larger
> than ints.

Originally that code was intended only for R2000-class processors.  But
LL/SC don't work correctly on XKPHYS addresses on some R5000-class
processors, so we avoid these instructions on the affected processors
running 64-bit kernels and use the fallback method of disabling interrupts
anyway, so long indeed might be longer than int.  What in the end saves
us is that atomic_sub_if_positive's only caller atomic_dec_if_positive
is only being used by the RM9000 watchdog driver and the RM9000 doesn't
suffer from the mentioned CPU bug.

Still no reason not to fix it.  I changed the intermediate variables used in
all functions that operate on atomic_t to int rsp. long for the atomic64_*
functions for consistency - which as a nice side effect also shaves of
almost 2kB from the kernel due to the elemination of gcc-generated
non-sense sign extension code.

  Ralf

MIPS: atomic_*(): Change type of intermediate variables.

This shaves of 1912 bytes of an IP27 defconfig kernel and avoids
unexpected overflow behaviour in atomic_sub_if_positive.  Apply the same
changes to the atomic64_* functions for consistency.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/arch/mips/include/asm/atomic.h b/arch/mips/include/asm/atomic.h
index c996c3b..1b332e1 100644
--- a/arch/mips/include/asm/atomic.h
+++ b/arch/mips/include/asm/atomic.h
@@ -50,7 +50,7 @@
 static __inline__ void atomic_add(int i, atomic_t * v)
 {
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -62,7 +62,7 @@ static __inline__ void atomic_add(int i, atomic_t * v)
 		: "=&r" (temp), "=m" (v->counter)
 		: "Ir" (i), "m" (v->counter));
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -95,7 +95,7 @@ static __inline__ void atomic_add(int i, atomic_t * v)
 static __inline__ void atomic_sub(int i, atomic_t * v)
 {
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -107,7 +107,7 @@ static __inline__ void atomic_sub(int i, atomic_t * v)
 		: "=&r" (temp), "=m" (v->counter)
 		: "Ir" (i), "m" (v->counter));
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -135,12 +135,12 @@ static __inline__ void atomic_sub(int i, atomic_t * v)
  */
 static __inline__ int atomic_add_return(int i, atomic_t * v)
 {
-	unsigned long result;
+	int result;
 
 	smp_llsc_mb();
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -154,7 +154,7 @@ static __inline__ int atomic_add_return(int i, atomic_t * v)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -187,12 +187,12 @@ static __inline__ int atomic_add_return(int i, atomic_t * v)
 
 static __inline__ int atomic_sub_return(int i, atomic_t * v)
 {
-	unsigned long result;
+	int result;
 
 	smp_llsc_mb();
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -206,7 +206,7 @@ static __inline__ int atomic_sub_return(int i, atomic_t * v)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -247,12 +247,12 @@ static __inline__ int atomic_sub_return(int i, atomic_t * v)
  */
 static __inline__ int atomic_sub_if_positive(int i, atomic_t * v)
 {
-	unsigned long result;
+	int result;
 
 	smp_llsc_mb();
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -270,7 +270,7 @@ static __inline__ int atomic_sub_if_positive(int i, atomic_t * v)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		int temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -429,7 +429,7 @@ static __inline__ int atomic_add_unless(atomic_t *v, int a, int u)
 static __inline__ void atomic64_add(long i, atomic64_t * v)
 {
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -441,7 +441,7 @@ static __inline__ void atomic64_add(long i, atomic64_t * v)
 		: "=&r" (temp), "=m" (v->counter)
 		: "Ir" (i), "m" (v->counter));
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -474,7 +474,7 @@ static __inline__ void atomic64_add(long i, atomic64_t * v)
 static __inline__ void atomic64_sub(long i, atomic64_t * v)
 {
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -486,7 +486,7 @@ static __inline__ void atomic64_sub(long i, atomic64_t * v)
 		: "=&r" (temp), "=m" (v->counter)
 		: "Ir" (i), "m" (v->counter));
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -514,12 +514,12 @@ static __inline__ void atomic64_sub(long i, atomic64_t * v)
  */
 static __inline__ long atomic64_add_return(long i, atomic64_t * v)
 {
-	unsigned long result;
+	long result;
 
 	smp_llsc_mb();
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -533,7 +533,7 @@ static __inline__ long atomic64_add_return(long i, atomic64_t * v)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -566,12 +566,12 @@ static __inline__ long atomic64_add_return(long i, atomic64_t * v)
 
 static __inline__ long atomic64_sub_return(long i, atomic64_t * v)
 {
-	unsigned long result;
+	long result;
 
 	smp_llsc_mb();
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -585,7 +585,7 @@ static __inline__ long atomic64_sub_return(long i, atomic64_t * v)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -626,12 +626,12 @@ static __inline__ long atomic64_sub_return(long i, atomic64_t * v)
  */
 static __inline__ long atomic64_sub_if_positive(long i, atomic64_t * v)
 {
-	unsigned long result;
+	long result;
 
 	smp_llsc_mb();
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -649,7 +649,7 @@ static __inline__ long atomic64_sub_if_positive(long i, atomic64_t * v)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (cpu_has_llsc) {
-		unsigned long temp;
+		long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2009-01-12  1:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-02 15:09 [PATCH] MIPS: unsigned result is always greater than 0 Roel Kluin
2009-01-06 21:46 ` David VomLehn (dvomlehn)
2009-01-06 21:46   ` David VomLehn (dvomlehn)
2009-01-12  1:26   ` Ralf Baechle

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.