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

[Jae Hyun Yoo]

On 9/28/2020 2:09 PM, Zev Weiss wrote:
> On Mon, Sep 28, 2020 at 03:21:23PM CDT, Jae Hyun Yoo wrote:
>> On 9/28/2020 12:54 PM, Zev Weiss wrote:
>>> On Mon, Sep 28, 2020 at 02:08:24PM CDT, Jae Hyun Yoo wrote:
>>>>
>>>>
>>>> On 9/26/2020 2:27 PM, Zev Weiss wrote:
>>>>> Zero-based numbering is more consistent with all other cpu/core
>>>>> numbering I'm aware of (including the PECI spec).
>>>>>
>>>>> Signed-off-by: Zev Weiss <zev at bewilderbeest.net>
>>>>> ---
>>>>>  drivers/hwmon/peci-cputemp.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/peci-cputemp.c 
>>>>> b/drivers/hwmon/peci-cputemp.c
>>>>> index b9fe91281d58..78e442f433a7 100644
>>>>> --- a/drivers/hwmon/peci-cputemp.c
>>>>> +++ b/drivers/hwmon/peci-cputemp.c
>>>>> @@ -363,7 +363,7 @@ static int create_core_temp_label(struct 
>>>>> peci_cputemp *priv, int idx)
>>>>>      if (!priv->coretemp_label[idx])
>>>>>          return -ENOMEM;
>>>>> -    sprintf(priv->coretemp_label[idx], "Core %d", idx + 1);
>>>>> +    sprintf(priv->coretemp_label[idx], "Core %d", idx);
>>>>
>>>> Differently from low level indexing, it's labeling for users and it
>>>> should be synced with other temp or ADC sensors such as
>>>>
>>>> PVCCIN CPU1
>>>> PVDQ ABC CPU1
>>>> CPU1 P12V PVCCIN
>>>> CPU1 VR Mem ABCD
>>>> CPU1 VR P1V8
>>>>
>>>> These are using indexes starting from '1'.
>>>>
>>>
>>> OK, if it's for consistency with other existing drivers I suppose 
>>> that's reasonable, though for my own reference, could you point me to 
>>> where those are implemented?  Some rough grepping around the source 
>>> tree didn't appear to turn up anything relevant.
>>
>> Sensor names get assigned through these services
>> https://github.com/openbmc/entity-manager
>> https://github.com/openbmc/dbus-sensors
>>
>> and it depends on board configuration of each machine.
>>
> 
> 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?

Thanks,
Jae