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

[Palmer Dabbelt]

On Thu, 21 Sep 2023 05:55:20 PDT (-0700), ajones at 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 at 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 at rivosinc.com>