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

[Andrew Jones <ajones@ventanamicro.com>]

On Mon, Sep 25, 2023 at 04:23:22AM -0700, Palmer Dabbelt wrote:
> 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.

After applying the patch we can use 'git show --word-diff=color' on it.
In this case, we'll see the "with size ``cpusetsize``" change, and that I
also changed "vendor/arch/impl" to "vendor, arch, impl", dropped the first
'be' from "will be only be" and added `` around cpus.

> 
> > +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>

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv