Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-25 Thread Google
On Wed, 25 Oct 2023 15:16:16 +0200
Geert Uytterhoeven  wrote:

> Hi Adrian,
> 
> On Wed, Oct 25, 2023 at 12:32 PM John Paul Adrian Glaubitz
>  wrote:
> > On Wed, 2023-10-25 at 11:30 +0100, Mark Rutland wrote:
> > > On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote:
> > > > On Tue, 24 Oct 2023 16:08:12 +0100
> > > > Mark Rutland  wrote:
> > > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) 
> > > > > wrote:
> > > > > > From: Masami Hiramatsu (Google) 
> > > > > >
> > > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> > > > > > in SH architecture because it does not implement 
> > > > > > arch_cmpxchg_local().
> > > > >
> > > > > I do not think this is correct.
> > > > >
> > > > > The implementation in  is UP-only (and 
> > > > > it only
> > > > > disables interrupts), whereas arch/sh can be built SMP. We should 
> > > > > probably add
> > > > > some guards into  for that as we have in
> > > > > .
> > > >
> > > > Isn't cmpxchg_local for the data which only needs to ensure to do 
> > > > cmpxchg
> > > > on local CPU?
> > > > So I think it doesn't care about the other CPUs (IOW, it should not 
> > > > touched by
> > > > other CPUs), so it only considers UP case. E.g. on x86, 
> > > > arch_cmpxchg_local() is
> > > > defined as raw "cmpxchg" without lock prefix.
> > > >
> > > > #define __cmpxchg_local(ptr, old, new, size)
> > > > \
> > > > __raw_cmpxchg((ptr), (old), (new), (size), "")
> > > >
> > >
> > > Yes, you're right; sorry for the noise.
> > >
> > > For your original patch:
> > >
> > > Acked-by: Mark Rutland 
> >
> > Geert, what's your opinion on this?
> 
> While this looks OK on first sight (ARM includes the same file, even
> on SMP), it does not seem to work?
> 
> For sh-allnoconfig, as reported by kernel test robot:
> 
> $ make ARCH=sh CROSS_COMPILE=sh2-linux- allnoconfig lib/objpool.o
> lib/objpool.c: In function 'objpool_try_add_slot':
> ./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit
> declaration of function 'arch_cmpxchg_local'; did you mean
> 'raw_cmpxchg_local'? [-Werror=implicit-function-declaration]
>   384 | #define raw_cmpxchg_local arch_cmpxchg_local
>   |   ^~
> ./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in
> expansion of macro 'raw_cmpxchg_local'
>   392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \
>   |^
> ./include/linux/atomic/atomic-instrumented.h:4980:9: note: in
> expansion of macro 'raw_try_cmpxchg_local'
>  4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \
>   | ^
> lib/objpool.c:169:19: note: in expansion of macro 'try_cmpxchg_local'
>   169 | } while (!try_cmpxchg_local(>tail, , tail + 1));
>   |   ^
> 
> For an SMP defconfig:
> 
> $ make ARCH=sh CROSS_COMPILE=sh4-linux-gnu- sdk7786_defconfig lib/objpool.o
> 
> ./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit
> declaration of function ‘arch_cmpxchg_local’; did you mean
> ‘try_cmpxchg_local’? [-Werror=implicit-function-declaration]
>   384 | #define raw_cmpxchg_local arch_cmpxchg_local
>   |   ^~
> ./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in
> expansion of macro ‘raw_cmpxchg_local’
>   392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \
>   |^
> ./include/linux/atomic/atomic-instrumented.h:4980:9: note: in
> expansion of macro ‘raw_try_cmpxchg_local’
>  4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \
>   | ^
> lib/objpool.c:169:19: note: in expansion of macro ‘try_cmpxchg_local’
>   169 | } while (!try_cmpxchg_local(>tail, , tail + 1));
>   |   ^
> 
> Hiramatsu-san: do these build for you?

Thanks for pointing. I thought I just need to include the header file.
That's my fault.

Let me fix that.

Thank you!

> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds


-- 
Masami Hiramatsu (Google) 



Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-25 Thread Geert Uytterhoeven
Hi Adrian,

On Wed, Oct 25, 2023 at 12:32 PM John Paul Adrian Glaubitz
 wrote:
> On Wed, 2023-10-25 at 11:30 +0100, Mark Rutland wrote:
> > On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote:
> > > On Tue, 24 Oct 2023 16:08:12 +0100
> > > Mark Rutland  wrote:
> > > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) 
> > > > wrote:
> > > > > From: Masami Hiramatsu (Google) 
> > > > >
> > > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> > > > > in SH architecture because it does not implement arch_cmpxchg_local().
> > > >
> > > > I do not think this is correct.
> > > >
> > > > The implementation in  is UP-only (and it 
> > > > only
> > > > disables interrupts), whereas arch/sh can be built SMP. We should 
> > > > probably add
> > > > some guards into  for that as we have in
> > > > .
> > >
> > > Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg
> > > on local CPU?
> > > So I think it doesn't care about the other CPUs (IOW, it should not 
> > > touched by
> > > other CPUs), so it only considers UP case. E.g. on x86, 
> > > arch_cmpxchg_local() is
> > > defined as raw "cmpxchg" without lock prefix.
> > >
> > > #define __cmpxchg_local(ptr, old, new, size)\
> > > __raw_cmpxchg((ptr), (old), (new), (size), "")
> > >
> >
> > Yes, you're right; sorry for the noise.
> >
> > For your original patch:
> >
> > Acked-by: Mark Rutland 
>
> Geert, what's your opinion on this?

While this looks OK on first sight (ARM includes the same file, even
on SMP), it does not seem to work?

For sh-allnoconfig, as reported by kernel test robot:

$ make ARCH=sh CROSS_COMPILE=sh2-linux- allnoconfig lib/objpool.o
lib/objpool.c: In function 'objpool_try_add_slot':
./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit
declaration of function 'arch_cmpxchg_local'; did you mean
'raw_cmpxchg_local'? [-Werror=implicit-function-declaration]
  384 | #define raw_cmpxchg_local arch_cmpxchg_local
  |   ^~
./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in
expansion of macro 'raw_cmpxchg_local'
  392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \
  |^
./include/linux/atomic/atomic-instrumented.h:4980:9: note: in
expansion of macro 'raw_try_cmpxchg_local'
 4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \
  | ^
lib/objpool.c:169:19: note: in expansion of macro 'try_cmpxchg_local'
  169 | } while (!try_cmpxchg_local(>tail, , tail + 1));
  |   ^

For an SMP defconfig:

$ make ARCH=sh CROSS_COMPILE=sh4-linux-gnu- sdk7786_defconfig lib/objpool.o

./include/linux/atomic/atomic-arch-fallback.h:384:27: error: implicit
declaration of function ‘arch_cmpxchg_local’; did you mean
‘try_cmpxchg_local’? [-Werror=implicit-function-declaration]
  384 | #define raw_cmpxchg_local arch_cmpxchg_local
  |   ^~
./include/linux/atomic/atomic-arch-fallback.h:392:16: note: in
expansion of macro ‘raw_cmpxchg_local’
  392 | ___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \
  |^
./include/linux/atomic/atomic-instrumented.h:4980:9: note: in
expansion of macro ‘raw_try_cmpxchg_local’
 4980 | raw_try_cmpxchg_local(__ai_ptr, __ai_oldp, __VA_ARGS__); \
  | ^
lib/objpool.c:169:19: note: in expansion of macro ‘try_cmpxchg_local’
  169 | } while (!try_cmpxchg_local(>tail, , tail + 1));
  |   ^

Hiramatsu-san: do these build for you?

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds



Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-25 Thread wuqiang.matt

On 2023/10/25 09:51, wuqiang.matt wrote:

On 2023/10/25 07:42, Masami Hiramatsu (Google) wrote:

On Tue, 24 Oct 2023 16:08:12 +0100
Mark Rutland  wrote:


On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote:

From: Masami Hiramatsu (Google) 

Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
in SH architecture because it does not implement arch_cmpxchg_local().


I do not think this is correct.

The implementation in  is UP-only (and it only
disables interrupts), whereas arch/sh can be built SMP. We should probably add
some guards into  for that as we have in
.


Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg
on local CPU?


asm-generic/cmpxchg.h is only for UP, will throw an error for SMP building:

#ifdef CONFIG_SMP
#error "Cannot use generic cmpxchg on SMP"
#endif


Sorry that I just noticed Masami's patch has asm-generic/cmpxchg-local.h
included, not asm-generic/cmpxchg.h. cmpxchg.h does throw an error for SMP
configs, but cmpxchg-local.h doesn't.


SH arch seems it does have SMP systems. The arch/sh/include/asm/cmpxchg.h has
the following codes:

#if defined(CONFIG_GUSA_RB)
#include 
#elif defined(CONFIG_CPU_SH4A)
#include 
#elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP)
#include 
#else
#include 
#endif


So I think it doesn't care about the other CPUs (IOW, it should not touched by
other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() is
defined as raw "cmpxchg" without lock prefix.

#define __cmpxchg_local(ptr, old, new, size)    \
 __raw_cmpxchg((ptr), (old), (new), (size), "")


Thank you,




I think the right thing to do here is to define arch_cmpxchg_local() in terms
of arch_cmpxchg(), i.e. at the bottom of arch/sh's  add:

#define arch_cmpxchg_local  arch_cmpxchg


I agree too. Might not be performance optimized but guarantees correctness.


Mark.



Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/

Signed-off-by: Masami Hiramatsu (Google) 
---
  arch/sh/include/asm/cmpxchg.h |    2 ++
  1 file changed, 2 insertions(+)

diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
index 288f6f38d98f..e920e61fb817 100644
--- a/arch/sh/include/asm/cmpxchg.h
+++ b/arch/sh/include/asm/cmpxchg.h
@@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * 
ptr, unsigned long old,

  (unsigned long)_n_, sizeof(*(ptr))); \
    })
+#include 
+
  #endif /* __ASM_SH_CMPXCHG_H */











Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-25 Thread John Paul Adrian Glaubitz
On Wed, 2023-10-25 at 11:30 +0100, Mark Rutland wrote:
> On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote:
> > On Tue, 24 Oct 2023 16:08:12 +0100
> > Mark Rutland  wrote:
> > 
> > > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote:
> > > > From: Masami Hiramatsu (Google) 
> > > > 
> > > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> > > > in SH architecture because it does not implement arch_cmpxchg_local().
> > > 
> > > I do not think this is correct.
> > > 
> > > The implementation in  is UP-only (and it 
> > > only
> > > disables interrupts), whereas arch/sh can be built SMP. We should 
> > > probably add
> > > some guards into  for that as we have in
> > > .
> > 
> > Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg
> > on local CPU?
> > So I think it doesn't care about the other CPUs (IOW, it should not touched 
> > by
> > other CPUs), so it only considers UP case. E.g. on x86, 
> > arch_cmpxchg_local() is
> > defined as raw "cmpxchg" without lock prefix.
> > 
> > #define __cmpxchg_local(ptr, old, new, size)\
> > __raw_cmpxchg((ptr), (old), (new), (size), "")
> > 
> 
> Yes, you're right; sorry for the noise.
> 
> For your original patch:
> 
> Acked-by: Mark Rutland 
> 

Geert, what's your opinion on this?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913



Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-25 Thread Mark Rutland
On Wed, Oct 25, 2023 at 08:42:55AM +0900, Masami Hiramatsu wrote:
> On Tue, 24 Oct 2023 16:08:12 +0100
> Mark Rutland  wrote:
> 
> > On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote:
> > > From: Masami Hiramatsu (Google) 
> > > 
> > > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> > > in SH architecture because it does not implement arch_cmpxchg_local().
> > 
> > I do not think this is correct.
> > 
> > The implementation in  is UP-only (and it only
> > disables interrupts), whereas arch/sh can be built SMP. We should probably 
> > add
> > some guards into  for that as we have in
> > .
> 
> Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg
> on local CPU?
> So I think it doesn't care about the other CPUs (IOW, it should not touched by
> other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() 
> is
> defined as raw "cmpxchg" without lock prefix.
> 
> #define __cmpxchg_local(ptr, old, new, size)\
> __raw_cmpxchg((ptr), (old), (new), (size), "")
> 

Yes, you're right; sorry for the noise.

For your original patch:

Acked-by: Mark Rutland 

Mark.

> 
> Thank you,
> 
> 
> > 
> > I think the right thing to do here is to define arch_cmpxchg_local() in 
> > terms
> > of arch_cmpxchg(), i.e. at the bottom of arch/sh's  add:
> > 
> > #define arch_cmpxchg_local  arch_cmpxchg
> > 
> > Mark.
> > 
> > > 
> > > Reported-by: kernel test robot 
> > > Closes: 
> > > https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/
> > > Signed-off-by: Masami Hiramatsu (Google) 
> > > ---
> > >  arch/sh/include/asm/cmpxchg.h |2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
> > > index 288f6f38d98f..e920e61fb817 100644
> > > --- a/arch/sh/include/asm/cmpxchg.h
> > > +++ b/arch/sh/include/asm/cmpxchg.h
> > > @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * 
> > > ptr, unsigned long old,
> > >   (unsigned long)_n_, sizeof(*(ptr))); \
> > >})
> > >  
> > > +#include 
> > > +
> > >  #endif /* __ASM_SH_CMPXCHG_H */
> > > 
> 
> 
> -- 
> Masami Hiramatsu (Google) 



Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-24 Thread wuqiang.matt

On 2023/10/25 07:42, Masami Hiramatsu (Google) wrote:

On Tue, 24 Oct 2023 16:08:12 +0100
Mark Rutland  wrote:


On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote:

From: Masami Hiramatsu (Google) 

Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
in SH architecture because it does not implement arch_cmpxchg_local().


I do not think this is correct.

The implementation in  is UP-only (and it only
disables interrupts), whereas arch/sh can be built SMP. We should probably add
some guards into  for that as we have in
.


Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg
on local CPU?


asm-generic/cmpxchg.h is only for UP, will throw an error for SMP building:

#ifdef CONFIG_SMP
#error "Cannot use generic cmpxchg on SMP"
#endif

SH arch seems it does have SMP systems. The arch/sh/include/asm/cmpxchg.h has
the following codes:

#if defined(CONFIG_GUSA_RB)
#include 
#elif defined(CONFIG_CPU_SH4A)
#include 
#elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP)
#include 
#else
#include 
#endif


So I think it doesn't care about the other CPUs (IOW, it should not touched by
other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() is
defined as raw "cmpxchg" without lock prefix.

#define __cmpxchg_local(ptr, old, new, size)\
 __raw_cmpxchg((ptr), (old), (new), (size), "")


Thank you,




I think the right thing to do here is to define arch_cmpxchg_local() in terms
of arch_cmpxchg(), i.e. at the bottom of arch/sh's  add:

#define arch_cmpxchg_local  arch_cmpxchg


I agree too. Might not be performance optimized but guarantees correctness.


Mark.



Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/
Signed-off-by: Masami Hiramatsu (Google) 
---
  arch/sh/include/asm/cmpxchg.h |2 ++
  1 file changed, 2 insertions(+)

diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
index 288f6f38d98f..e920e61fb817 100644
--- a/arch/sh/include/asm/cmpxchg.h
+++ b/arch/sh/include/asm/cmpxchg.h
@@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, 
unsigned long old,
(unsigned long)_n_, sizeof(*(ptr))); \
})
  
+#include 

+
  #endif /* __ASM_SH_CMPXCHG_H */









Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-24 Thread Google
On Tue, 24 Oct 2023 16:08:12 +0100
Mark Rutland  wrote:

> On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) 
> > 
> > Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> > in SH architecture because it does not implement arch_cmpxchg_local().
> 
> I do not think this is correct.
> 
> The implementation in  is UP-only (and it only
> disables interrupts), whereas arch/sh can be built SMP. We should probably add
> some guards into  for that as we have in
> .

Isn't cmpxchg_local for the data which only needs to ensure to do cmpxchg
on local CPU?

So I think it doesn't care about the other CPUs (IOW, it should not touched by
other CPUs), so it only considers UP case. E.g. on x86, arch_cmpxchg_local() is
defined as raw "cmpxchg" without lock prefix.

#define __cmpxchg_local(ptr, old, new, size)\
__raw_cmpxchg((ptr), (old), (new), (size), "")


Thank you,


> 
> I think the right thing to do here is to define arch_cmpxchg_local() in terms
> of arch_cmpxchg(), i.e. at the bottom of arch/sh's  add:
> 
> #define arch_cmpxchg_local  arch_cmpxchg
> 
> Mark.
> 
> > 
> > Reported-by: kernel test robot 
> > Closes: 
> > https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/
> > Signed-off-by: Masami Hiramatsu (Google) 
> > ---
> >  arch/sh/include/asm/cmpxchg.h |2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
> > index 288f6f38d98f..e920e61fb817 100644
> > --- a/arch/sh/include/asm/cmpxchg.h
> > +++ b/arch/sh/include/asm/cmpxchg.h
> > @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * 
> > ptr, unsigned long old,
> > (unsigned long)_n_, sizeof(*(ptr))); \
> >})
> >  
> > +#include 
> > +
> >  #endif /* __ASM_SH_CMPXCHG_H */
> > 


-- 
Masami Hiramatsu (Google) 



Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-24 Thread John Paul Adrian Glaubitz
On Tue, 2023-10-24 at 23:52 +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) 
> 
> Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> in SH architecture because it does not implement arch_cmpxchg_local().
> 
> Reported-by: kernel test robot 
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/
> Signed-off-by: Masami Hiramatsu (Google) 
> ---
>  arch/sh/include/asm/cmpxchg.h |2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
> index 288f6f38d98f..e920e61fb817 100644
> --- a/arch/sh/include/asm/cmpxchg.h
> +++ b/arch/sh/include/asm/cmpxchg.h
> @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, 
> unsigned long old,
>   (unsigned long)_n_, sizeof(*(ptr))); \
>})
>  
> +#include 
> +
>  #endif /* __ASM_SH_CMPXCHG_H */

Reviewed-by: John Paul Adrian Glaubitz 

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913



Re: [PATCH] locking/atomic: sh: Use generic_cmpxchg_local for arch_cmpxchg_local()

2023-10-24 Thread Mark Rutland
On Tue, Oct 24, 2023 at 11:52:54PM +0900, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) 
> 
> Use generic_cmpxchg_local() for arch_cmpxchg_local() implementation
> in SH architecture because it does not implement arch_cmpxchg_local().

I do not think this is correct.

The implementation in  is UP-only (and it only
disables interrupts), whereas arch/sh can be built SMP. We should probably add
some guards into  for that as we have in
.

I think the right thing to do here is to define arch_cmpxchg_local() in terms
of arch_cmpxchg(), i.e. at the bottom of arch/sh's  add:

#define arch_cmpxchg_local  arch_cmpxchg

Mark.

> 
> Reported-by: kernel test robot 
> Closes: 
> https://lore.kernel.org/oe-kbuild-all/202310241310.ir5uukog-...@intel.com/
> Signed-off-by: Masami Hiramatsu (Google) 
> ---
>  arch/sh/include/asm/cmpxchg.h |2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/sh/include/asm/cmpxchg.h b/arch/sh/include/asm/cmpxchg.h
> index 288f6f38d98f..e920e61fb817 100644
> --- a/arch/sh/include/asm/cmpxchg.h
> +++ b/arch/sh/include/asm/cmpxchg.h
> @@ -71,4 +71,6 @@ static inline unsigned long __cmpxchg(volatile void * ptr, 
> unsigned long old,
>   (unsigned long)_n_, sizeof(*(ptr))); \
>})
>  
> +#include 
> +
>  #endif /* __ASM_SH_CMPXCHG_H */
>