cmpxchg broken in some situation

cmpxchg broken in some situation

[Fuxin Zhang]

hi,   
   Today I run across a possible bug of cmpxchg implementation. When 
playing with DRM on our Fulong, the following function 
(drivers/char/drm/drm_lock.c) is not working correctly in 64BIT mips:
   cmpxchg failed to set *lock to new value. (return 0 with *lock unchanged)
It is probably due to type conversions between unisigned int and 
unsigned long.  When I change cmpxchg to mycmpxchg(attached below), 
problem disappeared.                                 

int drm_lock_free(struct drm_lock_data *lock_data, unsigned int context)
{
        unsigned int old, new, prev;
        volatile unsigned int *lock = &lock_data->hw_lock->lock;

        spin_lock(&lock_data->spinlock);
        if (lock_data->kernel_waiters != 0) {
                drm_lock_transfer(lock_data, 0);
                lock_data->idle_has_lock = 1;
                spin_unlock(&lock_data->spinlock);
                return 1;
        }
        spin_unlock(&lock_data->spinlock);

        do {
                old = *lock;
                new = _DRM_LOCKING_CONTEXT(old);
                prev = cmpxchg(lock, old, new);
        } while (prev != old);

        if (_DRM_LOCK_IS_HELD(old) && _DRM_LOCKING_CONTEXT(old) != 
context) {
                DRM_ERROR("%d freed heavyweight lock held by %d\n",
                          context, _DRM_LOCKING_CONTEXT(old));
                return 1;
        }
        wake_up_interruptible(&lock_data->lock_queue);
        return 0;
}

static inline unsigned int mycmpxchg(volatile int * m, unsigned int old,
        unsigned int new)  //unsigned long to unsigned int
{
        __u32 retval;

        __asm__ __volatile__(
                        "       .set    
push                                    \n"
                        "       .set    
noat                                    \n"
                        "       .set    
mips3                                   \n"
                        "1:     ll      %0, %2                  # 
__mycmpxchg_u32       \n"
                        "       bne     %0, %z3, 
2f                             \n"
                        "       .set    
mips0                                   \n"
                        "       move    $1, 
%z4                                 \n"
                        "       .set    
mips3                                   \n"
                        "       sc      $1, 
%1                                  \n"
                        "       beqz    $1, 
3f                                  \n"
                        
"2:                                                     \n"
                        "       .subsection 
2                                   \n"
                        "3:     b       
1b                                      \n"
                        "       
.previous                                       \n"
                        "       .set    
pop                                     \n"
                        : "=&r" (retval), "=R" (*m)
                        : "R" (*m), "Jr" (old), "Jr" (new)
                        : "memory");

        smp_llsc_mb();

        return retval;
}

Re: cmpxchg broken in some situation

[Ralf Baechle]

On Sun, Sep 30, 2007 at 06:34:42PM +0800, Fuxin Zhang wrote:

> hi,   
>   Today I run across a possible bug of cmpxchg implementation. When 
> playing with DRM on our Fulong, the following function 
> (drivers/char/drm/drm_lock.c) is not working correctly in 64BIT mips:
>   cmpxchg failed to set *lock to new value. (return 0 with *lock unchanged)
> It is probably due to type conversions between unisigned int and 
> unsigned long.  When I change cmpxchg to mycmpxchg(attached below), 
> problem disappeared.                                 

Below a patch to implement the get_user() style big solution to type
safety in cmpxchg.  Can you test it?  Thanks,

  Ralf

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

 include/asm-mips/cmpxchg.h |  104 ++++++++++++++++++
 include/asm-mips/local.h   |    1 
 include/asm-mips/system.h  |  261 --------------------------------------------
 3 files changed, 106 insertions(+), 260 deletions(-)

diff --git a/include/asm-mips/cmpxchg.h b/include/asm-mips/cmpxchg.h
new file mode 100644
index 0000000..d1523dd
--- /dev/null
+++ b/include/asm-mips/cmpxchg.h
@@ -0,0 +1,104 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2003, 06, 07 by Ralf Baechle (ralf@linux-mips.org)
+ */
+#ifndef __ASM_CMPXCHG_H
+#define __ASM_CMPXCHG_H
+
+#include <linux/irqflags.h>
+
+#define __HAVE_ARCH_CMPXCHG 1
+
+#define __cmpxchg(ld, st, m, old, new)					\
+({									\
+	__typeof(*(m)) __ret;						\
+									\
+	if (cpu_has_llsc && R10000_LLSC_WAR) {				\
+		__asm__ __volatile__(					\
+		"	.set	push				\n"	\
+		"	.set	noat				\n"	\
+		"	.set	mips3				\n"	\
+		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
+		"	bne	%0, %z3, 2f			\n"	\
+		"	.set	mips0				\n"	\
+		"	move	$1, %z4				\n"	\
+		"	.set	mips3				\n"	\
+		"	" st "	$1, %1				\n"	\
+		"	beqzl	$1, 1b				\n"	\
+		"2:						\n"	\
+		"	.set	pop				\n"	\
+		: "=&r" (__ret), "=R" (*m)				\
+		: "R" (*m), "Jr" (old), "Jr" (new)			\
+		: "memory");						\
+	} else if (cpu_has_llsc) {					\
+		__asm__ __volatile__(					\
+		"	.set	push				\n"	\
+		"	.set	noat				\n"	\
+		"	.set	mips3				\n"	\
+		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
+		"	bne	%0, %z3, 2f			\n"	\
+		"	.set	mips0				\n"	\
+		"	move	$1, %z4				\n"	\
+		"	.set	mips3				\n"	\
+		"	" st "	$1, %1				\n"	\
+		"	beqz	$1, 3f				\n"	\
+		"2:						\n"	\
+		"	.subsection 2				\n"	\
+		"3:	b	1b				\n"	\
+		"	.previous				\n"	\
+		"	.set	pop				\n"	\
+		: "=&r" (__ret), "=R" (*m)				\
+		: "R" (*m), "Jr" (old), "Jr" (new)			\
+		: "memory");						\
+	} else {							\
+		unsigned long __flags;					\
+									\
+		raw_local_irq_save(__flags);				\
+		__ret = *m;						\
+		if (__ret == old)					\
+			*m = new;					\
+		raw_local_irq_restore(__flags);				\
+	}								\
+									\
+	smp_llsc_mb();							\
+									\
+	__ret;								\
+})
+
+/*
+ * This function doesn't exist, so you'll get a linker error
+ * if something tries to do an invalid cmpxchg().
+ */
+extern void __cmpxchg_called_with_bad_pointer(void);
+
+#define cmpxchg(ptr,old,new)						\
+({									\
+	__typeof__(ptr) __ptr = (ptr);					\
+	__typeof__(*(ptr)) __old = (old);				\
+	__typeof__(*(ptr)) __new = (new);				\
+	__typeof__(*(ptr)) __res = 0;					\
+									\
+	switch (sizeof(__ptr)) {					\
+	case 4:								\
+		__res = __cmpxchg("ll", "sc", __ptr, __old, __new);	\
+		break;							\
+	case 8:								\
+		if (sizeof(long) == 8) {				\
+			__res = __cmpxchg("lld", "scd", __ptr,		\
+					   __old, __new);		\
+			break;						\
+		}							\
+	default:							\
+		__cmpxchg_called_with_bad_pointer();			\
+		break;							\
+	}								\
+									\
+	__res;								\
+})
+
+#define cmpxchg_local(ptr, old, new) cmpxchg(ptr, old, new)
+
+#endif /* __ASM_CMPXCHG_H */
diff --git a/include/asm-mips/local.h b/include/asm-mips/local.h
index ed882c8..7034a01 100644
--- a/include/asm-mips/local.h
+++ b/include/asm-mips/local.h
@@ -4,6 +4,7 @@
 #include <linux/percpu.h>
 #include <linux/bitops.h>
 #include <asm/atomic.h>
+#include <asm/cmpxchg.h>
 #include <asm/war.h>
 
 typedef struct
diff --git a/include/asm-mips/system.h b/include/asm-mips/system.h
index 357251f..480b574 100644
--- a/include/asm-mips/system.h
+++ b/include/asm-mips/system.h
@@ -17,6 +17,7 @@
 
 #include <asm/addrspace.h>
 #include <asm/barrier.h>
+#include <asm/cmpxchg.h>
 #include <asm/cpu-features.h>
 #include <asm/dsp.h>
 #include <asm/war.h>
@@ -194,266 +195,6 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
 
 #define xchg(ptr,x) ((__typeof__(*(ptr)))__xchg((unsigned long)(x),(ptr),sizeof(*(ptr))))
 
-#define __HAVE_ARCH_CMPXCHG 1
-
-static inline unsigned long __cmpxchg_u32(volatile int * m, unsigned long old,
-	unsigned long new)
-{
-	__u32 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqz	$1, 3f					\n"
-		"2:							\n"
-		"	.subsection 2					\n"
-		"3:	b	1b					\n"
-		"	.previous					\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		raw_local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		raw_local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	smp_llsc_mb();
-
-	return retval;
-}
-
-static inline unsigned long __cmpxchg_u32_local(volatile int * m,
-	unsigned long old, unsigned long new)
-{
-	__u32 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqz	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	return retval;
-}
-
-#ifdef CONFIG_64BIT
-static inline unsigned long __cmpxchg_u64(volatile int * m, unsigned long old,
-	unsigned long new)
-{
-	__u64 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqz	$1, 3f					\n"
-		"2:							\n"
-		"	.subsection 2					\n"
-		"3:	b	1b					\n"
-		"	.previous					\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		raw_local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		raw_local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	smp_llsc_mb();
-
-	return retval;
-}
-
-static inline unsigned long __cmpxchg_u64_local(volatile int * m,
-	unsigned long old, unsigned long new)
-{
-	__u64 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqz	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	return retval;
-}
-
-#else
-extern unsigned long __cmpxchg_u64_unsupported_on_32bit_kernels(
-	volatile int * m, unsigned long old, unsigned long new);
-#define __cmpxchg_u64 __cmpxchg_u64_unsupported_on_32bit_kernels
-extern unsigned long __cmpxchg_u64_local_unsupported_on_32bit_kernels(
-	volatile int * m, unsigned long old, unsigned long new);
-#define __cmpxchg_u64_local __cmpxchg_u64_local_unsupported_on_32bit_kernels
-#endif
-
-/* This function doesn't exist, so you'll get a linker error
-   if something tries to do an invalid cmpxchg().  */
-extern void __cmpxchg_called_with_bad_pointer(void);
-
-static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old,
-	unsigned long new, int size)
-{
-	switch (size) {
-	case 4:
-		return __cmpxchg_u32(ptr, old, new);
-	case 8:
-		return __cmpxchg_u64(ptr, old, new);
-	}
-	__cmpxchg_called_with_bad_pointer();
-	return old;
-}
-
-static inline unsigned long __cmpxchg_local(volatile void * ptr,
-	unsigned long old, unsigned long new, int size)
-{
-	switch (size) {
-	case 4:
-		return __cmpxchg_u32_local(ptr, old, new);
-	case 8:
-		return __cmpxchg_u64_local(ptr, old, new);
-	}
-	__cmpxchg_called_with_bad_pointer();
-	return old;
-}
-
-#define cmpxchg(ptr,old,new) \
-	((__typeof__(*(ptr)))__cmpxchg((ptr), \
-		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
-
-#define cmpxchg_local(ptr,old,new) \
-	((__typeof__(*(ptr)))__cmpxchg_local((ptr), \
-		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
-
 extern void set_handler (unsigned long offset, void *addr, unsigned long len);
 extern void set_uncached_handler (unsigned long offset, void *addr, unsigned long len);
 

Re: cmpxchg broken in some situation

[David Daney]

Ralf Baechle wrote:
> +	} else if (cpu_has_llsc) {					\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqz	$1, 3f				\n"	\
> +		"2:						\n"	\
> +		"	.subsection 2				\n"	\
> +		"3:	b	1b				\n"	\
> +		"	.previous				\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
>   
Is a 'sync' needed after the 'sc'?

According to this message:
http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20070919084515.GM9972%40networkno.de
it would seem so.


David Daney

Re: cmpxchg broken in some situation

[David Daney]

David Daney wrote:
> Ralf Baechle wrote:
>> +    } else if (cpu_has_llsc) {                    \
>> +        __asm__ __volatile__(                    \
>> +        "    .set    push                \n"    \
>> +        "    .set    noat                \n"    \
>> +        "    .set    mips3                \n"    \
>> +        "1:    " ld "    %0, %2        # __cmpxchg_u32    \n"    \
>> +        "    bne    %0, %z3, 2f            \n"    \
>> +        "    .set    mips0                \n"    \
>> +        "    move    $1, %z4                \n"    \
>> +        "    .set    mips3                \n"    \
>> +        "    " st "    $1, %1                \n"    \
>> +        "    beqz    $1, 3f                \n"    \
>> +        "2:                        \n"    \
>> +        "    .subsection 2                \n"    \
>> +        "3:    b    1b                \n"    \
>> +        "    .previous                \n"    \
>> +        "    .set    pop                \n"    \
>> +        : "=&r" (__ret), "=R" (*m)                \
>> +        : "R" (*m), "Jr" (old), "Jr" (new)            \
>> +        : "memory");                        \
>>   
> Is a 'sync' needed after the 'sc'?
>
> According to this message:
> http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20070919084515.GM9972%40networkno.de 
>
> it would seem so.

Drat, I probably posted too soon.  That is the smp_llsc_mb(); isn't it.

David Daney

Re: cmpxchg broken in some situation

[Ralf Baechle]

On Sun, Sep 30, 2007 at 08:59:07PM -0700, David Daney wrote:

> David Daney wrote:
> >Ralf Baechle wrote:
> >>+    } else if (cpu_has_llsc) {                    \
> >>+        __asm__ __volatile__(                    \
> >>+        "    .set    push                \n"    \
> >>+        "    .set    noat                \n"    \
> >>+        "    .set    mips3                \n"    \
> >>+        "1:    " ld "    %0, %2        # __cmpxchg_u32    \n"    \
> >>+        "    bne    %0, %z3, 2f            \n"    \
> >>+        "    .set    mips0                \n"    \
> >>+        "    move    $1, %z4                \n"    \
> >>+        "    .set    mips3                \n"    \
> >>+        "    " st "    $1, %1                \n"    \
> >>+        "    beqz    $1, 3f                \n"    \
> >>+        "2:                        \n"    \
> >>+        "    .subsection 2                \n"    \
> >>+        "3:    b    1b                \n"    \
> >>+        "    .previous                \n"    \
> >>+        "    .set    pop                \n"    \
> >>+        : "=&r" (__ret), "=R" (*m)                \
> >>+        : "R" (*m), "Jr" (old), "Jr" (new)            \
> >>+        : "memory");                        \
> >>  
> >Is a 'sync' needed after the 'sc'?
> >
> >According to this message:
> >http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20070919084515.GM9972%40networkno.de 
> >
> >it would seem so.
> 
> Drat, I probably posted too soon.  That is the smp_llsc_mb(); isn't it.

Yes - and the answer to your original question is a clear and definate
maybe ;-)

In the kernel we can afford to optimize for every piece of silicon on earth.
In userspace we can't make that sort of compile time choices as easily so
it's a better idea to just litter a few SYNCs over the code.

  Ralf

Re: cmpxchg broken in some situation

[Fuxin Zhang]

Sorry that it seems not work:
the kernel oops at sysfs_open_file->sysfs_get_active with unaligned 
access(last seen exception on screen, no serial console by hand so it 
may not be the first exception). It is probably caused by 
"atomic_cmpxchg" there.
And keep the old kernel using new modules with patched cmpxchg also lead 
to glxgears die(should be lock problem like before).

Ralf Baechle 写道:
> On Sun, Sep 30, 2007 at 06:34:42PM +0800, Fuxin Zhang wrote:
>
>   
>> hi,   
>>   Today I run across a possible bug of cmpxchg implementation. When 
>> playing with DRM on our Fulong, the following function 
>> (drivers/char/drm/drm_lock.c) is not working correctly in 64BIT mips:
>>   cmpxchg failed to set *lock to new value. (return 0 with *lock unchanged)
>> It is probably due to type conversions between unisigned int and 
>> unsigned long.  When I change cmpxchg to mycmpxchg(attached below), 
>> problem disappeared.                                 
>>     
>
> Below a patch to implement the get_user() style big solution to type
> safety in cmpxchg.  Can you test it?  Thanks,
>
>   Ralf
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>
>  include/asm-mips/cmpxchg.h |  104 ++++++++++++++++++
>  include/asm-mips/local.h   |    1 
>  include/asm-mips/system.h  |  261 --------------------------------------------
>  3 files changed, 106 insertions(+), 260 deletions(-)
>
> diff --git a/include/asm-mips/cmpxchg.h b/include/asm-mips/cmpxchg.h
> new file mode 100644
> index 0000000..d1523dd
> --- /dev/null
> +++ b/include/asm-mips/cmpxchg.h
> @@ -0,0 +1,104 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2003, 06, 07 by Ralf Baechle (ralf@linux-mips.org)
> + */
> +#ifndef __ASM_CMPXCHG_H
> +#define __ASM_CMPXCHG_H
> +
> +#include <linux/irqflags.h>
> +
> +#define __HAVE_ARCH_CMPXCHG 1
> +
> +#define __cmpxchg(ld, st, m, old, new)					\
> +({									\
> +	__typeof(*(m)) __ret;						\
> +									\
> +	if (cpu_has_llsc && R10000_LLSC_WAR) {				\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqzl	$1, 1b				\n"	\
> +		"2:						\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
> +	} else if (cpu_has_llsc) {					\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqz	$1, 3f				\n"	\
> +		"2:						\n"	\
> +		"	.subsection 2				\n"	\
> +		"3:	b	1b				\n"	\
> +		"	.previous				\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
> +	} else {							\
> +		unsigned long __flags;					\
> +									\
> +		raw_local_irq_save(__flags);				\
> +		__ret = *m;						\
> +		if (__ret == old)					\
> +			*m = new;					\
> +		raw_local_irq_restore(__flags);				\
> +	}								\
> +									\
> +	smp_llsc_mb();							\
> +									\
> +	__ret;								\
> +})
> +
> +/*
> + * This function doesn't exist, so you'll get a linker error
> + * if something tries to do an invalid cmpxchg().
> + */
> +extern void __cmpxchg_called_with_bad_pointer(void);
> +
> +#define cmpxchg(ptr,old,new)						\
> +({									\
> +	__typeof__(ptr) __ptr = (ptr);					\
> +	__typeof__(*(ptr)) __old = (old);				\
> +	__typeof__(*(ptr)) __new = (new);				\
> +	__typeof__(*(ptr)) __res = 0;					\
> +									\
> +	switch (sizeof(__ptr)) {					\
> +	case 4:								\
> +		__res = __cmpxchg("ll", "sc", __ptr, __old, __new);	\
> +		break;							\
> +	case 8:								\
> +		if (sizeof(long) == 8) {				\
> +			__res = __cmpxchg("lld", "scd", __ptr,		\
> +					   __old, __new);		\
> +			break;						\
> +		}							\
> +	default:							\
> +		__cmpxchg_called_with_bad_pointer();			\
> +		break;							\
> +	}								\
> +									\
> +	__res;								\
> +})
> +
> +#define cmpxchg_local(ptr, old, new) cmpxchg(ptr, old, new)
> +
> +#endif /* __ASM_CMPXCHG_H */
> diff --git a/include/asm-mips/local.h b/include/asm-mips/local.h
> index ed882c8..7034a01 100644
> --- a/include/asm-mips/local.h
> +++ b/include/asm-mips/local.h
> @@ -4,6 +4,7 @@
>  #include <linux/percpu.h>
>  #include <linux/bitops.h>
>  #include <asm/atomic.h>
> +#include <asm/cmpxchg.h>
>  #include <asm/war.h>
>  
>  typedef struct
> diff --git a/include/asm-mips/system.h b/include/asm-mips/system.h
> index 357251f..480b574 100644
> --- a/include/asm-mips/system.h
> +++ b/include/asm-mips/system.h
> @@ -17,6 +17,7 @@
>  
>  #include <asm/addrspace.h>
>  #include <asm/barrier.h>
> +#include <asm/cmpxchg.h>
>  #include <asm/cpu-features.h>
>  #include <asm/dsp.h>
>  #include <asm/war.h>
> @@ -194,266 +195,6 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
>  
>  #define xchg(ptr,x) ((__typeof__(*(ptr)))__xchg((unsigned long)(x),(ptr),sizeof(*(ptr))))
>  
> -#define __HAVE_ARCH_CMPXCHG 1
> -
> -static inline unsigned long __cmpxchg_u32(volatile int * m, unsigned long old,
> -	unsigned long new)
> -{
> -	__u32 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqz	$1, 3f					\n"
> -		"2:							\n"
> -		"	.subsection 2					\n"
> -		"3:	b	1b					\n"
> -		"	.previous					\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		raw_local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		raw_local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	smp_llsc_mb();
> -
> -	return retval;
> -}
> -
> -static inline unsigned long __cmpxchg_u32_local(volatile int * m,
> -	unsigned long old, unsigned long new)
> -{
> -	__u32 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqz	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	return retval;
> -}
> -
> -#ifdef CONFIG_64BIT
> -static inline unsigned long __cmpxchg_u64(volatile int * m, unsigned long old,
> -	unsigned long new)
> -{
> -	__u64 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqz	$1, 3f					\n"
> -		"2:							\n"
> -		"	.subsection 2					\n"
> -		"3:	b	1b					\n"
> -		"	.previous					\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		raw_local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		raw_local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	smp_llsc_mb();
> -
> -	return retval;
> -}
> -
> -static inline unsigned long __cmpxchg_u64_local(volatile int * m,
> -	unsigned long old, unsigned long new)
> -{
> -	__u64 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqz	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	return retval;
> -}
> -
> -#else
> -extern unsigned long __cmpxchg_u64_unsupported_on_32bit_kernels(
> -	volatile int * m, unsigned long old, unsigned long new);
> -#define __cmpxchg_u64 __cmpxchg_u64_unsupported_on_32bit_kernels
> -extern unsigned long __cmpxchg_u64_local_unsupported_on_32bit_kernels(
> -	volatile int * m, unsigned long old, unsigned long new);
> -#define __cmpxchg_u64_local __cmpxchg_u64_local_unsupported_on_32bit_kernels
> -#endif
> -
> -/* This function doesn't exist, so you'll get a linker error
> -   if something tries to do an invalid cmpxchg().  */
> -extern void __cmpxchg_called_with_bad_pointer(void);
> -
> -static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old,
> -	unsigned long new, int size)
> -{
> -	switch (size) {
> -	case 4:
> -		return __cmpxchg_u32(ptr, old, new);
> -	case 8:
> -		return __cmpxchg_u64(ptr, old, new);
> -	}
> -	__cmpxchg_called_with_bad_pointer();
> -	return old;
> -}
> -
> -static inline unsigned long __cmpxchg_local(volatile void * ptr,
> -	unsigned long old, unsigned long new, int size)
> -{
> -	switch (size) {
> -	case 4:
> -		return __cmpxchg_u32_local(ptr, old, new);
> -	case 8:
> -		return __cmpxchg_u64_local(ptr, old, new);
> -	}
> -	__cmpxchg_called_with_bad_pointer();
> -	return old;
> -}
> -
> -#define cmpxchg(ptr,old,new) \
> -	((__typeof__(*(ptr)))__cmpxchg((ptr), \
> -		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
> -
> -#define cmpxchg_local(ptr,old,new) \
> -	((__typeof__(*(ptr)))__cmpxchg_local((ptr), \
> -		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
> -
>  extern void set_handler (unsigned long offset, void *addr, unsigned long len);
>  extern void set_uncached_handler (unsigned long offset, void *addr, unsigned long len);
>  
>
>
>
>   

Re: cmpxchg broken in some situation

[Ralf Baechle]

On Mon, Oct 01, 2007 at 11:11:17PM +0800, Fuxin Zhang wrote:

> Sorry that it seems not work:
> the kernel oops at sysfs_open_file->sysfs_get_active with unaligned 
> access(last seen exception on screen, no serial console by hand so it 
> may not be the first exception). It is probably caused by 
> "atomic_cmpxchg" there.
> And keep the old kernel using new modules with patched cmpxchg also lead 
> to glxgears die(should be lock problem like before).

Can you look at the disassembly of the generated code?  It should hopefully
be relativly obvious in the disassembly.

  Ralf

Re: cmpxchg broken in some situation

[Fuxin Zhang]

The problem is here:

switch (sizeof(__ptr)) { // --> should be sizeof(*(__ptr))
case 4:
...
Recompiling..

Ralf Baechle 写道:
> On Mon, Oct 01, 2007 at 11:11:17PM +0800, Fuxin Zhang wrote:
>
>   
>> Sorry that it seems not work:
>> the kernel oops at sysfs_open_file->sysfs_get_active with unaligned 
>> access(last seen exception on screen, no serial console by hand so it 
>> may not be the first exception). It is probably caused by 
>> "atomic_cmpxchg" there.
>> And keep the old kernel using new modules with patched cmpxchg also lead 
>> to glxgears die(should be lock problem like before).
>>     
>
> Can you look at the disassembly of the generated code?  It should hopefully
> be relativly obvious in the disassembly.
>
>   Ralf
>
>
>
>   

Re: cmpxchg broken in some situation

[Ralf Baechle]

On Tue, Oct 02, 2007 at 05:34:44PM +0800, Fuxin Zhang wrote:

> The problem is here:
> 
> switch (sizeof(__ptr)) { // --> should be sizeof(*(__ptr))
> case 4:
> ...
> Recompiling..

There was another small kink, cmpxchg_local() does not imply a memory
barrier so I optimized that case.

And I don't complain about it being 151 lines shorter ;-)

  Ralf

From: Ralf Baechle <ralf@linux-mips.org>

[MIPS] Typeproof reimplementation of cmpxchg.

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

diff --git a/include/asm-mips/cmpxchg.h b/include/asm-mips/cmpxchg.h
new file mode 100644
index 0000000..46bac47
--- /dev/null
+++ b/include/asm-mips/cmpxchg.h
@@ -0,0 +1,107 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2003, 06, 07 by Ralf Baechle (ralf@linux-mips.org)
+ */
+#ifndef __ASM_CMPXCHG_H
+#define __ASM_CMPXCHG_H
+
+#include <linux/irqflags.h>
+
+#define __HAVE_ARCH_CMPXCHG 1
+
+#define __cmpxchg_asm(ld, st, m, old, new)				\
+({									\
+	__typeof(*(m)) __ret;						\
+									\
+	if (cpu_has_llsc && R10000_LLSC_WAR) {				\
+		__asm__ __volatile__(					\
+		"	.set	push				\n"	\
+		"	.set	noat				\n"	\
+		"	.set	mips3				\n"	\
+		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
+		"	bne	%0, %z3, 2f			\n"	\
+		"	.set	mips0				\n"	\
+		"	move	$1, %z4				\n"	\
+		"	.set	mips3				\n"	\
+		"	" st "	$1, %1				\n"	\
+		"	beqzl	$1, 1b				\n"	\
+		"2:						\n"	\
+		"	.set	pop				\n"	\
+		: "=&r" (__ret), "=R" (*m)				\
+		: "R" (*m), "Jr" (old), "Jr" (new)			\
+		: "memory");						\
+	} else if (cpu_has_llsc) {					\
+		__asm__ __volatile__(					\
+		"	.set	push				\n"	\
+		"	.set	noat				\n"	\
+		"	.set	mips3				\n"	\
+		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
+		"	bne	%0, %z3, 2f			\n"	\
+		"	.set	mips0				\n"	\
+		"	move	$1, %z4				\n"	\
+		"	.set	mips3				\n"	\
+		"	" st "	$1, %1				\n"	\
+		"	beqz	$1, 3f				\n"	\
+		"2:						\n"	\
+		"	.subsection 2				\n"	\
+		"3:	b	1b				\n"	\
+		"	.previous				\n"	\
+		"	.set	pop				\n"	\
+		: "=&r" (__ret), "=R" (*m)				\
+		: "R" (*m), "Jr" (old), "Jr" (new)			\
+		: "memory");						\
+	} else {							\
+		unsigned long __flags;					\
+									\
+		raw_local_irq_save(__flags);				\
+		__ret = *m;						\
+		if (__ret == old)					\
+			*m = new;					\
+		raw_local_irq_restore(__flags);				\
+	}								\
+									\
+	smp_llsc_mb();							\
+									\
+	__ret;								\
+})
+
+/*
+ * This function doesn't exist, so you'll get a linker error
+ * if something tries to do an invalid cmpxchg().
+ */
+extern void __cmpxchg_called_with_bad_pointer(void);
+
+#define __cmpxchg(ptr,old,new,barrier)					\
+({									\
+	__typeof__(ptr) __ptr = (ptr);					\
+	__typeof__(*(ptr)) __old = (old);				\
+	__typeof__(*(ptr)) __new = (new);				\
+	__typeof__(*(ptr)) __res = 0;					\
+									\
+	switch (sizeof(*(__ptr))) {					\
+	case 4:								\
+		__res = __cmpxchg_asm("ll", "sc", __ptr, __old, __new);	\
+		break;							\
+	case 8:								\
+		if (sizeof(long) == 8) {				\
+			__res = __cmpxchg_asm("lld", "scd", __ptr,	\
+					   __old, __new);		\
+			break;						\
+		}							\
+	default:							\
+		__cmpxchg_called_with_bad_pointer();			\
+		break;							\
+	}								\
+									\
+	barrier;							\
+									\
+	__res;								\
+})
+
+#define cmpxchg(ptr, old, new)		__cmpxchg(ptr, old, new, smp_llsc_mb())
+#define cmpxchg_local(ptr, old, new)	__cmpxchg(ptr, old, new,)
+
+#endif /* __ASM_CMPXCHG_H */
diff --git a/include/asm-mips/local.h b/include/asm-mips/local.h
index ed882c8..7034a01 100644
--- a/include/asm-mips/local.h
+++ b/include/asm-mips/local.h
@@ -4,6 +4,7 @@
 #include <linux/percpu.h>
 #include <linux/bitops.h>
 #include <asm/atomic.h>
+#include <asm/cmpxchg.h>
 #include <asm/war.h>
 
 typedef struct
diff --git a/include/asm-mips/system.h b/include/asm-mips/system.h
index 357251f..480b574 100644
--- a/include/asm-mips/system.h
+++ b/include/asm-mips/system.h
@@ -17,6 +17,7 @@
 
 #include <asm/addrspace.h>
 #include <asm/barrier.h>
+#include <asm/cmpxchg.h>
 #include <asm/cpu-features.h>
 #include <asm/dsp.h>
 #include <asm/war.h>
@@ -194,266 +195,6 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
 
 #define xchg(ptr,x) ((__typeof__(*(ptr)))__xchg((unsigned long)(x),(ptr),sizeof(*(ptr))))
 
-#define __HAVE_ARCH_CMPXCHG 1
-
-static inline unsigned long __cmpxchg_u32(volatile int * m, unsigned long old,
-	unsigned long new)
-{
-	__u32 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqz	$1, 3f					\n"
-		"2:							\n"
-		"	.subsection 2					\n"
-		"3:	b	1b					\n"
-		"	.previous					\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		raw_local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		raw_local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	smp_llsc_mb();
-
-	return retval;
-}
-
-static inline unsigned long __cmpxchg_u32_local(volatile int * m,
-	unsigned long old, unsigned long new)
-{
-	__u32 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	.set	mips0					\n"
-		"	move	$1, %z4					\n"
-		"	.set	mips3					\n"
-		"	sc	$1, %1					\n"
-		"	beqz	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	return retval;
-}
-
-#ifdef CONFIG_64BIT
-static inline unsigned long __cmpxchg_u64(volatile int * m, unsigned long old,
-	unsigned long new)
-{
-	__u64 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqz	$1, 3f					\n"
-		"2:							\n"
-		"	.subsection 2					\n"
-		"3:	b	1b					\n"
-		"	.previous					\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		raw_local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		raw_local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	smp_llsc_mb();
-
-	return retval;
-}
-
-static inline unsigned long __cmpxchg_u64_local(volatile int * m,
-	unsigned long old, unsigned long new)
-{
-	__u64 retval;
-
-	if (cpu_has_llsc && R10000_LLSC_WAR) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqzl	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else if (cpu_has_llsc) {
-		__asm__ __volatile__(
-		"	.set	push					\n"
-		"	.set	noat					\n"
-		"	.set	mips3					\n"
-		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
-		"	bne	%0, %z3, 2f				\n"
-		"	move	$1, %z4					\n"
-		"	scd	$1, %1					\n"
-		"	beqz	$1, 1b					\n"
-		"2:							\n"
-		"	.set	pop					\n"
-		: "=&r" (retval), "=R" (*m)
-		: "R" (*m), "Jr" (old), "Jr" (new)
-		: "memory");
-	} else {
-		unsigned long flags;
-
-		local_irq_save(flags);
-		retval = *m;
-		if (retval == old)
-			*m = new;
-		local_irq_restore(flags);	/* implies memory barrier  */
-	}
-
-	return retval;
-}
-
-#else
-extern unsigned long __cmpxchg_u64_unsupported_on_32bit_kernels(
-	volatile int * m, unsigned long old, unsigned long new);
-#define __cmpxchg_u64 __cmpxchg_u64_unsupported_on_32bit_kernels
-extern unsigned long __cmpxchg_u64_local_unsupported_on_32bit_kernels(
-	volatile int * m, unsigned long old, unsigned long new);
-#define __cmpxchg_u64_local __cmpxchg_u64_local_unsupported_on_32bit_kernels
-#endif
-
-/* This function doesn't exist, so you'll get a linker error
-   if something tries to do an invalid cmpxchg().  */
-extern void __cmpxchg_called_with_bad_pointer(void);
-
-static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old,
-	unsigned long new, int size)
-{
-	switch (size) {
-	case 4:
-		return __cmpxchg_u32(ptr, old, new);
-	case 8:
-		return __cmpxchg_u64(ptr, old, new);
-	}
-	__cmpxchg_called_with_bad_pointer();
-	return old;
-}
-
-static inline unsigned long __cmpxchg_local(volatile void * ptr,
-	unsigned long old, unsigned long new, int size)
-{
-	switch (size) {
-	case 4:
-		return __cmpxchg_u32_local(ptr, old, new);
-	case 8:
-		return __cmpxchg_u64_local(ptr, old, new);
-	}
-	__cmpxchg_called_with_bad_pointer();
-	return old;
-}
-
-#define cmpxchg(ptr,old,new) \
-	((__typeof__(*(ptr)))__cmpxchg((ptr), \
-		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
-
-#define cmpxchg_local(ptr,old,new) \
-	((__typeof__(*(ptr)))__cmpxchg_local((ptr), \
-		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
-
 extern void set_handler (unsigned long offset, void *addr, unsigned long len);
 extern void set_uncached_handler (unsigned long offset, void *addr, unsigned long len);
 

Re: cmpxchg broken in some situation

[Thiemo Seufer]

Ralf Baechle wrote:
> On Tue, Oct 02, 2007 at 05:34:44PM +0800, Fuxin Zhang wrote:
> 
> > The problem is here:
> > 
> > switch (sizeof(__ptr)) { // --> should be sizeof(*(__ptr))
> > case 4:
> > ...
> > Recompiling..
> 
> There was another small kink, cmpxchg_local() does not imply a memory
> barrier so I optimized that case.
> 
> And I don't complain about it being 151 lines shorter ;-)
> 
>   Ralf
> 
> From: Ralf Baechle <ralf@linux-mips.org>
> 
> [MIPS] Typeproof reimplementation of cmpxchg.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> diff --git a/include/asm-mips/cmpxchg.h b/include/asm-mips/cmpxchg.h
> new file mode 100644
> index 0000000..46bac47
> --- /dev/null
> +++ b/include/asm-mips/cmpxchg.h
> @@ -0,0 +1,107 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2003, 06, 07 by Ralf Baechle (ralf@linux-mips.org)
> + */
> +#ifndef __ASM_CMPXCHG_H
> +#define __ASM_CMPXCHG_H
> +
> +#include <linux/irqflags.h>
> +
> +#define __HAVE_ARCH_CMPXCHG 1
> +
> +#define __cmpxchg_asm(ld, st, m, old, new)				\
> +({									\
> +	__typeof(*(m)) __ret;						\
> +									\
> +	if (cpu_has_llsc && R10000_LLSC_WAR) {				\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqzl	$1, 1b				\n"	\
> +		"2:						\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
> +	} else if (cpu_has_llsc) {					\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqz	$1, 3f				\n"	\
> +		"2:						\n"	\
> +		"	.subsection 2				\n"	\
> +		"3:	b	1b				\n"	\
> +		"	.previous				\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
> +	} else {							\
> +		unsigned long __flags;					\
> +									\
> +		raw_local_irq_save(__flags);				\
> +		__ret = *m;						\
> +		if (__ret == old)					\
> +			*m = new;					\
> +		raw_local_irq_restore(__flags);				\
> +	}								\
> +									\
> +	smp_llsc_mb();							\

I think this line is surplus.

> +									\
> +	__ret;								\
> +})
> +
> +/*
> + * This function doesn't exist, so you'll get a linker error
> + * if something tries to do an invalid cmpxchg().
> + */
> +extern void __cmpxchg_called_with_bad_pointer(void);
> +
> +#define __cmpxchg(ptr,old,new,barrier)					\
> +({									\
> +	__typeof__(ptr) __ptr = (ptr);					\
> +	__typeof__(*(ptr)) __old = (old);				\
> +	__typeof__(*(ptr)) __new = (new);				\
> +	__typeof__(*(ptr)) __res = 0;					\

Maybe we need an acquire barrier here for some systems.

> +	switch (sizeof(*(__ptr))) {					\
> +	case 4:								\
> +		__res = __cmpxchg_asm("ll", "sc", __ptr, __old, __new);	\
> +		break;							\
> +	case 8:								\
> +		if (sizeof(long) == 8) {				\
> +			__res = __cmpxchg_asm("lld", "scd", __ptr,	\
> +					   __old, __new);		\
> +			break;						\
> +		}							\
> +	default:							\
> +		__cmpxchg_called_with_bad_pointer();			\
> +		break;							\
> +	}								\
> +									\
> +	barrier;							\
> +									\
> +	__res;								\
> +})
> +
> +#define cmpxchg(ptr, old, new)		__cmpxchg(ptr, old, new, smp_llsc_mb())
> +#define cmpxchg_local(ptr, old, new)	__cmpxchg(ptr, old, new,)
> +
> +#endif /* __ASM_CMPXCHG_H */


Thiemo

Re: cmpxchg broken in some situation

[Fuxin Zhang]

And the comment "# cmpxchg_u32" is out of date too:)

Ralf Baechle 写道:
> On Tue, Oct 02, 2007 at 05:34:44PM +0800, Fuxin Zhang wrote:
>
>   
>> The problem is here:
>>
>> switch (sizeof(__ptr)) { // --> should be sizeof(*(__ptr))
>> case 4:
>> ...
>> Recompiling..
>>     
>
> There was another small kink, cmpxchg_local() does not imply a memory
> barrier so I optimized that case.
>
> And I don't complain about it being 151 lines shorter ;-)
>
>   Ralf
>
> From: Ralf Baechle <ralf@linux-mips.org>
>
> [MIPS] Typeproof reimplementation of cmpxchg.
>
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>
> diff --git a/include/asm-mips/cmpxchg.h b/include/asm-mips/cmpxchg.h
> new file mode 100644
> index 0000000..46bac47
> --- /dev/null
> +++ b/include/asm-mips/cmpxchg.h
> @@ -0,0 +1,107 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2003, 06, 07 by Ralf Baechle (ralf@linux-mips.org)
> + */
> +#ifndef __ASM_CMPXCHG_H
> +#define __ASM_CMPXCHG_H
> +
> +#include <linux/irqflags.h>
> +
> +#define __HAVE_ARCH_CMPXCHG 1
> +
> +#define __cmpxchg_asm(ld, st, m, old, new)				\
> +({									\
> +	__typeof(*(m)) __ret;						\
> +									\
> +	if (cpu_has_llsc && R10000_LLSC_WAR) {				\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqzl	$1, 1b				\n"	\
> +		"2:						\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
> +	} else if (cpu_has_llsc) {					\
> +		__asm__ __volatile__(					\
> +		"	.set	push				\n"	\
> +		"	.set	noat				\n"	\
> +		"	.set	mips3				\n"	\
> +		"1:	" ld "	%0, %2		# __cmpxchg_u32	\n"	\
> +		"	bne	%0, %z3, 2f			\n"	\
> +		"	.set	mips0				\n"	\
> +		"	move	$1, %z4				\n"	\
> +		"	.set	mips3				\n"	\
> +		"	" st "	$1, %1				\n"	\
> +		"	beqz	$1, 3f				\n"	\
> +		"2:						\n"	\
> +		"	.subsection 2				\n"	\
> +		"3:	b	1b				\n"	\
> +		"	.previous				\n"	\
> +		"	.set	pop				\n"	\
> +		: "=&r" (__ret), "=R" (*m)				\
> +		: "R" (*m), "Jr" (old), "Jr" (new)			\
> +		: "memory");						\
> +	} else {							\
> +		unsigned long __flags;					\
> +									\
> +		raw_local_irq_save(__flags);				\
> +		__ret = *m;						\
> +		if (__ret == old)					\
> +			*m = new;					\
> +		raw_local_irq_restore(__flags);				\
> +	}								\
> +									\
> +	smp_llsc_mb();							\
> +									\
> +	__ret;								\
> +})
> +
> +/*
> + * This function doesn't exist, so you'll get a linker error
> + * if something tries to do an invalid cmpxchg().
> + */
> +extern void __cmpxchg_called_with_bad_pointer(void);
> +
> +#define __cmpxchg(ptr,old,new,barrier)					\
> +({									\
> +	__typeof__(ptr) __ptr = (ptr);					\
> +	__typeof__(*(ptr)) __old = (old);				\
> +	__typeof__(*(ptr)) __new = (new);				\
> +	__typeof__(*(ptr)) __res = 0;					\
> +									\
> +	switch (sizeof(*(__ptr))) {					\
> +	case 4:								\
> +		__res = __cmpxchg_asm("ll", "sc", __ptr, __old, __new);	\
> +		break;							\
> +	case 8:								\
> +		if (sizeof(long) == 8) {				\
> +			__res = __cmpxchg_asm("lld", "scd", __ptr,	\
> +					   __old, __new);		\
> +			break;						\
> +		}							\
> +	default:							\
> +		__cmpxchg_called_with_bad_pointer();			\
> +		break;							\
> +	}								\
> +									\
> +	barrier;							\
> +									\
> +	__res;								\
> +})
> +
> +#define cmpxchg(ptr, old, new)		__cmpxchg(ptr, old, new, smp_llsc_mb())
> +#define cmpxchg_local(ptr, old, new)	__cmpxchg(ptr, old, new,)
> +
> +#endif /* __ASM_CMPXCHG_H */
> diff --git a/include/asm-mips/local.h b/include/asm-mips/local.h
> index ed882c8..7034a01 100644
> --- a/include/asm-mips/local.h
> +++ b/include/asm-mips/local.h
> @@ -4,6 +4,7 @@
>  #include <linux/percpu.h>
>  #include <linux/bitops.h>
>  #include <asm/atomic.h>
> +#include <asm/cmpxchg.h>
>  #include <asm/war.h>
>  
>  typedef struct
> diff --git a/include/asm-mips/system.h b/include/asm-mips/system.h
> index 357251f..480b574 100644
> --- a/include/asm-mips/system.h
> +++ b/include/asm-mips/system.h
> @@ -17,6 +17,7 @@
>  
>  #include <asm/addrspace.h>
>  #include <asm/barrier.h>
> +#include <asm/cmpxchg.h>
>  #include <asm/cpu-features.h>
>  #include <asm/dsp.h>
>  #include <asm/war.h>
> @@ -194,266 +195,6 @@ static inline unsigned long __xchg(unsigned long x, volatile void * ptr, int siz
>  
>  #define xchg(ptr,x) ((__typeof__(*(ptr)))__xchg((unsigned long)(x),(ptr),sizeof(*(ptr))))
>  
> -#define __HAVE_ARCH_CMPXCHG 1
> -
> -static inline unsigned long __cmpxchg_u32(volatile int * m, unsigned long old,
> -	unsigned long new)
> -{
> -	__u32 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqz	$1, 3f					\n"
> -		"2:							\n"
> -		"	.subsection 2					\n"
> -		"3:	b	1b					\n"
> -		"	.previous					\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		raw_local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		raw_local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	smp_llsc_mb();
> -
> -	return retval;
> -}
> -
> -static inline unsigned long __cmpxchg_u32_local(volatile int * m,
> -	unsigned long old, unsigned long new)
> -{
> -	__u32 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	ll	%0, %2			# __cmpxchg_u32	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	.set	mips0					\n"
> -		"	move	$1, %z4					\n"
> -		"	.set	mips3					\n"
> -		"	sc	$1, %1					\n"
> -		"	beqz	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	return retval;
> -}
> -
> -#ifdef CONFIG_64BIT
> -static inline unsigned long __cmpxchg_u64(volatile int * m, unsigned long old,
> -	unsigned long new)
> -{
> -	__u64 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqz	$1, 3f					\n"
> -		"2:							\n"
> -		"	.subsection 2					\n"
> -		"3:	b	1b					\n"
> -		"	.previous					\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		raw_local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		raw_local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	smp_llsc_mb();
> -
> -	return retval;
> -}
> -
> -static inline unsigned long __cmpxchg_u64_local(volatile int * m,
> -	unsigned long old, unsigned long new)
> -{
> -	__u64 retval;
> -
> -	if (cpu_has_llsc && R10000_LLSC_WAR) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqzl	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else if (cpu_has_llsc) {
> -		__asm__ __volatile__(
> -		"	.set	push					\n"
> -		"	.set	noat					\n"
> -		"	.set	mips3					\n"
> -		"1:	lld	%0, %2			# __cmpxchg_u64	\n"
> -		"	bne	%0, %z3, 2f				\n"
> -		"	move	$1, %z4					\n"
> -		"	scd	$1, %1					\n"
> -		"	beqz	$1, 1b					\n"
> -		"2:							\n"
> -		"	.set	pop					\n"
> -		: "=&r" (retval), "=R" (*m)
> -		: "R" (*m), "Jr" (old), "Jr" (new)
> -		: "memory");
> -	} else {
> -		unsigned long flags;
> -
> -		local_irq_save(flags);
> -		retval = *m;
> -		if (retval == old)
> -			*m = new;
> -		local_irq_restore(flags);	/* implies memory barrier  */
> -	}
> -
> -	return retval;
> -}
> -
> -#else
> -extern unsigned long __cmpxchg_u64_unsupported_on_32bit_kernels(
> -	volatile int * m, unsigned long old, unsigned long new);
> -#define __cmpxchg_u64 __cmpxchg_u64_unsupported_on_32bit_kernels
> -extern unsigned long __cmpxchg_u64_local_unsupported_on_32bit_kernels(
> -	volatile int * m, unsigned long old, unsigned long new);
> -#define __cmpxchg_u64_local __cmpxchg_u64_local_unsupported_on_32bit_kernels
> -#endif
> -
> -/* This function doesn't exist, so you'll get a linker error
> -   if something tries to do an invalid cmpxchg().  */
> -extern void __cmpxchg_called_with_bad_pointer(void);
> -
> -static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old,
> -	unsigned long new, int size)
> -{
> -	switch (size) {
> -	case 4:
> -		return __cmpxchg_u32(ptr, old, new);
> -	case 8:
> -		return __cmpxchg_u64(ptr, old, new);
> -	}
> -	__cmpxchg_called_with_bad_pointer();
> -	return old;
> -}
> -
> -static inline unsigned long __cmpxchg_local(volatile void * ptr,
> -	unsigned long old, unsigned long new, int size)
> -{
> -	switch (size) {
> -	case 4:
> -		return __cmpxchg_u32_local(ptr, old, new);
> -	case 8:
> -		return __cmpxchg_u64_local(ptr, old, new);
> -	}
> -	__cmpxchg_called_with_bad_pointer();
> -	return old;
> -}
> -
> -#define cmpxchg(ptr,old,new) \
> -	((__typeof__(*(ptr)))__cmpxchg((ptr), \
> -		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
> -
> -#define cmpxchg_local(ptr,old,new) \
> -	((__typeof__(*(ptr)))__cmpxchg_local((ptr), \
> -		(unsigned long)(old), (unsigned long)(new),sizeof(*(ptr))))
> -
>  extern void set_handler (unsigned long offset, void *addr, unsigned long len);
>  extern void set_uncached_handler (unsigned long offset, void *addr, unsigned long len);
>  
>
>
>
>   

Re: cmpxchg broken in some situation

[Ralf Baechle]

On Wed, Oct 03, 2007 at 06:48:28AM +0800, Fuxin Zhang wrote:

> And the comment "# cmpxchg_u32" is out of date too:)

I've seen worse bugs ;-)

  Ralf

Re: cmpxchg broken in some situation

[Fuxin Zhang]

en? It seems that it is working for my case now.

Ralf Baechle 写道:
> On Wed, Oct 03, 2007 at 06:48:28AM +0800, Fuxin Zhang wrote:
>
>   
>> And the comment "# cmpxchg_u32" is out of date too:)
>>     
>
> I've seen worse bugs ;-)
>
>   Ralf
>
>
>
>
>   

Re: cmpxchg broken in some situation

[Ralf Baechle]

On Tue, Oct 02, 2007 at 03:22:10PM +0100, Thiemo Seufer wrote:

> > +#define __cmpxchg(ptr,old,new,barrier)					\
> > +({									\
> > +	__typeof__(ptr) __ptr = (ptr);					\
> > +	__typeof__(*(ptr)) __old = (old);				\
> > +	__typeof__(*(ptr)) __new = (new);				\
> > +	__typeof__(*(ptr)) __res = 0;					\
> 
> Maybe we need an acquire barrier here for some systems.

Release you meant.  The acquire lock would be at the end.  Documentation
and actual implmeentations of cmpxchg seem to differ.  It's a relativly
rarely used primitve so I think I err on the side of paranoia for now
and throw in the additional SYNC you suggest.

  Ralf