openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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 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

* 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 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 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 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 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

* 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

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).