This was reported by the kernel test bot on an architecture I can't really test on. I was only able to make sure the build succeeds, but nothing more.
This patch is based on the __cmpxchg_u32 impletmentation and seems incomplete based on the different cmpxchg headers I can find.

Do these function need to be impletmented in each header simulataneously?

Thanks,
Liam

 arch/sh/include/asm/cmpxchg-irq.h | 27 +++++++++++++++++++++++++++
 arch/sh/include/asm/cmpxchg.h     |  5 +++--
 2 files changed, 30 insertions(+), 2 deletions(-)

arch/sh/include/asm/cmpxchg-irq.h +27 -0

index 07d3e7f08389..918c4153a930 100644
@@ -51,4 +51,31 @@ static inline unsigned long __cmpxchg_u32(volatile int *m, unsigned long old,
 	return retval;
 }
 
+static inline unsigned long __cmpxchg_u16(volatile u16 *m, unsigned long old,
+	unsigned long new)
+{
+	u16 retval;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	retval = *m;
+	if (retval = old)
+		*m = new;
+	local_irq_restore(flags);
+	return (unsigned long)retval;
+}
+
+static inline unsigned long __cmpxchg_u8(volatile u8 *m, unsigned long old,
+	unsigned long new)
+{
+	u8 retval;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	retval = *m;
+	if (retval = old)
+		*m = new;
+	local_irq_restore(flags);
+	return (unsigned long)retval;
+}
 #endif /* __ASM_SH_CMPXCHG_IRQ_H */

arch/sh/include/asm/cmpxchg.h +3 -2

index e9501d85c278..7d65d0fd1665 100644
@@ -56,8 +56,9 @@ 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 4: return __cmpxchg_u32((int *)ptr, old, new);
+	case 2: return __cmpxchg_u16((u16 *)ptr, old, new);
+	case 1: return __cmpxchg_u8((u8 *)ptr, old, new);
 	}
 	__cmpxchg_called_with_bad_pointer();
 	return old;

Signature · 1 line

2.27.0

Geert Uytterhoeven

2020-08-19 09:09 UTC #1 lore raw

Hi Liam,

Quoted 13 lines · Liam Beguin

The kernel test bot reported[1] that using set_mask_bits on a u8 causes the following issue on SuperH:
Quoted 1 line
Quoted 1 line
ERROR: modpost: "__cmpxchg_called_with_bad_pointer" [drivers/phy/ti/phy-tusb1210.ko] undefined!
Add support for cmpxchg on u8 and u16 pointers.
[1] https://lore.kernel.org/patchwork/patch/1288894/#1485536
Reported-by kernel test robot <[email protected]>
Signed-off-by Liam Beguin <[email protected]>
---

Quoted 7 lines

Hi,
This was reported by the kernel test bot on an architecture I can't really test on. I was only able to make sure the build succeeds, but nothing more.
This patch is based on the __cmpxchg_u32 impletmentation and seems incomplete based on the different cmpxchg headers I can find.

Indeed. This version is suitable for non-SMP machines only. BTW, it looks like this version can be replaced by the one in asm-generic?

Quoted 3 lines

Do these function need to be impletmented in each header simulataneously?

Yes, we need them for all variants.

Quoted 46 lines

 arch/sh/include/asm/cmpxchg-irq.h | 27 +++++++++++++++++++++++++++
 arch/sh/include/asm/cmpxchg.h     |  5 +++--
 2 files changed, 30 insertions(+), 2 deletions(-)

arch/sh/include/asm/cmpxchg-irq.h +27 -0

index 07d3e7f08389..918c4153a930 100644
@@ -51,4 +51,31 @@ static inline unsigned long __cmpxchg_u32(volatile int *m, unsigned long old,
        return retval;
 }
 
+static inline unsigned long __cmpxchg_u16(volatile u16 *m, unsigned long old,
+       unsigned long new)
+{
+       u16 retval;
+       unsigned long flags;
+
+       local_irq_save(flags);
+       retval = *m;
+       if (retval = old)
+               *m = new;
+       local_irq_restore(flags);
+       return (unsigned long)retval;
+}
+
+static inline unsigned long __cmpxchg_u8(volatile u8 *m, unsigned long old,
+       unsigned long new)
+{
+       u8 retval;
+       unsigned long flags;
+
+       local_irq_save(flags);
+       retval = *m;
+       if (retval = old)
+               *m = new;
+       local_irq_restore(flags);
+       return (unsigned long)retval;
+}
 #endif /* __ASM_SH_CMPXCHG_IRQ_H */

arch/sh/include/asm/cmpxchg.h +0 -0

index e9501d85c278..7d65d0fd1665 100644
@@ -56,8 +56,9 @@ static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old,
                unsigned long new, int size)

arch/sh/include/asm/cmpxchg.h static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old, · line 64

Quoted 10 lines

arch/sh/include/asm/cmpxchg.h +3 -2

 {
        switch (size) {
-       case 4:
-               return __cmpxchg_u32(ptr, old, new);
+       case 4: return __cmpxchg_u32((int *)ptr, old, new);
+       case 2: return __cmpxchg_u16((u16 *)ptr, old, new);
+       case 1: return __cmpxchg_u8((u8 *)ptr, old, new);
        }
        __cmpxchg_called_with_bad_pointer();
        return old;

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Liam Beguin

author

2020-08-19 13:23 UTC #2 lore raw

Hi Geert,

Quoted 25 lines · Geert Uytterhoeven

Hi Liam,
Quoted 13 lines · Liam Beguin
The kernel test bot reported[1] that using set_mask_bits on a u8 causes the following issue on SuperH:
Quoted 1 line
Quoted 1 line
ERROR: modpost: "__cmpxchg_called_with_bad_pointer" [drivers/phy/ti/phy-tusb1210.ko] undefined!
Add support for cmpxchg on u8 and u16 pointers.
[1] https://lore.kernel.org/patchwork/patch/1288894/#1485536
Reported-by kernel test robot <[email protected]>
Signed-off-by Liam Beguin <[email protected]>
---
Quoted 7 lines
Hi,
This was reported by the kernel test bot on an architecture I can't really test on. I was only able to make sure the build succeeds, but nothing more.
This patch is based on the __cmpxchg_u32 impletmentation and seems incomplete based on the different cmpxchg headers I can find.

Quoted 4 lines

Indeed. This version is suitable for non-SMP machines only. BTW, it looks like this version can be replaced by the one in asm-generic?

Thanks for your feedback I'll have a look at the asm-generic functions and try to use those instead.

Quoted 6 lines

Quoted 3 lines
Do these function need to be impletmented in each header simulataneously?
Yes, we need them for all variants.

Okay, I'll look into that. Would you recommend a good way to test these changes?

Quoted 65 lines

Quoted 46 lines

 arch/sh/include/asm/cmpxchg-irq.h | 27 +++++++++++++++++++++++++++
 arch/sh/include/asm/cmpxchg.h     |  5 +++--
 2 files changed, 30 insertions(+), 2 deletions(-)

arch/sh/include/asm/cmpxchg-irq.h +27 -0

index 07d3e7f08389..918c4153a930 100644
@@ -51,4 +51,31 @@ static inline unsigned long __cmpxchg_u32(volatile int *m, unsigned long old,
        return retval;
 }
 
+static inline unsigned long __cmpxchg_u16(volatile u16 *m, unsigned long old,
+       unsigned long new)
+{
+       u16 retval;
+       unsigned long flags;
+
+       local_irq_save(flags);
+       retval = *m;
+       if (retval == old)
+               *m = new;
+       local_irq_restore(flags);
+       return (unsigned long)retval;
+}
+
+static inline unsigned long __cmpxchg_u8(volatile u8 *m, unsigned long old,
+       unsigned long new)
+{
+       u8 retval;
+       unsigned long flags;
+
+       local_irq_save(flags);
+       retval = *m;
+       if (retval == old)
+               *m = new;
+       local_irq_restore(flags);
+       return (unsigned long)retval;
+}
 #endif /* __ASM_SH_CMPXCHG_IRQ_H */

arch/sh/include/asm/cmpxchg.h +0 -0

index e9501d85c278..7d65d0fd1665 100644
@@ -56,8 +56,9 @@ static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old,
                unsigned long new, int size)

arch/sh/include/asm/cmpxchg.h static inline unsigned long __cmpxchg(volatile void * ptr, unsigned long old, · line 64

Quoted 10 lines

arch/sh/include/asm/cmpxchg.h +3 -2

 {
        switch (size) {
-       case 4:
-               return __cmpxchg_u32(ptr, old, new);
+       case 4: return __cmpxchg_u32((int *)ptr, old, new);
+       case 2: return __cmpxchg_u16((u16 *)ptr, old, new);
+       case 1: return __cmpxchg_u8((u8 *)ptr, old, new);
        }
        __cmpxchg_called_with_bad_pointer();
        return old;

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

Quoted 5 lines

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

Thanks for your time,
Liam

Geert Uytterhoeven

2020-08-19 13:50 UTC #3 lore raw

Hi Liam,

Quoted 33 lines · Liam Beguin

Quoted 23 lines · Geert Uytterhoeven
Quoted 13 lines · Liam Beguin
The kernel test bot reported[1] that using set_mask_bits on a u8 causes the following issue on SuperH:
Quoted 1 line
Quoted 1 line
ERROR: modpost: "__cmpxchg_called_with_bad_pointer" [drivers/phy/ti/phy-tusb1210.ko] undefined!
Add support for cmpxchg on u8 and u16 pointers.
[1] https://lore.kernel.org/patchwork/patch/1288894/#1485536
Reported-by kernel test robot <[email protected]>
Signed-off-by Liam Beguin <[email protected]>
---
Quoted 7 lines
Hi,
This was reported by the kernel test bot on an architecture I can't really test on. I was only able to make sure the build succeeds, but nothing more.
This patch is based on the __cmpxchg_u32 impletmentation and seems incomplete based on the different cmpxchg headers I can find.
Quoted 4 lines
Indeed. This version is suitable for non-SMP machines only. BTW, it looks like this version can be replaced by the one in asm-generic?
Thanks for your feedback I'll have a look at the asm-generic functions and try to use those instead.
Quoted 1 line
Quoted 1 line

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Do these function need to be impletmented in each header simulataneously?
Yes, we need them for all variants.
Okay, I'll look into that. Would you recommend a good way to test these changes?

That's gonna be harder, I'm afraid.
Who has suitable hardware?

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Rob Landley

2020-08-21 20:46 UTC #4 lore raw

In reply to Liam Beguin

Quoted 34 lines · Geert Uytterhoeven

Hi Liam,
Quoted 31 lines · Liam Beguin
Quoted 23 lines · Geert Uytterhoeven
Quoted 13 lines · Liam Beguin
The kernel test bot reported[1] that using set_mask_bits on a u8 causes the following issue on SuperH:
Quoted 1 line
Quoted 1 line
ERROR: modpost: "__cmpxchg_called_with_bad_pointer" [drivers/phy/ti/phy-tusb1210.ko] undefined!
Add support for cmpxchg on u8 and u16 pointers.
[1] https://lore.kernel.org/patchwork/patch/1288894/#1485536
Reported-by kernel test robot <[email protected]>
Signed-off-by Liam Beguin <[email protected]>
---
Quoted 7 lines
Hi,
This was reported by the kernel test bot on an architecture I can't really test on. I was only able to make sure the build succeeds, but nothing more.
This patch is based on the __cmpxchg_u32 impletmentation and seems incomplete based on the different cmpxchg headers I can find.
Quoted 4 lines
Indeed. This version is suitable for non-SMP machines only. BTW, it looks like this version can be replaced by the one in asm-generic?
Thanks for your feedback I'll have a look at the asm-generic functions and try to use those instead.

Does using the asm-generic one automatically use the architecture version for j-core? (Technically we're compare-and-swap rather than cmpxchg but eh, close
enough: CAS.L Rm, Rn, @R0 opcode 0010-nnnn-mmmm-0011, based on the IBM 360 instruction. Not sure how the #include plumbing winds up selecting the version here, if there's an extra thing we have to do we should do it.)

Quoted 5 lines

Quoted 2 lines
Okay, I'll look into that. Would you recommend a good way to test these changes?
That's gonna be harder, I'm afraid.
Who has suitable hardware?

Define suitable? (Not familiar with testbot? In addition to a raspberry pi form factor j-core board that runs off my laptop's usb power pretty much with me at all times, I have a johnson controls sh4 board in a box, a little blue board that runs an st kernel fork in the same box, and an sh2 board from the dawn of time in another box. I generally use the j-core board as my hardware and regression test sh4 on qemu unless I'm checking a specific hardware thing.)

Rob

Geert Uytterhoeven

2020-08-23 08:47 UTC #5 lore raw

In reply to Liam Beguin

Hi Rob,

Quoted 28 lines · Rob Landley

Quoted 27 lines · Geert Uytterhoeven
Quoted 26 lines · Liam Beguin
Quoted 24 lines · Geert Uytterhoeven
Quoted 9 lines · Liam Beguin
The kernel test bot reported[1] that using set_mask_bits on a u8 causes the following issue on SuperH:
Quoted 1 line
Quoted 1 line
ERROR: modpost: "__cmpxchg_called_with_bad_pointer" [drivers/phy/ti/phy-tusb1210.ko] undefined!
Add support for cmpxchg on u8 and u16 pointers.
[1] https://lore.kernel.org/patchwork/patch/1288894/#1485536
Quoted 8 lines
Reported-by kernel test robot <[email protected]>
Signed-off-by Liam Beguin <[email protected]>
This was reported by the kernel test bot on an architecture I can't really test on. I was only able to make sure the build succeeds, but nothing more.
This patch is based on the __cmpxchg_u32 impletmentation and seems incomplete based on the different cmpxchg headers I can find.
Indeed. This version is suitable for non-SMP machines only. BTW, it looks like this version can be replaced by the one in asm-generic?

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
Thanks for your feedback I'll have a look at the asm-generic functions and try to use those instead.
Does using the asm-generic one automatically use the architecture version for j-core? (Technically we're compare-and-swap rather than cmpxchg but eh, close
enough: CAS.L Rm, Rn, @R0 opcode 0010-nnnn-mmmm-0011, based on the IBM 360 instruction. Not sure how the #include plumbing winds up selecting the version here, if there's an extra thing we have to do we should do it.)

No, the asm-generic is only suitable for arch/sh/include/asm/cmpxchg-irq.h. There are 3 different implementations needed to cover all, cfr. below.

Quoted 6 lines

Quoted 5 lines
Quoted 2 lines
Okay, I'll look into that. Would you recommend a good way to test these changes?
That's gonna be harder, I'm afraid.
Who has suitable hardware?

Quoted 6 lines

Define suitable? (Not familiar with testbot? In addition to a raspberry pi form factor j-core board that runs off my laptop's usb power pretty much with me at all times, I have a johnson controls sh4 board in a box, a little blue board that runs an st kernel fork in the same box, and an sh2 board from the dawn of time in another box. I generally use the j-core board as my hardware and regression test sh4 on qemu unless I'm checking a specific hardware thing.)

arch/sh/include/asm/cmpxchg.h:

    #if defined(CONFIG_GUSA_RB)
    #include <asm/cmpxchg-grb.h>
    #elif defined(CONFIG_CPU_SH4A)
    #include <asm/cmpxchg-llsc.h>
    #elif defined(CONFIG_CPU_J2) && defined(CONFIG_SMP)
    #include <asm/cmpxchg-cas.h>
    #else
    #include <asm/cmpxchg-irq.h>

#endif

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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