linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] hwmon (occ): Add temp sensor value check
@ 2019-04-17 11:26 Alexander Amelkin
  2019-04-17 13:03 ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Amelkin @ 2019-04-17 11:26 UTC (permalink / raw)
  To: linux-hwmon
  Cc: openbmc, Alexander Soldatov, Alexander Amelkin, Edward A . James,
	Joel Stanley

From: Alexander Soldatov <a.soldatov@yadro.com>

The occ driver supports two formats for the temp sensor value.

The OCC firmware for P8 supports only the first format, for which
no range checking or error processing is performed in the driver.
Inspecting the OCC sources for P8 reveals that OCC may send
a special value 0xFFFF to indicate that a sensor read timeout
has occured, see

https://github.com/open-power/occ/blob/master_p8/src/occ/cmdh/cmdh_fsp_cmds.c#L395

That situation wasn't handled in the driver. This patch adds invalid
temp value check for the sensor data format 1 and handles it the same
way as it is done for the format 2, where EREMOTEIO is reported for
this case.

Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com>
Reviewed-by: Alexander Amelkin <a.amelkin@yadro.com>
Cc: Edward A. James <eajames@us.ibm.com>
Cc: Joel Stanley <joel@jms.id.au>
---
 drivers/hwmon/occ/common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 4679acb..825631c 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -235,6 +235,10 @@ static ssize_t occ_show_temp_1(struct device *dev,
 		val = get_unaligned_be16(&temp->sensor_id);
 		break;
 	case 1:
+		/* If a sensor timed out long enough,
+		   OCC returns 0xFFFF for that sensor.*/
+		if (temp->value == 0xFFFF)
+			return -EREMOTEIO;
 		val = get_unaligned_be16(&temp->value) * 1000;
 		break;
 	default:
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hwmon (occ): Add temp sensor value check
  2019-04-17 11:26 [PATCH 1/1] hwmon (occ): Add temp sensor value check Alexander Amelkin
@ 2019-04-17 13:03 ` Guenter Roeck
  2019-04-17 17:46   ` Alexander Amelkin
  0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2019-04-17 13:03 UTC (permalink / raw)
  To: Alexander Amelkin, linux-hwmon
  Cc: openbmc, Alexander Soldatov, Edward A . James, Joel Stanley

On 4/17/19 4:26 AM, Alexander Amelkin wrote:
> From: Alexander Soldatov <a.soldatov@yadro.com>
> 
> The occ driver supports two formats for the temp sensor value.
> 
> The OCC firmware for P8 supports only the first format, for which
> no range checking or error processing is performed in the driver.
> Inspecting the OCC sources for P8 reveals that OCC may send
> a special value 0xFFFF to indicate that a sensor read timeout
> has occured, see
> 
occurred

> https://github.com/open-power/occ/blob/master_p8/src/occ/cmdh/cmdh_fsp_cmds.c#L395
> 
> That situation wasn't handled in the driver. This patch adds invalid
> temp value check for the sensor data format 1 and handles it the same
> way as it is done for the format 2, where EREMOTEIO is reported for
> this case.
>
ETIMEDOUT ? Though that is really a corner case, so I guess both are fine.

> Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
> Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com>
> Reviewed-by: Alexander Amelkin <a.amelkin@yadro.com>
> Cc: Edward A. James <eajames@us.ibm.com>
> Cc: Joel Stanley <joel@jms.id.au>
> ---
>   drivers/hwmon/occ/common.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 4679acb..825631c 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -235,6 +235,10 @@ static ssize_t occ_show_temp_1(struct device *dev,
>   		val = get_unaligned_be16(&temp->sensor_id);
>   		break;
>   	case 1:
> +		/* If a sensor timed out long enough,

"timed out" is sufficient. "timed out long enough" is difficult to understand.

> +		   OCC returns 0xFFFF for that sensor.*/

/*
  * This is how multi-line comments look like
  */

Please run checkpatch on your patches.

Thanks,
Guenter

> +		if (temp->value == 0xFFFF)
> +			return -EREMOTEIO;
>   		val = get_unaligned_be16(&temp->value) * 1000;
>   		break;
>   	default:
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hwmon (occ): Add temp sensor value check
  2019-04-17 13:03 ` Guenter Roeck
@ 2019-04-17 17:46   ` Alexander Amelkin
  2019-04-17 18:03     ` [PATCH v2 " Alexander Amelkin
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Amelkin @ 2019-04-17 17:46 UTC (permalink / raw)
  To: Guenter Roeck, linux-hwmon
  Cc: openbmc, Alexander Soldatov, Edward A . James, Joel Stanley


[-- Attachment #1.1: Type: text/plain, Size: 2431 bytes --]

17.04.2019 16:03, Guenter Roeck wrote:
> On 4/17/19 4:26 AM, Alexander Amelkin wrote:
>> Inspecting the OCC sources for P8 reveals that OCC may send
>> a special value 0xFFFF to indicate that a sensor read timeout
>> has occured, see
>>
> occurred

Yup. A typo. Will fix.

>
>> https://github.com/open-power/occ/blob/master_p8/src/occ/cmdh/cmdh_fsp_cmds.c#L395
>>
>> That situation wasn't handled in the driver. This patch adds invalid
>> temp value check for the sensor data format 1 and handles it the same
>> way as it is done for the format 2, where EREMOTEIO is reported for
>> this case.
>>
> ETIMEDOUT ? Though that is really a corner case, so I guess both are fine.

We just reused the error code used for the same case for format 2 in common.c:309 (inside occ_show_temp_2() function).

We thought it would be strange to report different codes for the same case in different format versions.

Besides, it's quite a remote I/O error indeed.

>
>> Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
>> Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com>
>> Reviewed-by: Alexander Amelkin <a.amelkin@yadro.com>
>> Cc: Edward A. James <eajames@us.ibm.com>
>> Cc: Joel Stanley <joel@jms.id.au>
>> ---
>>   drivers/hwmon/occ/common.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
>> index 4679acb..825631c 100644
>> --- a/drivers/hwmon/occ/common.c
>> +++ b/drivers/hwmon/occ/common.c
>> @@ -235,6 +235,10 @@ static ssize_t occ_show_temp_1(struct device *dev,
>>           val = get_unaligned_be16(&temp->sensor_id);
>>           break;
>>       case 1:
>> +        /* If a sensor timed out long enough,
>
> "timed out" is sufficient. "timed out long enough" is difficult to understand.
Agreed. That's a weird wording, but I double-checked before submitting that it was just a copy-paste of the wording from the OCC sources for this case. You're probably right though that it's better to fix it.
>
>> +           OCC returns 0xFFFF for that sensor.*/
>
> /*
>  * This is how multi-line comments look like
>  */
>
> Please run checkpatch on your patches.

Mea culpa. Didn't check. Will fix tomorrow.

>
> Thanks,
> Guenter
>
Thanks.

With best regards,
Alexander Amelkin,
Leading BMC Software Engineer, YADRO
https://yadro.com




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 1/1] hwmon (occ): Add temp sensor value check
  2019-04-17 17:46   ` Alexander Amelkin
@ 2019-04-17 18:03     ` Alexander Amelkin
  2019-04-17 18:35       ` Eddie James
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Amelkin @ 2019-04-17 18:03 UTC (permalink / raw)
  To: linux-hwmon
  Cc: openbmc, Alexander Soldatov, Alexander Amelkin, Edward A . James,
	Joel Stanley

From: Alexander Soldatov <a.soldatov@yadro.com>

The occ driver supports two formats for the temp sensor value.

The OCC firmware for P8 supports only the first format, for which
no range checking or error processing is performed in the driver.
Inspecting the OCC sources for P8 reveals that OCC may send
a special value 0xFFFF to indicate that a sensor read timeout
has occurred, see

https://github.com/open-power/occ/blob/master_p8/src/occ/cmdh/cmdh_fsp_cmds.c#L395

That situation wasn't handled in the driver. This patch adds invalid
temp value check for the sensor data format 1 and handles it the same
way as it is done for the format 2, where EREMOTEIO is reported for
this case.

Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com>
Reviewed-by: Alexander Amelkin <a.amelkin@yadro.com>
Cc: Edward A. James <eajames@us.ibm.com>
Cc: Joel Stanley <joel@jms.id.au>
---
 drivers/hwmon/occ/common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 4679acb..825631c 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -235,6 +235,12 @@ static ssize_t occ_show_temp_1(struct device *dev,
 		val = get_unaligned_be16(&temp->sensor_id);
 		break;
 	case 1:
+		/*
+		 * If a sensor reading has expired and couldn't be refreshed,
+		 * OCC returns 0xFFFF for that sensor.
+		 */
+		if (temp->value == 0xFFFF)
+			return -EREMOTEIO;
 		val = get_unaligned_be16(&temp->value) * 1000;
 		break;
 	default:
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] hwmon (occ): Add temp sensor value check
  2019-04-17 18:03     ` [PATCH v2 " Alexander Amelkin
@ 2019-04-17 18:35       ` Eddie James
  2019-04-17 19:36         ` Guenter Roeck
  0 siblings, 1 reply; 6+ messages in thread
From: Eddie James @ 2019-04-17 18:35 UTC (permalink / raw)
  To: Alexander Amelkin, linux-hwmon
  Cc: Edward A . James, openbmc, Alexander Soldatov


On 4/17/19 1:03 PM, Alexander Amelkin wrote:
> From: Alexander Soldatov <a.soldatov@yadro.com>
>
> The occ driver supports two formats for the temp sensor value.
>
> The OCC firmware for P8 supports only the first format, for which
> no range checking or error processing is performed in the driver.
> Inspecting the OCC sources for P8 reveals that OCC may send
> a special value 0xFFFF to indicate that a sensor read timeout
> has occurred, see
>
> https://github.com/open-power/occ/blob/master_p8/src/occ/cmdh/cmdh_fsp_cmds.c#L395
>
> That situation wasn't handled in the driver. This patch adds invalid
> temp value check for the sensor data format 1 and handles it the same
> way as it is done for the format 2, where EREMOTEIO is reported for
> this case.


Thanks Alexander and Guenter.


Reviewed-by: Eddie James <eajames@linux.ibm.com>


>
> Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
> Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com>
> Reviewed-by: Alexander Amelkin <a.amelkin@yadro.com>
> Cc: Edward A. James <eajames@us.ibm.com>
> Cc: Joel Stanley <joel@jms.id.au>
> ---
>   drivers/hwmon/occ/common.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 4679acb..825631c 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -235,6 +235,12 @@ static ssize_t occ_show_temp_1(struct device *dev,
>   		val = get_unaligned_be16(&temp->sensor_id);
>   		break;
>   	case 1:
> +		/*
> +		 * If a sensor reading has expired and couldn't be refreshed,
> +		 * OCC returns 0xFFFF for that sensor.
> +		 */
> +		if (temp->value == 0xFFFF)
> +			return -EREMOTEIO;
>   		val = get_unaligned_be16(&temp->value) * 1000;
>   		break;
>   	default:


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] hwmon (occ): Add temp sensor value check
  2019-04-17 18:35       ` Eddie James
@ 2019-04-17 19:36         ` Guenter Roeck
  0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2019-04-17 19:36 UTC (permalink / raw)
  To: Eddie James
  Cc: Alexander Amelkin, linux-hwmon, Edward A . James, openbmc,
	Alexander Soldatov

On Wed, Apr 17, 2019 at 01:35:41PM -0500, Eddie James wrote:
> 
> On 4/17/19 1:03 PM, Alexander Amelkin wrote:
> >From: Alexander Soldatov <a.soldatov@yadro.com>
> >
> >The occ driver supports two formats for the temp sensor value.
> >
> >The OCC firmware for P8 supports only the first format, for which
> >no range checking or error processing is performed in the driver.
> >Inspecting the OCC sources for P8 reveals that OCC may send
> >a special value 0xFFFF to indicate that a sensor read timeout
> >has occurred, see
> >
> >https://github.com/open-power/occ/blob/master_p8/src/occ/cmdh/cmdh_fsp_cmds.c#L395
> >
> >That situation wasn't handled in the driver. This patch adds invalid
> >temp value check for the sensor data format 1 and handles it the same
> >way as it is done for the format 2, where EREMOTEIO is reported for
> >this case.
> 
> 
> Thanks Alexander and Guenter.
> 
> 
> Reviewed-by: Eddie James <eajames@linux.ibm.com>
> 

Applied.

Thanks,
Guenter

> 
> >
> >Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
> >Signed-off-by: Alexander Amelkin <a.amelkin@yadro.com>
> >Reviewed-by: Alexander Amelkin <a.amelkin@yadro.com>
> >Cc: Edward A. James <eajames@us.ibm.com>
> >Cc: Joel Stanley <joel@jms.id.au>
> >---
> >  drivers/hwmon/occ/common.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> >index 4679acb..825631c 100644
> >--- a/drivers/hwmon/occ/common.c
> >+++ b/drivers/hwmon/occ/common.c
> >@@ -235,6 +235,12 @@ static ssize_t occ_show_temp_1(struct device *dev,
> >  		val = get_unaligned_be16(&temp->sensor_id);
> >  		break;
> >  	case 1:
> >+		/*
> >+		 * If a sensor reading has expired and couldn't be refreshed,
> >+		 * OCC returns 0xFFFF for that sensor.
> >+		 */
> >+		if (temp->value == 0xFFFF)
> >+			return -EREMOTEIO;
> >  		val = get_unaligned_be16(&temp->value) * 1000;
> >  		break;
> >  	default:
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-17 19:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-17 11:26 [PATCH 1/1] hwmon (occ): Add temp sensor value check Alexander Amelkin
2019-04-17 13:03 ` Guenter Roeck
2019-04-17 17:46   ` Alexander Amelkin
2019-04-17 18:03     ` [PATCH v2 " Alexander Amelkin
2019-04-17 18:35       ` Eddie James
2019-04-17 19:36         ` Guenter Roeck

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