Re: [PATCH v2 (RESEND) 2/2] riscv: atomic: Using ARCH_ATOMIC in asm/atomic.h

From: Arnd Bergmann <arnd@arndb.de>
To: Guo Ren <guoren@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-csky@vger.kernel.org,
	linux-arch <linux-arch@vger.kernel.org>,
	Guo Ren <guoren@linux.alibaba.com>,
	Anup Patel <anup@brainfault.org>,
	Palmer Dabbelt <palmerdabbelt@google.com>
Subject: Re: [PATCH v2 (RESEND) 2/2] riscv: atomic: Using ARCH_ATOMIC in asm/atomic.h
Date: Mon, 19 Apr 2021 13:45:36 +0200	[thread overview]
Message-ID: <CAK8P3a1ygwS7jTXqYXCfppEEonCASqG_5GM9O_AtE9YgdgNqVQ@mail.gmail.com> (raw)
In-Reply-To: <1618634729-88821-2-git-send-email-guoren@kernel.org>

On Sat, Apr 17, 2021 at 6:45 AM <guoren@kernel.org> wrote:
> +#define arch_atomic_read(v)                    __READ_ONCE((v)->counter)
> +#define arch_atomic_set(v, i)                  __WRITE_ONCE(((v)->counter), (i))

> +#define ATOMIC64_INIT                          ATOMIC_INIT
> +#define arch_atomic64_read                     arch_atomic_read
> +#define arch_atomic64_set                      arch_atomic_set
>  #endif

I think it's a bit confusing to define arch_atomic64_read() etc in terms
of arch_atomic_read(), given that they operate on different types.

IMHO the clearest would be to define both in terms of the open-coded
version you have for the 32-bit atomics.

Also, given that all three architectures (x86, arm64, riscv) use the same
definitions for the six macros above, maybe those can just get moved
into a common file with a possible override?

x86 uses an inline function here instead of the macro. This would also
be my preference, but it may add complexity to avoid circular header
dependencies.

The rest of this patch looks good to me.

        Arnd