* [PATCH 0/2] PECI patchset tweaks
@ 2020-09-26 21:27 Zev Weiss
2020-09-26 21:27 ` [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() Zev Weiss
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Zev Weiss @ 2020-09-26 21:27 UTC (permalink / raw)
To: openbmc; +Cc: Jason M Biils, James Feist, Jae Hyun Yoo, Zev Weiss
[Re-sending as I clumsily typoed the mailing list's address on the
first attempt; apologies for the duplication.]
Hello,
These patches address a few small things I noticed with the PECI patch
set currently in the OpenBMC kernel tree. Assuming they're deemed
acceptable, I'd of course hope they get folded in to the version of
the PECI code that gets upstreamed into the mainline kernel.
(Please let me know if there's some other way I should send this -- I
looked for but couldn't find a gerrit repo for the obmc kernel or any
kernel-specific patch submission instructions in
openbmc/docs/CONTRIBUTING.md, so 'git send-email' is my best guess.)
Thanks,
Zev
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() 2020-09-26 21:27 [PATCH 0/2] PECI patchset tweaks Zev Weiss @ 2020-09-26 21:27 ` Zev Weiss 2020-09-28 19:03 ` 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:01 ` [PATCH 0/2] PECI patchset tweaks Jae Hyun Yoo 2 siblings, 2 replies; 18+ messages in thread From: Zev Weiss @ 2020-09-26 21:27 UTC (permalink / raw) To: openbmc; +Cc: Jason M Biils, James Feist, Jae Hyun Yoo, Zev Weiss peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also avoid calling kfree() on an ERR_PTR. Signed-off-by: Zev Weiss <zev@bewilderbeest.net> --- drivers/peci/peci-dev.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c index e0fe09467a80..84e90af81ccc 100644 --- a/drivers/peci/peci-dev.c +++ b/drivers/peci/peci-dev.c @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) } xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); - if (IS_ERR(xmsg)) { - ret = PTR_ERR(xmsg); + if (!xmsg) { + ret = -ENOMEM; break; } @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) } peci_put_xfer_msg(xmsg); - kfree(msg); + if (!IS_ERR(msg)) + kfree(msg); return (long)ret; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() 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-29 5:55 ` Joel Stanley 1 sibling, 1 reply; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 19:03 UTC (permalink / raw) To: Zev Weiss, openbmc; +Cc: Jason M Biils, James Feist Hello Zev, On 9/26/2020 2:27 PM, Zev Weiss wrote: > peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also > avoid calling kfree() on an ERR_PTR. > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net> > --- > drivers/peci/peci-dev.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c > index e0fe09467a80..84e90af81ccc 100644 > --- a/drivers/peci/peci-dev.c > +++ b/drivers/peci/peci-dev.c > @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); > - if (IS_ERR(xmsg)) { > - ret = PTR_ERR(xmsg); > + if (!xmsg) { > + ret = -ENOMEM; Yes, it's a right fix. Thanks! > break; > } > > @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > peci_put_xfer_msg(xmsg); > - kfree(msg); > + if (!IS_ERR(msg)) > + kfree(msg); Not needed. kfree itself has null pointer checking inside. > > return (long)ret; > } > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() 2020-09-28 19:03 ` Jae Hyun Yoo @ 2020-09-28 19:09 ` Zev Weiss 2020-09-28 19:37 ` Jae Hyun Yoo 0 siblings, 1 reply; 18+ messages in thread From: Zev Weiss @ 2020-09-28 19:09 UTC (permalink / raw) To: Jae Hyun Yoo; +Cc: openbmc, Jason M Biils, James Feist On Mon, Sep 28, 2020 at 02:03:12PM CDT, Jae Hyun Yoo wrote: >Hello Zev, > >On 9/26/2020 2:27 PM, Zev Weiss wrote: >>peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also >>avoid calling kfree() on an ERR_PTR. >> >>Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>--- >> drivers/peci/peci-dev.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c >>index e0fe09467a80..84e90af81ccc 100644 >>--- a/drivers/peci/peci-dev.c >>+++ b/drivers/peci/peci-dev.c >>@@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) >> } >> xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); >>- if (IS_ERR(xmsg)) { >>- ret = PTR_ERR(xmsg); >>+ if (!xmsg) { >>+ ret = -ENOMEM; > >Yes, it's a right fix. Thanks! > >> break; >> } >>@@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) >> } >> peci_put_xfer_msg(xmsg); >>- kfree(msg); >>+ if (!IS_ERR(msg)) >>+ kfree(msg); > >Not needed. kfree itself has null pointer checking inside. > Certainly, but the condition in question here isn't whether it's NULL, but whether it's an ERR_PTR (which as far as I can tell kfree() does not check for). As is, there's an error path that leads to passing a non-NULL but also non-kfree-safe ERR_PTR (the memdup_user() return value). Zev ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() 2020-09-28 19:09 ` Zev Weiss @ 2020-09-28 19:37 ` Jae Hyun Yoo 0 siblings, 0 replies; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 19:37 UTC (permalink / raw) To: Zev Weiss; +Cc: openbmc, Jason M Biils, James Feist On 9/28/2020 12:09 PM, Zev Weiss wrote: > On Mon, Sep 28, 2020 at 02:03:12PM CDT, Jae Hyun Yoo wrote: >> Hello Zev, >> >> On 9/26/2020 2:27 PM, Zev Weiss wrote: >>> peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also >>> avoid calling kfree() on an ERR_PTR. >>> >>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net> >>> --- >>> drivers/peci/peci-dev.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c >>> index e0fe09467a80..84e90af81ccc 100644 >>> --- a/drivers/peci/peci-dev.c >>> +++ b/drivers/peci/peci-dev.c >>> @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, >>> uint iocmd, ulong arg) >>> } >>> xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); >>> - if (IS_ERR(xmsg)) { >>> - ret = PTR_ERR(xmsg); >>> + if (!xmsg) { >>> + ret = -ENOMEM; >> >> Yes, it's a right fix. Thanks! >> >>> break; >>> } >>> @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, >>> uint iocmd, ulong arg) >>> } >>> peci_put_xfer_msg(xmsg); >>> - kfree(msg); >>> + if (!IS_ERR(msg)) >>> + kfree(msg); >> >> Not needed. kfree itself has null pointer checking inside. >> > > Certainly, but the condition in question here isn't whether it's NULL, > but whether it's an ERR_PTR (which as far as I can tell kfree() does not > check for). As is, there's an error path that leads to passing a > non-NULL but also non-kfree-safe ERR_PTR (the memdup_user() return value). Yeah, checked that memdup_user can also return ERR_PTR(-ENOMEM) or ERR_PTR(-EFAULT) in error cases so null checking isn't enough for calling free(). I'll add this change into PECI patch set. Thanks! Reviewed-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > > > Zev > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] peci: fix error-handling in peci_dev_ioctl() 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-29 5:55 ` Joel Stanley 1 sibling, 0 replies; 18+ messages in thread From: Joel Stanley @ 2020-09-29 5:55 UTC (permalink / raw) To: Zev Weiss; +Cc: OpenBMC Maillist, Jae Hyun Yoo, Jason M Biils, James Feist On Sat, 26 Sep 2020 at 21:27, Zev Weiss <zev@bewilderbeest.net> wrote: > > peci_get_xfer_msg() returns NULL on failure, not an ERR_PTR. Also > avoid calling kfree() on an ERR_PTR. > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net> Fixes: 90ddc4e972b5 ("peci: Add support for PECI bus driver core") Reviewed-by: Joel Stanley <joel@jms.id.au> Applied to dev-5.8. Cheers, Joel > --- > drivers/peci/peci-dev.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/peci/peci-dev.c b/drivers/peci/peci-dev.c > index e0fe09467a80..84e90af81ccc 100644 > --- a/drivers/peci/peci-dev.c > +++ b/drivers/peci/peci-dev.c > @@ -122,8 +122,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > xmsg = peci_get_xfer_msg(uxmsg.tx_len, uxmsg.rx_len); > - if (IS_ERR(xmsg)) { > - ret = PTR_ERR(xmsg); > + if (!xmsg) { > + ret = -ENOMEM; > break; > } > > @@ -162,7 +162,8 @@ static long peci_dev_ioctl(struct file *file, uint iocmd, ulong arg) > } > > peci_put_xfer_msg(xmsg); > - kfree(msg); > + if (!IS_ERR(msg)) > + kfree(msg); > > return (long)ret; > } > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 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-26 21:27 ` Zev Weiss 2020-09-28 19:08 ` Jae Hyun Yoo 2020-09-28 19:01 ` [PATCH 0/2] PECI patchset tweaks Jae Hyun Yoo 2 siblings, 1 reply; 18+ messages in thread From: Zev Weiss @ 2020-09-26 21:27 UTC (permalink / raw) To: openbmc; +Cc: Jason M Biils, James Feist, Jae Hyun Yoo, Zev Weiss 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@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); return 0; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 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 0 siblings, 1 reply; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 19:08 UTC (permalink / raw) To: Zev Weiss, openbmc; +Cc: Jason M Biils, James Feist 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@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'. Thanks, Jae > return 0; > } > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 19:08 ` Jae Hyun Yoo @ 2020-09-28 19:54 ` Zev Weiss 2020-09-28 20:21 ` Jae Hyun Yoo 0 siblings, 1 reply; 18+ messages in thread From: Zev Weiss @ 2020-09-28 19:54 UTC (permalink / raw) To: Jae Hyun Yoo; +Cc: openbmc, Jason M Biils, James Feist 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@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. Thanks, Zev ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 19:54 ` Zev Weiss @ 2020-09-28 20:21 ` Jae Hyun Yoo 2020-09-28 21:09 ` Zev Weiss 0 siblings, 1 reply; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 20:21 UTC (permalink / raw) To: Zev Weiss; +Cc: openbmc, Jason M Biils, James Feist 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@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. > Thanks, > Zev > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 20:21 ` Jae Hyun Yoo @ 2020-09-28 21:09 ` Zev Weiss 2020-09-28 21:32 ` Jae Hyun Yoo 0 siblings, 1 reply; 18+ messages in thread From: Zev Weiss @ 2020-09-28 21:09 UTC (permalink / raw) To: Jae Hyun Yoo; +Cc: openbmc, Jason M Biils, James Feist 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@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? Thanks, Zev ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 21:09 ` Zev Weiss @ 2020-09-28 21:32 ` Jae Hyun Yoo 2020-09-28 22:02 ` Zev Weiss 0 siblings, 1 reply; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 21:32 UTC (permalink / raw) To: Zev Weiss; +Cc: openbmc, Jason M Biils, James Feist 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@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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 21:32 ` Jae Hyun Yoo @ 2020-09-28 22:02 ` Zev Weiss 2020-09-28 22:20 ` Jae Hyun Yoo 2020-09-29 6:00 ` Joel Stanley 0 siblings, 2 replies; 18+ messages in thread From: Zev Weiss @ 2020-09-28 22:02 UTC (permalink / raw) To: Jae Hyun Yoo; +Cc: openbmc, Jason M Biils, James Feist 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. Thanks, Zev ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 22:02 ` Zev Weiss @ 2020-09-28 22:20 ` Jae Hyun Yoo 2020-09-29 6:00 ` Joel Stanley 1 sibling, 0 replies; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 22:20 UTC (permalink / raw) To: Zev Weiss; +Cc: openbmc, Jason M Biils, James Feist 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-28 22:02 ` Zev Weiss 2020-09-28 22:20 ` Jae Hyun Yoo @ 2020-09-29 6:00 ` Joel Stanley 2020-10-06 18:01 ` Jae Hyun Yoo 1 sibling, 1 reply; 18+ messages in thread From: Joel Stanley @ 2020-09-29 6:00 UTC (permalink / raw) To: Zev Weiss; +Cc: Jason M Biils, Jae Hyun Yoo, OpenBMC Maillist, James Feist On Mon, 28 Sep 2020 at 22:02, Zev Weiss <zev@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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-09-29 6:00 ` Joel Stanley @ 2020-10-06 18:01 ` Jae Hyun Yoo 2020-10-07 5:39 ` Joel Stanley 0 siblings, 1 reply; 18+ messages in thread From: Jae Hyun Yoo @ 2020-10-06 18:01 UTC (permalink / raw) To: Joel Stanley, Zev Weiss; +Cc: Jason M Biils, OpenBMC Maillist, James Feist Hi Zev, On 9/28/2020 11:00 PM, Joel Stanley wrote: > On Mon, 28 Sep 2020 at 22:02, Zev Weiss <zev@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@linux.intel.com> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one 2020-10-06 18:01 ` Jae Hyun Yoo @ 2020-10-07 5:39 ` Joel Stanley 0 siblings, 0 replies; 18+ messages in thread From: Joel Stanley @ 2020-10-07 5:39 UTC (permalink / raw) To: Jae Hyun Yoo; +Cc: Jason M Biils, OpenBMC Maillist, Zev Weiss, James Feist On Tue, 6 Oct 2020 at 18:02, Jae Hyun Yoo <jae.hyun.yoo@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@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@linux.intel.com> Applied to dev-5.8. Cheers, Joel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] PECI patchset tweaks 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-26 21:27 ` [PATCH 2/2] peci-cputemp: label CPU cores from zero instead of one Zev Weiss @ 2020-09-28 19:01 ` Jae Hyun Yoo 2 siblings, 0 replies; 18+ messages in thread From: Jae Hyun Yoo @ 2020-09-28 19:01 UTC (permalink / raw) To: Zev Weiss, openbmc; +Cc: Jason M Biils, James Feist Hello Zev, On 9/26/2020 2:27 PM, Zev Weiss wrote: > [Re-sending as I clumsily typoed the mailing list's address on the > first attempt; apologies for the duplication.] > > Hello, > > These patches address a few small things I noticed with the PECI patch > set currently in the OpenBMC kernel tree. Assuming they're deemed > acceptable, I'd of course hope they get folded in to the version of > the PECI code that gets upstreamed into the mainline kernel. > > (Please let me know if there's some other way I should send this -- I > looked for but couldn't find a gerrit repo for the obmc kernel or any > kernel-specific patch submission instructions in > openbmc/docs/CONTRIBUTING.md, so 'git send-email' is my best guess.) This mailing list would be a right place as long as the PECI patch set is in OpenBMC kernel tree but in case if it is removed from the tree later (probably from the 5.9 kernel), you may need to use pull request through https://github.com/Intel-BMC/openbmc when it replaces the kernel repo with forked kernel. Thanks, Jae > > Thanks, > Zev > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-10-07 5:41 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).