Re: cmpxchg broken in some situation

[Thiemo Seufer <ths@networkno.de>]

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