On Fri, 26 Aug 2022, Linus Torvalds wrote:
> On Fri, Aug 26, 2022 at 12:23 PM Geert Uytterhoeven
> <geert@xxxxxxxxxxxxxx> wrote:
> >
> > include/asm-generic/bitops/non-instrumented-non-atomic.h:15:33:
> > error: implicit declaration of function 'arch_test_bit_acquire'; did
> > you mean '_test_bit_acquire'? [-Werror=implicit-function-declaration]
> >
>
> Ahh. m68k isn't using any of the generic bitops headers.
>
> *Most* architectures have that
>
> #include <asm-generic/bitops/non-atomic.h>
>
> and get it that way, but while it's common, it's most definitely not universal:
>
> [torvalds@ryzen linux]$ git grep -L bitops/non-atomic.h
> arch/*/include/asm/bitops.h
> arch/alpha/include/asm/bitops.h
> arch/hexagon/include/asm/bitops.h
> arch/ia64/include/asm/bitops.h
> arch/m68k/include/asm/bitops.h
> arch/s390/include/asm/bitops.h
> arch/sparc/include/asm/bitops.h
> arch/x86/include/asm/bitops.h
>
> and of that list only x86 has the new arch_test_bit_acquire().
>
> So I assume it's not just m68k, but also alpha, hexagon, ia64, s390
> and sparc that have this issue (unless they maybe have some other path
> that includes the gerneric ones, I didn't check).
For sparc, there is arch/sparc/include/asm/bitops_32.h and
arch/sparc/include/asm/bitops_64.h that include
asm-generic/bitops/non-atomic.h
For the others, the generic version is not included.
I'm wondering why do the architectures redefine test_bit, if their
definition is equivalent to the generic one? We could just delete
arch_test_bit and use "#define arch_test_bit generic_test_bit" as well.
> This was actually why my original suggested patch used the
> 'generic-non-atomic.h' header for it, because that is actually
> included regardless of any architecture headers directly from
> <linux/bitops.h>.
>
> And it never triggered for me that Mikulas' updated patch then had
> this arch_test_bit_acquire() issue.
>
> Something like the attached patch *MAY* fix it, but I really haven't
> thought about it a lot, and it's pretty ugly. Maybe it would be better
> to just add the
>
> #define arch_test_bit_acquire generic_test_bit_acquire
>
> to the affected <asm/bitops.h> files instead, and then let those
> architectures decide on their own that maybe they want to use their
> own test_bit() function because it is _already_ an acquire one.
>
> Mikulas?
>
> Geert - any opinions on that "maybe the arch should just do that
> #define itself"? I don't think it actually matters for m68k, you end
> up with pretty much the same thing anyway, because
> "smp_load_acquire()" is just a load anyway..
>
> Linus
Another untested patch ... tomorrow, I'll try to compile it, at least for
architectures where Debian provides cross-compiling gcc.
From: Mikulas Patocka <mpatocka@xxxxxxxxxx>
Some architectures define their own arch_test_bit and they also need
arch_test_bit_acquire, otherwise they won't compile. We also clean up the
code by using the generic test_bit if that is equivalent to the
arch-specific version.
Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Fixes: 8238b4579866 ("wait_on_bit: add an acquire memory barrier")
---
arch/alpha/include/asm/bitops.h | 7 ++-----
arch/hexagon/include/asm/bitops.h | 15 +++++++++++++++
arch/ia64/include/asm/bitops.h | 7 ++-----
arch/m68k/include/asm/bitops.h | 7 ++-----
arch/s390/include/asm/bitops.h | 10 ++--------
arch/sh/include/asm/bitops-op32.h | 12 ++----------
6 files changed, 25 insertions(+), 33 deletions(-)
Index: linux-2.6/arch/alpha/include/asm/bitops.h
===================================================================
--- linux-2.6.orig/arch/alpha/include/asm/bitops.h
+++ linux-2.6/arch/alpha/include/asm/bitops.h
@@ -283,11 +283,8 @@ arch___test_and_change_bit(unsigned long
return (old & mask) != 0;
}
-static __always_inline bool
-arch_test_bit(unsigned long nr, const volatile unsigned long *addr)
-{
- return (1UL & (((const int *) addr)[nr >> 5] >> (nr & 31))) != 0UL;
-}
+#define arch_test_bit generic_test_bit
+#define arch_test_bit_acquire generic_test_bit_acquire
/*
* ffz = Find First Zero in word. Undefined if no zero exists,
Index: linux-2.6/arch/hexagon/include/asm/bitops.h
===================================================================
--- linux-2.6.orig/arch/hexagon/include/asm/bitops.h
+++ linux-2.6/arch/hexagon/include/asm/bitops.h
@@ -179,6 +179,21 @@ arch_test_bit(unsigned long nr, const vo
return retval;
}
+static __always_inline bool
+arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)
+{
+ int retval;
+
+ asm volatile(
+ "{P0 = tstbit(%1,%2); if (P0.new) %0 = #1; if (!P0.new) %0 = #0;}\n"
+ : "=&r" (retval)
+ : "r" (addr[BIT_WORD(nr)]), "r" (nr % BITS_PER_LONG)
+ : "p0", "memory"
+ );
+
+ return retval;
+}
+
/*
* ffz - find first zero in word.
* @word: The word to search
Index: linux-2.6/arch/ia64/include/asm/bitops.h
===================================================================
--- linux-2.6.orig/arch/ia64/include/asm/bitops.h
+++ linux-2.6/arch/ia64/include/asm/bitops.h
@@ -331,11 +331,8 @@ arch___test_and_change_bit(unsigned long
return (old & bit) != 0;
}
-static __always_inline bool
-arch_test_bit(unsigned long nr, const volatile unsigned long *addr)
-{
- return 1 & (((const volatile __u32 *) addr)[nr >> 5] >> (nr & 31));
-}
+#define arch_test_bit generic_test_bit
+#define arch_test_bit_acquire generic_test_bit_acquire
/**
* ffz - find the first zero bit in a long word
Index: linux-2.6/arch/s390/include/asm/bitops.h
===================================================================
--- linux-2.6.orig/arch/s390/include/asm/bitops.h
+++ linux-2.6/arch/s390/include/asm/bitops.h
@@ -176,14 +176,8 @@ arch___test_and_change_bit(unsigned long
return old & mask;
}
-static __always_inline bool
-arch_test_bit(unsigned long nr, const volatile unsigned long *addr)
-{
- const volatile unsigned long *p = __bitops_word(nr, addr);
- unsigned long mask = __bitops_mask(nr);
-
- return *p & mask;
-}
+#define arch_test_bit generic_test_bit
+#define arch_test_bit_acquire generic_test_bit_acquire
static inline bool arch_test_and_set_bit_lock(unsigned long nr,
volatile unsigned long *ptr)
Index: linux-2.6/arch/m68k/include/asm/bitops.h
===================================================================
--- linux-2.6.orig/arch/m68k/include/asm/bitops.h
+++ linux-2.6/arch/m68k/include/asm/bitops.h
@@ -157,11 +157,8 @@ arch___change_bit(unsigned long nr, vola
change_bit(nr, addr);
}
-static __always_inline bool
-arch_test_bit(unsigned long nr, const volatile unsigned long *addr)
-{
- return (addr[nr >> 5] & (1UL << (nr & 31))) != 0;
-}
+#define arch_test_bit generic_test_bit
+#define arch_test_bit_acquire generic_test_bit_acquire
static inline int bset_reg_test_and_set_bit(int nr,
volatile unsigned long *vaddr)
Index: linux-2.6/arch/sh/include/asm/bitops-op32.h
===================================================================
--- linux-2.6.orig/arch/sh/include/asm/bitops-op32.h
+++ linux-2.6/arch/sh/include/asm/bitops-op32.h
@@ -135,16 +135,8 @@ arch___test_and_change_bit(unsigned long
return (old & mask) != 0;
}
-/**
- * arch_test_bit - Determine whether a bit is set
- * @nr: bit number to test
- * @addr: Address to start counting from
- */
-static __always_inline bool
-arch_test_bit(unsigned long nr, const volatile unsigned long *addr)
-{
- return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
-}
+#define arch_test_bit generic_test_bit
+#define arch_test_bit_acquire generic_test_bit_acquire
#include <asm-generic/bitops/non-instrumented-non-atomic.h>
