1. Patchew
  2. linux
  3. View series

[PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg

Bibo Mao posted 1 patch 1 year, 5 months ago 
include/linux/atomic/atomic-arch-fallback.h | 50 ++++++++++++++-------
scripts/atomic/gen-atomic-fallback.sh       |  3 +-
2 files changed, 35 insertions(+), 18 deletions(-)
[PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
Posted by Bibo Mao 1 year, 5 months ago 
When porting pv spinlock function on LoongArch system, there is
compiling error such as:
                 from linux/include/linux/smp.h:13,
                 from linux/kernel/locking/qspinlock.c:16:
linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
  242 |         typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
      |                                  ^
linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
 4908 |         raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
      |         ^~~~~~~~~~~~~~~~~~~~~~~
linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
  377 |         if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
      |              ^~~~~~~~~~~~~~~~~~~
In file included from linux/kernel/locking/qspinlock.c:583:
linux/kernel/locking/qspinlock_paravirt.h: At top level:
linux/kernel/locking/qspinlock_paravirt.h:499:1: warning: no previous prototype for '__pv_queued_spin_unlock_slowpath' [-Wmissing-prototypes]
  499 | __pv_queued_spin_unlock_slowpath(struct qspinlock *lock, u8 locked)

Macro try_cmpxchg_relaxed() is used by paravirt qspinlock, on LoongArch
it is defined as raw_try_cmpxchg_relaxed(). And there is different type
conversion in marco raw_try_cmpxchg_relaxed(). Here type declaration is
added beore type conversion.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 include/linux/atomic/atomic-arch-fallback.h | 50 ++++++++++++++-------
 scripts/atomic/gen-atomic-fallback.sh       |  3 +-
 2 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/include/linux/atomic/atomic-arch-fallback.h b/include/linux/atomic/atomic-arch-fallback.h
index 2f9d36b72bd8..8273c53321c3 100644
--- a/include/linux/atomic/atomic-arch-fallback.h
+++ b/include/linux/atomic/atomic-arch-fallback.h
@@ -188,7 +188,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -206,7 +207,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg_acquire(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg_acquire((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -224,7 +226,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg_release(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg_release((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -239,7 +242,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg_relaxed(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg_relaxed((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -255,7 +259,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg64(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg64((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -273,7 +278,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg64_acquire(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg64_acquire((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -291,7 +297,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg64_release(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg64_release((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -306,7 +313,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg64_relaxed(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg64_relaxed((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -322,7 +330,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg128(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg128((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -340,7 +349,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg128_acquire(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg128_acquire((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -358,7 +368,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg128_release(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg128_release((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -373,7 +384,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg128_relaxed(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg128_relaxed((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -388,7 +400,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg_local(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg_local((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -403,7 +416,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg64_local(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg64_local((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -418,7 +432,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_try_cmpxchg128_local(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_cmpxchg128_local((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -433,7 +448,8 @@ extern void raw_cmpxchg128_relaxed_not_implemented(void);
 #else
 #define raw_sync_try_cmpxchg(_ptr, _oldp, _new) \
 ({ \
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \
+	typeof(*(_ptr)) ___o = *___op, ___r; \
 	___r = raw_sync_cmpxchg((_ptr), ___o, (_new)); \
 	if (unlikely(___r != ___o)) \
 		*___op = ___r; \
@@ -4690,4 +4706,4 @@ raw_atomic64_dec_if_positive(atomic64_t *v)
 }
 
 #endif /* _LINUX_ATOMIC_FALLBACK_H */
-// b565db590afeeff0d7c9485ccbca5bb6e155749f
+// 7b23ddca3c50c5869e68ded50748ffe111123156
diff --git a/scripts/atomic/gen-atomic-fallback.sh b/scripts/atomic/gen-atomic-fallback.sh
index f80d69cfeb1f..f3a9b084a176 100755
--- a/scripts/atomic/gen-atomic-fallback.sh
+++ b/scripts/atomic/gen-atomic-fallback.sh
@@ -230,7 +230,8 @@ gen_try_cmpxchg_fallback()
 cat <<EOF
 #define raw_${prefix}try_${cmpxchg}${suffix}(_ptr, _oldp, _new) \\
 ({ \\
-	typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \\
+	typeof((_ptr)) ___op = (typeof((_ptr)))(_oldp); \\
+	typeof(*(_ptr)) ___o = *___op, ___r; \\
 	___r = raw_${prefix}${cmpxchg}${suffix}((_ptr), ___o, (_new)); \\
 	if (unlikely(___r != ___o)) \\
 		*___op = ___r; \\

base-commit: 720261cfc7329406a50c2a8536e0039b9dd9a4e5
-- 
2.39.3
Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
Posted by Uros Bizjak 1 year, 4 months ago 
On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
> When porting pv spinlock function on LoongArch system, there is
> compiling error such as:
>                  from linux/include/linux/smp.h:13,
>                  from linux/kernel/locking/qspinlock.c:16:
> linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
>   242 |         typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
>       |                                  ^
> linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
>  4908 |         raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
>       |         ^~~~~~~~~~~~~~~~~~~~~~~
> linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
>   377 |         if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))

This points to the mismatch between "pn->state" and "old" variable.
The correct fix is:

--cut here--
diff --git a/kernel/locking/qspinlock_paravirt.h
b/kernel/locking/qspinlock_paravirt.h
index f5a36e67b593..ac2e22502741 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
*node, struct mcs_spinlock *prev)
static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
{
       struct pv_node *pn = (struct pv_node *)node;
-       enum vcpu_state old = vcpu_halted;
+       u8 old = vcpu_halted;
       /*
        * If the vCPU is indeed halted, advance its state to match that of
        * pv_wait_node(). If OTOH this fails, the vCPU was running and will
--cut here--

Uros.
Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
Posted by Boqun Feng 1 year, 4 months ago 
On Fri, Jul 19, 2024 at 12:15:28PM +0200, Uros Bizjak wrote:
> On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@loongson.cn> wrote:
> >
> > When porting pv spinlock function on LoongArch system, there is
> > compiling error such as:
> >                  from linux/include/linux/smp.h:13,
> >                  from linux/kernel/locking/qspinlock.c:16:
> > linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> > linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
> >   242 |         typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
> >       |                                  ^
> > linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
> >  4908 |         raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~
> > linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
> >   377 |         if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
> 
> This points to the mismatch between "pn->state" and "old" variable.
> The correct fix is:
> 
> --cut here--
> diff --git a/kernel/locking/qspinlock_paravirt.h
> b/kernel/locking/qspinlock_paravirt.h
> index f5a36e67b593..ac2e22502741 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
> *node, struct mcs_spinlock *prev)
> static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> {
>        struct pv_node *pn = (struct pv_node *)node;
> -       enum vcpu_state old = vcpu_halted;
> +       u8 old = vcpu_halted;
>        /*

Looks reasonable to me, we should also add static_assert() for
try_cmpxhg_*() to make sure the old has the same size of the cmpxchged
field.

Regards,
Boqun

>         * If the vCPU is indeed halted, advance its state to match that of
>         * pv_wait_node(). If OTOH this fails, the vCPU was running and will
> --cut here--
> 
> Uros.
Re: [PATCH] locking/atomic: scripts: Fix type error in macro try_cmpxchg
Posted by Uros Bizjak 1 year, 4 months ago 
On Fri, Jul 19, 2024 at 6:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Fri, Jul 19, 2024 at 12:15:28PM +0200, Uros Bizjak wrote:
> > On Fri, Jul 19, 2024 at 4:40 AM Bibo Mao <maobibo@loongson.cn> wrote:
> > >
> > > When porting pv spinlock function on LoongArch system, there is
> > > compiling error such as:
> > >                  from linux/include/linux/smp.h:13,
> > >                  from linux/kernel/locking/qspinlock.c:16:
> > > linux/kernel/locking/qspinlock_paravirt.h: In function 'pv_kick_node':
> > > linux/include/linux/atomic/atomic-arch-fallback.h:242:34: error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *' [-Wincompatible-pointer-types]
> > >   242 |         typeof(*(_ptr)) *___op = (_oldp), ___o = *___op, ___r; \
> > >       |                                  ^
> > > linux/atomic/atomic-instrumented.h:4908:9: note: in expansion of macro 'raw_try_cmpxchg_relaxed'
> > >  4908 |         raw_try_cmpxchg_relaxed(__ai_ptr, __ai_oldp, __VA_ARGS__); \
> > >       |         ^~~~~~~~~~~~~~~~~~~~~~~
> > > linux/kernel/locking/qspinlock_paravirt.h:377:14: note: in expansion of macro 'try_cmpxchg_relaxed'
> > >   377 |         if (!try_cmpxchg_relaxed(&pn->state, &old, vcpu_hashed))
> >
> > This points to the mismatch between "pn->state" and "old" variable.
> > The correct fix is:
> >
> > --cut here--
> > diff --git a/kernel/locking/qspinlock_paravirt.h
> > b/kernel/locking/qspinlock_paravirt.h
> > index f5a36e67b593..ac2e22502741 100644
> > --- a/kernel/locking/qspinlock_paravirt.h
> > +++ b/kernel/locking/qspinlock_paravirt.h
> > @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
> > *node, struct mcs_spinlock *prev)
> > static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> > {
> >        struct pv_node *pn = (struct pv_node *)node;
> > -       enum vcpu_state old = vcpu_halted;
> > +       u8 old = vcpu_halted;
> >        /*
>
> Looks reasonable to me, we should also add static_assert() for
> try_cmpxhg_*() to make sure the old has the same size of the cmpxchged
> field.

This is what -Wincompatible-pointer-types should detect, and it does
for the LoongArch target. Apparently, x86_64 targets use
-fshort-enums, so the mismatch was not detected on this particular
target.

Bibo Mao, does the proposed change works for you, so I can propose a
formal patch submission with the fix?

Thanks,
Uros.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.