[PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one

[Joel Stanley]

On Mon, 28 Sep 2020 at 22:02, Zev Weiss <zev at bewilderbeest.net> wrote:
>
> On Mon, Sep 28, 2020 at 04:32:31PM CDT, Jae Hyun Yoo wrote:
> >>Oh I see -- I had thought you were referring to other existing hwmon
> >>drivers in the kernel.
> >>
> >>As far as I can tell, all those instances appear to be numbering CPU
> >>*sockets* though -- which as Jason mentioned in a call earlier today
> >>I gather is done to line up with motherboard silkscreen labeling.
> >>But in the code in question here we're labeling *cores* within a
> >>given socket, which I don't see arising anywhere in any existing
> >>entity-manager configs.  So I'm still unclear on why we want to use
> >>one-based indexing here instead of zero-based -- I'd think we'd want
> >>the PECI driver to match the PECI spec?
> >
> >PECI driver uses zero-based index for PECI command handling but label is
> >user facing stuff which shouldn't make confusion to users. We can modify
> >driver like you did in this patch and previous driver also used
> >zero-based indexing but I changed it to natural number based indexing
> >to avoid confusion between driver labels and dbus-sensors names.
> >Any specific reason for the zero-based indexing? Any benefit?
> >
>
> [Re-adding CCs...]

Thanks. Please keep the discussion on the list.

>
> Well, as I see it basically just consistency with a larger set of
> things.  Most other related numbering schemes I'm aware of are
> zero-based -- userspace tools like 'taskset' and 'lscpu', system APIs
> like the <sched.h> CPU_SET() routines, and the kernel's own numbering
> (e.g. what's shown in /proc/cpuinfo) all number processors starting from
> zero, so dbus-sensors seems kind of like the odd one out there.
> (Personally I'd be fully in support of changing it to be zero-based as
> well, though I have no idea offhand about how distruptive a change that
> would be.)
>
> It also seems pretty OpenBMC-specific, whereas I'd expect we want to aim
> for greater generality in things going into mainline.

Agreed. The hwmon numbering varies; some attributes are zero indexed
and some start at 1. More commonly we start counting from zero in the
kernel, so I would expect PECI to do the same.

If there's some userspace that depends on the behaviour of these out
of tree PECI patches, then that userspace will need to change. This
reminds us why the project prefers patches exposing userspace ABI are
merged to mainline first.

Cheers,

Joel