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

Andrew Jones
2023-09-25T12:08:03Z

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