Re: [RFC PATCH 1/5] RISC-V: hwprobe: Clarify cpus size parameter

Palmer Dabbelt
2023-09-25T11:23:48Z

On Thu, 21 Sep 2023 05:55:20 PDT (-0700), ajones@ventanamicro.com wrote:
> The "count" parameter associated with the 'cpus' parameter of the
> hwprobe syscall is the size in bytes of 'cpus'. Naming it 'cpu_count'
> may mislead users (it did me) to think it's the number of CPUs that
> are or can be represented by 'cpus' instead. This is particularly
> easy (IMO) to get wrong since 'cpus' is documented to be defined by
> CPU_SET(3) and CPU_SET(3) also documents a CPU_COUNT() (the number
> of CPUs in set) macro. CPU_SET(3) refers to the size of cpu sets
> with 'setsize'. Adopt 'cpusetsize' for the hwprobe parameter and
> specifically state it is in bytes in Documentation/riscv/hwprobe.rst
> to clarify.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> Documentation/riscv/hwprobe.rst | 15 ++++++++-------
> arch/riscv/kernel/sys_riscv.c | 14 +++++++-------
> arch/riscv/kernel/vdso/hwprobe.c | 10 +++++-----
> 3 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/riscv/hwprobe.rst b/Documentation/riscv/hwprobe.rst
> index 7b2384de471f..132e9acaa8f4 100644
> --- a/Documentation/riscv/hwprobe.rst
> +++ b/Documentation/riscv/hwprobe.rst
> @@ -12,7 +12,7 @@ is defined in <asm/hwprobe.h>::
> };
>
> long sys_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> - size_t cpu_count, cpu_set_t *cpus,
> + size_t cpusetsize, cpu_set_t *cpus,
> unsigned int flags);
>
> The arguments are split into three groups: an array of key-value pairs, a CPU
> @@ -20,12 +20,13 @@ set, and some flags. The key-value pairs are supplied with a count. Userspace
> must prepopulate the key field for each element, and the kernel will fill in the
> value if the key is recognized. If a key is unknown to the kernel, its key field
> will be cleared to -1, and its value set to 0. The CPU set is defined by
> -CPU_SET(3). For value-like keys (eg. vendor/arch/impl), the returned value will
> -be only be valid if all CPUs in the given set have the same value. Otherwise -1
> -will be returned. For boolean-like keys, the value returned will be a logical
> -AND of the values for the specified CPUs. Usermode can supply NULL for cpus and
> -0 for cpu_count as a shortcut for all online CPUs. There are currently no flags,
> -this value must be zero for future compatibility.
> +CPU_SET(3) with size ``cpusetsize`` bytes. For value-like keys (eg. vendor,

So the only difference is just "with size ``cpusetsize`` bytes" and a
bunch of line wrap changes? That always makes these doc updates hard to
read, as I go a bit cross-eyed trying to diff it manually. I don't know
of a better way to do it, though.

> +arch, impl), the returned value will only be valid if all CPUs in the given set
> +have the same value. Otherwise -1 will be returned. For boolean-like keys, the
> +value returned will be a logical AND of the values for the specified CPUs.
> +Usermode can supply NULL for ``cpus`` and 0 for ``cpusetsize`` as a shortcut for
> +all online CPUs. There are currently no flags, this value must be zero for
> +future compatibility.
>
> On success 0 is returned, on failure a negative error code is returned.
>
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> index d7266a9abc66..14b6dfaa5d9f 100644
> --- a/arch/riscv/kernel/sys_riscv.c
> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -246,7 +246,7 @@ static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> }
>
> static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> - size_t pair_count, size_t cpu_count,
> + size_t pair_count, size_t cpusetsize,
> unsigned long __user *cpus_user,
> unsigned int flags)
> {
> @@ -264,13 +264,13 @@ static int do_riscv_hwprobe(struct riscv_hwprobe __user *pairs,
> * 0 as a shortcut to all online CPUs.
> */
> cpumask_clear(&cpus);
> - if (!cpu_count && !cpus_user) {
> + if (!cpusetsize && !cpus_user) {
> cpumask_copy(&cpus, cpu_online_mask);
> } else {
> - if (cpu_count > cpumask_size())
> - cpu_count = cpumask_size();
> + if (cpusetsize > cpumask_size())
> + cpusetsize = cpumask_size();
>
> - ret = copy_from_user(&cpus, cpus_user, cpu_count);
> + ret = copy_from_user(&cpus, cpus_user, cpusetsize);
> if (ret)
> return -EFAULT;
>
> @@ -347,10 +347,10 @@ arch_initcall_sync(init_hwprobe_vdso_data);
> #endif /* CONFIG_MMU */
>
> SYSCALL_DEFINE5(riscv_hwprobe, struct riscv_hwprobe __user *, pairs,
> - size_t, pair_count, size_t, cpu_count, unsigned long __user *,
> + size_t, pair_count, size_t, cpusetsize, unsigned long __user *,
> cpus, unsigned int, flags)
> {
> - return do_riscv_hwprobe(pairs, pair_count, cpu_count,
> + return do_riscv_hwprobe(pairs, pair_count, cpusetsize,
> cpus, flags);
> }
>
> diff --git a/arch/riscv/kernel/vdso/hwprobe.c b/arch/riscv/kernel/vdso/hwprobe.c
> index d40bec6ac078..3000e9fc0569 100644
> --- a/arch/riscv/kernel/vdso/hwprobe.c
> +++ b/arch/riscv/kernel/vdso/hwprobe.c
> @@ -8,21 +8,21 @@
> #include <vdso/helpers.h>
>
> extern int riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> - size_t cpu_count, unsigned long *cpus,
> + size_t cpusetsize, unsigned long *cpus,
> unsigned int flags);
>
> /* Add a prototype to avoid -Wmissing-prototypes warning. */
> int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> - size_t cpu_count, unsigned long *cpus,
> + size_t cpusetsize, unsigned long *cpus,
> unsigned int flags);
>
> int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> - size_t cpu_count, unsigned long *cpus,
> + size_t cpusetsize, unsigned long *cpus,
> unsigned int flags)
> {
> const struct vdso_data *vd = __arch_get_vdso_data();
> const struct arch_vdso_data *avd = &vd->arch_data;
> - bool all_cpus = !cpu_count && !cpus;
> + bool all_cpus = !cpusetsize && !cpus;
> struct riscv_hwprobe *p = pairs;
> struct riscv_hwprobe *end = pairs + pair_count;
>
> @@ -33,7 +33,7 @@ int __vdso_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> * masks.
> */
> if ((flags != 0) || (!all_cpus && !avd->homogeneous_cpus))
> - return riscv_hwprobe(pairs, pair_count, cpu_count, cpus, flags);
> + return riscv_hwprobe(pairs, pair_count, cpusetsize, cpus, flags);
>
> /* This is something we can handle, fill out the pairs. */
> while (p < end) {

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>