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

[Joel Stanley]

On Tue, 6 Oct 2020 at 18:02, Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com> wrote:
>
> Hi Zev,
>
> On 9/28/2020 11:00 PM, Joel Stanley wrote:
> > 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.
>
> Okay. Not a big deal. The coretemp module for local CPU also uses zero
> starting label index for core numbers so better match up. Thanks for
> your patch.
>
> Reviewed-by: Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com>

Applied to dev-5.8.

Cheers,

Joel