On Fri, 26 Aug 2022, Linus Torvalds wrote:

> On Fri, Aug 26, 2022 at 12:23 PM Geert Uytterhoeven
> <geert@linux-m68k.org> 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@redhat.com>

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@redhat.com>
Cc: stable@vger.kernel.org
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>

On Fri, Aug 26, 2022 at 1:43 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> 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.

I think generic_test_bit() came after many of them, and when it
didn't, people copied earlier architectures where they had already
done their own.

> Another untested patch ... tomorrow, I'll try to compile it, at least for
> architectures where Debian provides cross-compiling gcc.

Looks good to me, except I'd just do

#define arch_test_bit_acquire arch_test_bit

on hexagon rather than duplicate that function.

From my reading, Hexagon doesn't have any fancy memory ordering, it's
just the usual UP with barriers basically for instruction cache
coherence etc.

                 Linus

On Fri, 26 Aug 2022, Linus Torvalds wrote:

> On Fri, Aug 26, 2022 at 1:43 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > 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.
> 
> I think generic_test_bit() came after many of them, and when it
> didn't, people copied earlier architectures where they had already
> done their own.
> 
> > Another untested patch ... tomorrow, I'll try to compile it, at least for
> > architectures where Debian provides cross-compiling gcc.

I compile-tested this patch on alpha, s390x, m68k, sh, sparc32, sparc64. 
So, you can commit it to close these uncompilable-kernel reports.

Mikulas

On Sat, Aug 27, 2022 at 4:38 AM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> I compile-tested this patch on alpha, s390x, m68k, sh, sparc32, sparc64.
> So, you can commit it to close these uncompilable-kernel reports.

Thanks, done.

                Linus

On Fri, Aug 26, 2022 at 4:10 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Looks good to me, except I'd just do
>
> #define arch_test_bit_acquire arch_test_bit
>
> on hexagon rather than duplicate that function.

Oh, except you didn't quite duplicate it, you added the "memory"
clober to it to make sure it's ordered.

Which looks correct to me, even if the "almost entirely duplicated" is
a bit annoying.

                   Linus