RE: [PATCH 2/3] arm64: add __READ_ONCE_EX()

From: Haris Okanovic
> Sent: 02 April 2024 02:47
>=20
> Perform an exclusive load, which atomically loads a word and arms the
> execusive monitor to enable wfe() polling of an address.
>=20
> Adding this macro in preparation for an arm64 cpuidle driver which
> supports a wfe() based polling state.
>=20
> https://developer.arm.com/documentation/dht0008/a/arm-synchronization-pri=
mitives/exclusive-
> accesses/exclusive-monitors
>=20
> Signed-off-by: Haris Okanovic <[email protected]>
> ---
>  arch/arm64/include/asm/readex.h | 46 +++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 arch/arm64/include/asm/readex.h
>=20
> diff --git a/arch/arm64/include/asm/readex.h b/arch/arm64/include/asm/rea=
dex.h
> new file mode 100644
> index 000000000000..51963c3107e1
> --- /dev/null
> +++ b/arch/arm64/include/asm/readex.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Based on arch/arm64/include/asm/rwonce.h
> + *
> + * Copyright (C) 2020 Google LLC.
> + * Copyright (C) 2024 Amazon.com, Inc. or its affiliates.
> + */
> +
> +#ifndef __ASM_READEX_H
> +#define __ASM_READEX_H
> +
> +#define __LOAD_EX(sfx, regs...) "ldaxr" #sfx "\t" #regs
> +
> +#define __READ_ONCE_EX(x)=09=09=09=09=09=09\
> +({=09=09=09=09=09=09=09=09=09\
> +=09typeof(&(x)) __x =3D &(x);=09=09=09=09=09\
> +=09int atomic =3D 1;=09=09=09=09=09=09=09\
> +=09union { __unqual_scalar_typeof(*__x) __val; char __c[1]; } __u;=09\
> +=09switch (sizeof(x)) {=09=09=09=09=09=09\
> +=09case 1:=09=09=09=09=09=09=09=09\
> +=09=09asm volatile(__LOAD_EX(b, %w0, %1)=09=09=09\
> +=09=09=09: "=3Dr" (*(__u8 *)__u.__c)=09=09=09\
> +=09=09=09: "Q" (*__x) : "memory");=09=09=09\
> +=09=09break;=09=09=09=09=09=09=09\
> +=09case 2:=09=09=09=09=09=09=09=09\
> +=09=09asm volatile(__LOAD_EX(h, %w0, %1)=09=09=09\
> +=09=09=09: "=3Dr" (*(__u16 *)__u.__c)=09=09=09\
> +=09=09=09: "Q" (*__x) : "memory");=09=09=09\
> +=09=09break;=09=09=09=09=09=09=09\
> +=09case 4:=09=09=09=09=09=09=09=09\
> +=09=09asm volatile(__LOAD_EX(, %w0, %1)=09=09=09\
> +=09=09=09: "=3Dr" (*(__u32 *)__u.__c)=09=09=09\
> +=09=09=09: "Q" (*__x) : "memory");=09=09=09\
> +=09=09break;=09=09=09=09=09=09=09\
> +=09case 8:=09=09=09=09=09=09=09=09\
> +=09=09asm volatile(__LOAD_EX(, %0, %1)=09=09=09\
> +=09=09=09: "=3Dr" (*(__u64 *)__u.__c)=09=09=09\
> +=09=09=09: "Q" (*__x) : "memory");=09=09=09\
> +=09=09break;=09=09=09=09=09=09=09\
> +=09default:=09=09=09=09=09=09=09\
> +=09=09atomic =3D 0;=09=09=09=09=09=09\
> +=09}=09=09=09=09=09=09=09=09\
> +=09atomic ? (typeof(*__x))__u.__val : (*(volatile typeof(__x))__x);\

I'm pretty sure that doesn't work the way you expect.
The ?: operator promotes 'unsigned char' to  'int'.
So you can fall foul of signedness tests (eg in min()).
It also isn't going to work for non-scalers.

Replacing the ?: with __builtin_choose_expr() may help.

(This is probably a bug in the code you copied.)

=09David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1=
PT, UK
Registration No: 1397386 (Wales)