Received-SPF: pass (google.com: domain of linux-kernel+bounces-135556-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: David Laight <David.Laight@ACULAB.COM>
To: 'Haris Okanovic' <harisokn@amazon.com>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "linux-pm@vger.kernel.org"
	<linux-pm@vger.kernel.org>, "linux-assembly@vger.kernel.org"
	<linux-assembly@vger.kernel.org>
CC: "peterz@infradead.org" <peterz@infradead.org>
Subject: RE: [PATCH 2/3] arm64: add __READ_ONCE_EX()
Thread-Topic: [PATCH 2/3] arm64: add __READ_ONCE_EX()
Thread-Index: AQHahJ/HcAF4vsIbZ0CGgbWFvfkyfLFefKGg
Date: Mon, 8 Apr 2024 14:51:13 +0000
Message-ID: <1b659ba3fa50482a8338b1a6942884cc@AcuMS.aculab.com>
References: <20240402014706.3969151-1-harisokn@amazon.com>
 <20240402014706.3969151-2-harisokn@amazon.com>
In-Reply-To: <20240402014706.3969151-2-harisokn@amazon.com>
Accept-Language: en-GB, en-US
Precedence: bulk
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 <harisokn@amazon.com>
> ---
>  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)