smp_mb() in asm-mips/bitops.h

smp_mb() in asm-mips/bitops.h

[Atsushi Nemoto]

I found some funny usages of smp_mb() in asm-mips/bitops.h:

static inline int test_and_set_bit(unsigned long nr,
	volatile unsigned long *addr)
{
	if (cpu_has_llsc && R10000_LLSC_WAR) {
...
		return res != 0;
	} else if (cpu_has_llsc) {
...
		return res != 0;
	} else {
...
		return retval;
	}

	smp_mb();
}

It looks this smp_mb() never have any effects.  This change is from:

> commit 0004a9dfeaa709a7f853487aba19932c9b1a87c8
> Author: Ralf Baechle <ralf@linux-mips.org>
> Date:   Tue Oct 31 03:45:07 2006 +0000
> 
>     [MIPS] Cleanup memory barriers for weakly ordered systems.

at 2.6.18 development cycle.

---
Atsushi Nemoto

Re: smp_mb() in asm-mips/bitops.h

[Ralf Baechle]

On Thu, Jun 07, 2007 at 04:53:01PM +0900, Atsushi Nemoto wrote:

> I found some funny usages of smp_mb() in asm-mips/bitops.h:

Funny indeed ;-)  Below patch should do the trick.

Due to very inagressive memory reordering on the few non-strongly ordered
MIPS SMP systems I am somewhat confident this didn't break anything but
if so, Sibyte and PMC-Sierra RM9000 SMP systems are affected.

  Ralf

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

diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h
index d995413..ffe245b 100644
--- a/include/asm-mips/bitops.h
+++ b/include/asm-mips/bitops.h
@@ -238,10 +238,11 @@ static inline int test_and_set_bit(unsigned long nr,
 	volatile unsigned long *addr)
 {
 	unsigned short bit = nr & SZLONG_MASK;
+	unsigned long res;
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -254,11 +255,9 @@ static inline int test_and_set_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "r" (1UL << bit), "m" (*m)
 		: "memory");
-
-		return res != 0;
 	} else if (cpu_has_llsc) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"	.set	push					\n"
@@ -277,25 +276,22 @@ static inline int test_and_set_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "r" (1UL << bit), "m" (*m)
 		: "memory");
-
-		return res != 0;
 	} else {
 		volatile unsigned long *a = addr;
 		unsigned long mask;
-		int retval;
 		unsigned long flags;
 
 		a += nr >> SZLONG_LOG;
 		mask = 1UL << bit;
 		raw_local_irq_save(flags);
-		retval = (mask & *a) != 0;
+		res = (mask & *a);
 		*a |= mask;
 		raw_local_irq_restore(flags);
-
-		return retval;
 	}
 
 	smp_mb();
+
+	return res != 0;
 }
 
 /*
@@ -310,6 +306,7 @@ static inline int test_and_clear_bit(unsigned long nr,
 	volatile unsigned long *addr)
 {
 	unsigned short bit = nr & SZLONG_MASK;
+	unsigned long res;
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
@@ -327,12 +324,10 @@ static inline int test_and_clear_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "r" (1UL << bit), "m" (*m)
 		: "memory");
-
-		return res != 0;
 #ifdef CONFIG_CPU_MIPSR2
 	} else if (__builtin_constant_p(nr)) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"1:	" __LL	"%0, %1		# test_and_clear_bit	\n"
@@ -346,12 +341,10 @@ static inline int test_and_clear_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "ri" (bit), "m" (*m)
 		: "memory");
-
-		return res;
 #endif
 	} else if (cpu_has_llsc) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"	.set	push					\n"
@@ -371,25 +364,22 @@ static inline int test_and_clear_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "r" (1UL << bit), "m" (*m)
 		: "memory");
-
-		return res != 0;
 	} else {
 		volatile unsigned long *a = addr;
 		unsigned long mask;
-		int retval;
 		unsigned long flags;
 
 		a += nr >> SZLONG_LOG;
 		mask = 1UL << bit;
 		raw_local_irq_save(flags);
-		retval = (mask & *a) != 0;
+		res = (mask & *a);
 		*a &= ~mask;
 		raw_local_irq_restore(flags);
-
-		return retval;
 	}
 
 	smp_mb();
+
+	return res != 0;
 }
 
 /*
@@ -404,10 +394,11 @@ static inline int test_and_change_bit(unsigned long nr,
 	volatile unsigned long *addr)
 {
 	unsigned short bit = nr & SZLONG_MASK;
+	unsigned long res;
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"
@@ -420,11 +411,9 @@ static inline int test_and_change_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "r" (1UL << bit), "m" (*m)
 		: "memory");
-
-		return res != 0;
 	} else if (cpu_has_llsc) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"	.set	push					\n"
@@ -443,24 +432,22 @@ static inline int test_and_change_bit(unsigned long nr,
 		: "=&r" (temp), "=m" (*m), "=&r" (res)
 		: "r" (1UL << bit), "m" (*m)
 		: "memory");
-
-		return res != 0;
 	} else {
 		volatile unsigned long *a = addr;
-		unsigned long mask, retval;
+		unsigned long mask;
 		unsigned long flags;
 
 		a += nr >> SZLONG_LOG;
 		mask = 1UL << bit;
 		raw_local_irq_save(flags);
-		retval = (mask & *a) != 0;
+		res = (mask & *a);
 		*a ^= mask;
 		raw_local_irq_restore(flags);
-
-		return retval;
 	}
 
 	smp_mb();
+
+	return res != 0;
 }
 
 #include <asm-generic/bitops/non-atomic.h>

Re: smp_mb() in asm-mips/bitops.h

[Atsushi Nemoto]

On Thu, 7 Jun 2007 13:23:44 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> Funny indeed ;-)  Below patch should do the trick.
> 
> Due to very inagressive memory reordering on the few non-strongly ordered
> MIPS SMP systems I am somewhat confident this didn't break anything but
> if so, Sibyte and PMC-Sierra RM9000 SMP systems are affected.
> 
>   Ralf
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

Thanks, looks good for me.

---
Atsushi Nemoto

Re: smp_mb() in asm-mips/bitops.h

[Atsushi Nemoto]

On Thu, 7 Jun 2007 13:23:44 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> @@ -310,6 +306,7 @@ static inline int test_and_clear_bit(unsigned long nr,
>  	volatile unsigned long *addr)
>  {
>  	unsigned short bit = nr & SZLONG_MASK;
> +	unsigned long res;
>  
>  	if (cpu_has_llsc && R10000_LLSC_WAR) {
>  		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);

You forgot to remove one more 'res' variable.


Subject: Remove a duplicated local variable in test_and_clear_bit()

Fix a sparse warning caused by 2c921d07f8c641e691b0dfd80a5cfe14c60ec489

include2/asm/bitops.h:313:23: warning: symbol 'res' shadows an earlier one
include2/asm/bitops.h:309:16: originally declared here

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h
index ffe245b..d9e81af 100644
--- a/include/asm-mips/bitops.h
+++ b/include/asm-mips/bitops.h
@@ -310,7 +310,7 @@ static inline int test_and_clear_bit(unsigned long nr,
 
 	if (cpu_has_llsc && R10000_LLSC_WAR) {
 		unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
-		unsigned long temp, res;
+		unsigned long temp;
 
 		__asm__ __volatile__(
 		"	.set	mips3					\n"

Re: smp_mb() in asm-mips/bitops.h

[Ralf Baechle]

On Thu, Jun 14, 2007 at 12:56:31AM +0900, Atsushi Nemoto wrote:

> You forgot to remove one more 'res' variable.
> 
> 
> Subject: Remove a duplicated local variable in test_and_clear_bit()
> 
> Fix a sparse warning caused by 2c921d07f8c641e691b0dfd80a5cfe14c60ec489

Applied, Thanks, alot!

  Ralf