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

[Jae Hyun Yoo]

On 9/28/2020 3:02 PM, Zev Weiss 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...]
> 
> 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.

First of all, it's for labeling of sysfs interfaces in hwmon subsystem
which monitors target CPUs, not for local CPUs you mentioned. As you can
see in hwmon subsystem, property indexing starts from 1. Also, we have
used this natural number indexing traditionally for all Intel BMCs we
made so far, and it's also for keeping product value against
misreading of actual core numbers. If you don't have any critical or
blocking issue, I need to keep the current indexing as is.

Thanks,
Jae