[PATCH] atomic functions broken

[PATCH] atomic functions broken

[Thomas Koeller]

The ll/sc versions of atomic_sub_if_positive(), and the corresponding 64-bit functions,
are broken. The value returned by those functions is always one, no matter what.


Signed-off-by: Thomas Koeller  <thomas.koeller@baslerweb.com>




diff --git a/include/asm-mips/atomic.h b/include/asm-mips/atomic.h
index 654b97d..472e855 100644
--- a/include/asm-mips/atomic.h
+++ b/include/asm-mips/atomic.h
@@ -247,15 +247,16 @@ static __inline__ int atomic_sub_if_posi
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
 		"1:	ll	%1, %2		# atomic_sub_if_positive\n"
-		"	subu	%0, %1, %3				\n"
-		"	bltz	%0, 1f					\n"
-		"	sc	%0, %2					\n"
-		"	beqzl	%0, 1b					\n"
+		"	subu	%1, %1, %3				\n"
+		"	addi	%0, %1, 0				\n"
+		"	bltz	%1, 1f					\n"
+		"	sc	%1, %2					\n"
+		"	beqzl	%1, 1b					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i)
 		: "memory");
 	} else if (cpu_has_llsc) {
 		unsigned long temp;
@@ -263,15 +264,16 @@ static __inline__ int atomic_sub_if_posi
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
 		"1:	ll	%1, %2		# atomic_sub_if_positive\n"
-		"	subu	%0, %1, %3				\n"
-		"	bltz	%0, 1f					\n"
-		"	sc	%0, %2					\n"
-		"	beqz	%0, 1b					\n"
+		"	subu	%1, %1, %3				\n"
+		"	addi	%0, %1, 0				\n"
+		"	bltz	%1, 1f					\n"
+		"	sc	%1, %2					\n"
+		"	beqz	%1, 1b					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i)
 		: "memory");
 	} else {
 		unsigned long flags;
@@ -595,15 +597,16 @@ static __inline__ long atomic64_sub_if_p
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
 		"1:	lld	%1, %2		# atomic64_sub_if_positive\n"
-		"	dsubu	%0, %1, %3				\n"
-		"	bltz	%0, 1f					\n"
-		"	scd	%0, %2					\n"
-		"	beqzl	%0, 1b					\n"
+		"	dsubu	%1, %1, %3				\n"
+		"	daddi	%0, %1, 0				\n"
+		"	bltz	%1, 1f					\n"
+		"	scd	%1, %2					\n"
+		"	beqzl	%1, 1b					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i)
 		: "memory");
 	} else if (cpu_has_llsc) {
 		unsigned long temp;
@@ -611,15 +614,16 @@ static __inline__ long atomic64_sub_if_p
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
 		"1:	lld	%1, %2		# atomic64_sub_if_positive\n"
-		"	dsubu	%0, %1, %3				\n"
-		"	bltz	%0, 1f					\n"
-		"	scd	%0, %2					\n"
-		"	beqz	%0, 1b					\n"
+		"	dsubu	%1, %1, %3				\n"
+		"	daddi	%0, %1, 0				\n"
+		"	bltz	%1, 1f					\n"
+		"	scd	%1, %2					\n"
+		"	beqz	%1, 1b					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i)
 		: "memory");
 	} else {
 		unsigned long flags;

-- 
--------------------------------------------------

Thomas Koeller, Software Development
Basler Vision Technologies

thomas dot koeller at baslerweb dot com
http://www.baslerweb.com

==============================

Re: [PATCH] atomic functions broken

[Ralf Baechle]

On Wed, Feb 22, 2006 at 09:25:39PM +0100, Thomas Koeller wrote:

> The ll/sc versions of atomic_sub_if_positive(), and the corresponding 64-bit functions,
> are broken. The value returned by those functions is always one, no matter what.

Good catch - and I wonder how you found this bug since it didn't seem to
cause any obvious damage.  I just went for a different fix though - I was
able to get away without an extra register and squeeze the needed extra
instruction into a delay slot, so no code size penalty either.

  Ralf

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

--- a/include/asm-mips/atomic.h
+++ b/include/asm-mips/atomic.h
@@ -250,7 +250,10 @@ static __inline__ int atomic_sub_if_posi
 		"	subu	%0, %1, %3				\n"
 		"	bltz	%0, 1f					\n"
 		"	sc	%0, %2					\n"
+		"	.set	noreorder				\n"
 		"	beqzl	%0, 1b					\n"
+		"	 subu	%0, %1, %3				\n"
+		"	.set	reorder					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
@@ -266,7 +269,10 @@ static __inline__ int atomic_sub_if_posi
 		"	subu	%0, %1, %3				\n"
 		"	bltz	%0, 1f					\n"
 		"	sc	%0, %2					\n"
+		"	.set	noreorder				\n"
 		"	beqz	%0, 1b					\n"
+		"	 subu	%0, %1, %3				\n"
+		"	.set	reorder					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
@@ -598,7 +604,10 @@ static __inline__ long atomic64_sub_if_p
 		"	dsubu	%0, %1, %3				\n"
 		"	bltz	%0, 1f					\n"
 		"	scd	%0, %2					\n"
+		"	.set	noreorder				\n"
 		"	beqzl	%0, 1b					\n"
+		"	 dsubu	%0, %1, %3				\n"
+		"	.set	reorder					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"
@@ -614,7 +623,10 @@ static __inline__ long atomic64_sub_if_p
 		"	dsubu	%0, %1, %3				\n"
 		"	bltz	%0, 1f					\n"
 		"	scd	%0, %2					\n"
+		"	.set	noreorder				\n"
 		"	beqz	%0, 1b					\n"
+		"	 dsubu	%0, %1, %3				\n"
+		"	.set	reorder					\n"
 		"	sync						\n"
 		"1:							\n"
 		"	.set	mips0					\n"