openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
To: Zev Weiss <zev@bewilderbeest.net>
Cc: openbmc@lists.ozlabs.org,
	Jason M Biils <jason.m.bills@linux.intel.com>,
	James Feist <james.feist@linux.intel.com>
Subject: Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one
Date: Mon, 28 Sep 2020 15:20:59 -0700	[thread overview]
Message-ID: <ca07b326-f65a-4b59-47a1-15f98a2f3a40@linux.intel.com> (raw)
In-Reply-To: <20200928220124.k47kocdvi2ahgtc6@hatter.bewilderbeest.net>



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



  reply	other threads:[~2020-09-28 22:22 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-26 21:27 [PATCH 0/2] PECI patchset tweaks Zev Weiss
2020-09-26 21:27 ` [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() Zev Weiss
2020-09-28 19:03   ` Jae Hyun Yoo
2020-09-28 19:09     ` Zev Weiss
2020-09-28 19:37       ` Jae Hyun Yoo
2020-09-29  5:55   ` Joel Stanley
2020-09-26 21:27 ` [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one Zev Weiss
2020-09-28 19:08   ` Jae Hyun Yoo
2020-09-28 19:54     ` Zev Weiss
2020-09-28 20:21       ` Jae Hyun Yoo
2020-09-28 21:09         ` Zev Weiss
2020-09-28 21:32           ` Jae Hyun Yoo
2020-09-28 22:02             ` Zev Weiss
2020-09-28 22:20               ` Jae Hyun Yoo [this message]
2020-09-29  6:00               ` Joel Stanley
2020-10-06 18:01                 ` Jae Hyun Yoo
2020-10-07  5:39                   ` Joel Stanley
2020-09-28 19:01 ` [PATCH 0/2] PECI patchset tweaks Jae Hyun Yoo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ca07b326-f65a-4b59-47a1-15f98a2f3a40@linux.intel.com \
    --to=jae.hyun.yoo@linux.intel.com \
    --cc=james.feist@linux.intel.com \
    --cc=jason.m.bills@linux.intel.com \
    --cc=openbmc@lists.ozlabs.org \
    --cc=zev@bewilderbeest.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).