All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hwmon: sparx5: Fix initial reading of temperature
@ 2020-09-03 13:47 ` Lars Povlsen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars Povlsen @ 2020-09-03 13:47 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Lars Povlsen, Jean Delvare, Microchip Linux Driver Support,
	linux-hwmon, linux-arm-kernel, linux-kernel

If the temperature is read before the internal calibration is
completed, the driver returns -EIO. Instead it should return -EAGAIN
to encourage repeating the operation.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 drivers/hwmon/sparx5-temp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
index 1a2b1026b026..98be48e3a22a 100644
--- a/drivers/hwmon/sparx5-temp.c
+++ b/drivers/hwmon/sparx5-temp.c
@@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_temp_input:
 		stat = readl_relaxed(hwmon->base + TEMP_STAT);
 		if (!(stat & TEMP_STAT_VALID))
-			return -EIO;
+			return -EAGAIN;
 		value = stat & TEMP_STAT_TEMP;
 		/*
 		 * From register documentation:
--
2.27.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] hwmon: sparx5: Fix initial reading of temperature
@ 2020-09-03 13:47 ` Lars Povlsen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars Povlsen @ 2020-09-03 13:47 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-hwmon, Jean Delvare, linux-kernel,
	Microchip Linux Driver Support, Lars Povlsen, linux-arm-kernel

If the temperature is read before the internal calibration is
completed, the driver returns -EIO. Instead it should return -EAGAIN
to encourage repeating the operation.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 drivers/hwmon/sparx5-temp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
index 1a2b1026b026..98be48e3a22a 100644
--- a/drivers/hwmon/sparx5-temp.c
+++ b/drivers/hwmon/sparx5-temp.c
@@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_temp_input:
 		stat = readl_relaxed(hwmon->base + TEMP_STAT);
 		if (!(stat & TEMP_STAT_VALID))
-			return -EIO;
+			return -EAGAIN;
 		value = stat & TEMP_STAT_TEMP;
 		/*
 		 * From register documentation:
--
2.27.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
  2020-09-03 13:47 ` Lars Povlsen
@ 2020-09-03 13:52   ` Guenter Roeck
  -1 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-09-03 13:52 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: Jean Delvare, Microchip Linux Driver Support, linux-hwmon,
	linux-arm-kernel, linux-kernel

On 9/3/20 6:47 AM, Lars Povlsen wrote:
> If the temperature is read before the internal calibration is
> completed, the driver returns -EIO. Instead it should return -EAGAIN
> to encourage repeating the operation.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  drivers/hwmon/sparx5-temp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
> index 1a2b1026b026..98be48e3a22a 100644
> --- a/drivers/hwmon/sparx5-temp.c
> +++ b/drivers/hwmon/sparx5-temp.c
> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>  	case hwmon_temp_input:
>  		stat = readl_relaxed(hwmon->base + TEMP_STAT);
>  		if (!(stat & TEMP_STAT_VALID))
> -			return -EIO;
> +			return -EAGAIN;

The problem is that this may result in a hard loop and effectively hang the system,
or at least the calling process. It may be better to return -ENODATA in this situation.

Guenter

>  		value = stat & TEMP_STAT_TEMP;
>  		/*
>  		 * From register documentation:
> --
> 2.27.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
@ 2020-09-03 13:52   ` Guenter Roeck
  0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-09-03 13:52 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: linux-hwmon, Jean Delvare, linux-kernel, linux-arm-kernel,
	Microchip Linux Driver Support

On 9/3/20 6:47 AM, Lars Povlsen wrote:
> If the temperature is read before the internal calibration is
> completed, the driver returns -EIO. Instead it should return -EAGAIN
> to encourage repeating the operation.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  drivers/hwmon/sparx5-temp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
> index 1a2b1026b026..98be48e3a22a 100644
> --- a/drivers/hwmon/sparx5-temp.c
> +++ b/drivers/hwmon/sparx5-temp.c
> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>  	case hwmon_temp_input:
>  		stat = readl_relaxed(hwmon->base + TEMP_STAT);
>  		if (!(stat & TEMP_STAT_VALID))
> -			return -EIO;
> +			return -EAGAIN;

The problem is that this may result in a hard loop and effectively hang the system,
or at least the calling process. It may be better to return -ENODATA in this situation.

Guenter

>  		value = stat & TEMP_STAT_TEMP;
>  		/*
>  		 * From register documentation:
> --
> 2.27.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
  2020-09-03 13:52   ` Guenter Roeck
@ 2020-09-04  8:33     ` Lars Povlsen
  -1 siblings, 0 replies; 10+ messages in thread
From: Lars Povlsen @ 2020-09-04  8:33 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Lars Povlsen, Jean Delvare, Microchip Linux Driver Support,
	linux-hwmon, linux-arm-kernel, linux-kernel, Zhang Rui,
	Daniel Lezcano, Amit Kucheria


Guenter Roeck writes:

> On 9/3/20 6:47 AM, Lars Povlsen wrote:
>> If the temperature is read before the internal calibration is
>> completed, the driver returns -EIO. Instead it should return -EAGAIN
>> to encourage repeating the operation.
>>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>> ---
>>  drivers/hwmon/sparx5-temp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
>> index 1a2b1026b026..98be48e3a22a 100644
>> --- a/drivers/hwmon/sparx5-temp.c
>> +++ b/drivers/hwmon/sparx5-temp.c
>> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>>       case hwmon_temp_input:
>>               stat = readl_relaxed(hwmon->base + TEMP_STAT);
>>               if (!(stat & TEMP_STAT_VALID))
>> -                     return -EIO;
>> +                     return -EAGAIN;
>
> The problem is that this may result in a hard loop and effectively hang the system,
> or at least the calling process. It may be better to return -ENODATA in this situation.
>

Guenther,

Thank you for your response.

The motivation for the patch was as I tried to use thermal support to
control a fan, and I noticed an initial quip from
thermal_core.c:update_temperature() - it will throw a dev_warn() for
anything else than an "EAGAIN". The EAGAIN also seems to be used by
several other hwmon drivers in similar situations.

The bottom line is that I would like to get rid of the warning
message. Changing the error code to ENODATA does not fix what I am
trying to accomplish.

Do you have any suggestions on how to achieve the objective?

Sincerely,

---Lars

> Guenter
>
>>               value = stat & TEMP_STAT_TEMP;
>>               /*
>>                * From register documentation:
>> --
>> 2.27.0
>>

-- 
Lars Povlsen,
Microchip

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
@ 2020-09-04  8:33     ` Lars Povlsen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars Povlsen @ 2020-09-04  8:33 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-hwmon, Jean Delvare, Daniel Lezcano, linux-kernel,
	Microchip Linux Driver Support, Lars Povlsen, Amit Kucheria,
	Zhang Rui, linux-arm-kernel


Guenter Roeck writes:

> On 9/3/20 6:47 AM, Lars Povlsen wrote:
>> If the temperature is read before the internal calibration is
>> completed, the driver returns -EIO. Instead it should return -EAGAIN
>> to encourage repeating the operation.
>>
>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>> ---
>>  drivers/hwmon/sparx5-temp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
>> index 1a2b1026b026..98be48e3a22a 100644
>> --- a/drivers/hwmon/sparx5-temp.c
>> +++ b/drivers/hwmon/sparx5-temp.c
>> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>>       case hwmon_temp_input:
>>               stat = readl_relaxed(hwmon->base + TEMP_STAT);
>>               if (!(stat & TEMP_STAT_VALID))
>> -                     return -EIO;
>> +                     return -EAGAIN;
>
> The problem is that this may result in a hard loop and effectively hang the system,
> or at least the calling process. It may be better to return -ENODATA in this situation.
>

Guenther,

Thank you for your response.

The motivation for the patch was as I tried to use thermal support to
control a fan, and I noticed an initial quip from
thermal_core.c:update_temperature() - it will throw a dev_warn() for
anything else than an "EAGAIN". The EAGAIN also seems to be used by
several other hwmon drivers in similar situations.

The bottom line is that I would like to get rid of the warning
message. Changing the error code to ENODATA does not fix what I am
trying to accomplish.

Do you have any suggestions on how to achieve the objective?

Sincerely,

---Lars

> Guenter
>
>>               value = stat & TEMP_STAT_TEMP;
>>               /*
>>                * From register documentation:
>> --
>> 2.27.0
>>

-- 
Lars Povlsen,
Microchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
  2020-09-04  8:33     ` Lars Povlsen
@ 2020-09-04 14:08       ` Guenter Roeck
  -1 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-09-04 14:08 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: Jean Delvare, Microchip Linux Driver Support, linux-hwmon,
	linux-arm-kernel, linux-kernel, Zhang Rui, Daniel Lezcano,
	Amit Kucheria

On 9/4/20 1:33 AM, Lars Povlsen wrote:
> 
> Guenter Roeck writes:
> 
>> On 9/3/20 6:47 AM, Lars Povlsen wrote:
>>> If the temperature is read before the internal calibration is
>>> completed, the driver returns -EIO. Instead it should return -EAGAIN
>>> to encourage repeating the operation.
>>>
>>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>>> ---
>>>  drivers/hwmon/sparx5-temp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
>>> index 1a2b1026b026..98be48e3a22a 100644
>>> --- a/drivers/hwmon/sparx5-temp.c
>>> +++ b/drivers/hwmon/sparx5-temp.c
>>> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>>>       case hwmon_temp_input:
>>>               stat = readl_relaxed(hwmon->base + TEMP_STAT);
>>>               if (!(stat & TEMP_STAT_VALID))
>>> -                     return -EIO;
>>> +                     return -EAGAIN;
>>
>> The problem is that this may result in a hard loop and effectively hang the system,
>> or at least the calling process. It may be better to return -ENODATA in this situation.
>>
> 
> Guenther,
> 
> Thank you for your response.
> 
> The motivation for the patch was as I tried to use thermal support to
> control a fan, and I noticed an initial quip from
> thermal_core.c:update_temperature() - it will throw a dev_warn() for
> anything else than an "EAGAIN". The EAGAIN also seems to be used by
> several other hwmon drivers in similar situations.
> 
> The bottom line is that I would like to get rid of the warning
> message. Changing the error code to ENODATA does not fix what I am
> trying to accomplish.
> 

Sigh. I know that -EAGAIN is used, but it has its own problems - userspace
is inclined to retry directly, causing hard loops.

Ok, lets go with -EAGAIN then. Can't be helped. I'll apply your patch.

Guenter

> Do you have any suggestions on how to achieve the objective?
> 
> Sincerely,
> 
> ---Lars
> 
>> Guenter
>>
>>>               value = stat & TEMP_STAT_TEMP;
>>>               /*
>>>                * From register documentation:
>>> --
>>> 2.27.0
>>>
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
@ 2020-09-04 14:08       ` Guenter Roeck
  0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-09-04 14:08 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: linux-hwmon, Jean Delvare, Daniel Lezcano, linux-kernel,
	Microchip Linux Driver Support, Amit Kucheria, Zhang Rui,
	linux-arm-kernel

On 9/4/20 1:33 AM, Lars Povlsen wrote:
> 
> Guenter Roeck writes:
> 
>> On 9/3/20 6:47 AM, Lars Povlsen wrote:
>>> If the temperature is read before the internal calibration is
>>> completed, the driver returns -EIO. Instead it should return -EAGAIN
>>> to encourage repeating the operation.
>>>
>>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>>> ---
>>>  drivers/hwmon/sparx5-temp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
>>> index 1a2b1026b026..98be48e3a22a 100644
>>> --- a/drivers/hwmon/sparx5-temp.c
>>> +++ b/drivers/hwmon/sparx5-temp.c
>>> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>>>       case hwmon_temp_input:
>>>               stat = readl_relaxed(hwmon->base + TEMP_STAT);
>>>               if (!(stat & TEMP_STAT_VALID))
>>> -                     return -EIO;
>>> +                     return -EAGAIN;
>>
>> The problem is that this may result in a hard loop and effectively hang the system,
>> or at least the calling process. It may be better to return -ENODATA in this situation.
>>
> 
> Guenther,
> 
> Thank you for your response.
> 
> The motivation for the patch was as I tried to use thermal support to
> control a fan, and I noticed an initial quip from
> thermal_core.c:update_temperature() - it will throw a dev_warn() for
> anything else than an "EAGAIN". The EAGAIN also seems to be used by
> several other hwmon drivers in similar situations.
> 
> The bottom line is that I would like to get rid of the warning
> message. Changing the error code to ENODATA does not fix what I am
> trying to accomplish.
> 

Sigh. I know that -EAGAIN is used, but it has its own problems - userspace
is inclined to retry directly, causing hard loops.

Ok, lets go with -EAGAIN then. Can't be helped. I'll apply your patch.

Guenter

> Do you have any suggestions on how to achieve the objective?
> 
> Sincerely,
> 
> ---Lars
> 
>> Guenter
>>
>>>               value = stat & TEMP_STAT_TEMP;
>>>               /*
>>>                * From register documentation:
>>> --
>>> 2.27.0
>>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
  2020-09-03 13:47 ` Lars Povlsen
@ 2020-09-04 16:40   ` Guenter Roeck
  -1 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-09-04 16:40 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: Jean Delvare, Microchip Linux Driver Support, linux-hwmon,
	linux-arm-kernel, linux-kernel

On Thu, Sep 03, 2020 at 03:47:04PM +0200, Lars Povlsen wrote:
> If the temperature is read before the internal calibration is
> completed, the driver returns -EIO. Instead it should return -EAGAIN
> to encourage repeating the operation.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>

Applied. I added a note stating that I would prefer -ENODATA, and why,
but that this is not feasible due to thermal subsystem requirements. 

Thanks,
Guenter

> ---
>  drivers/hwmon/sparx5-temp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --
> 2.27.0
> 
> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
> index 1a2b1026b026..98be48e3a22a 100644
> --- a/drivers/hwmon/sparx5-temp.c
> +++ b/drivers/hwmon/sparx5-temp.c
> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>  	case hwmon_temp_input:
>  		stat = readl_relaxed(hwmon->base + TEMP_STAT);
>  		if (!(stat & TEMP_STAT_VALID))
> -			return -EIO;
> +			return -EAGAIN;
>  		value = stat & TEMP_STAT_TEMP;
>  		/*
>  		 * From register documentation:

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] hwmon: sparx5: Fix initial reading of temperature
@ 2020-09-04 16:40   ` Guenter Roeck
  0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-09-04 16:40 UTC (permalink / raw)
  To: Lars Povlsen
  Cc: linux-hwmon, Jean Delvare, linux-kernel, linux-arm-kernel,
	Microchip Linux Driver Support

On Thu, Sep 03, 2020 at 03:47:04PM +0200, Lars Povlsen wrote:
> If the temperature is read before the internal calibration is
> completed, the driver returns -EIO. Instead it should return -EAGAIN
> to encourage repeating the operation.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>

Applied. I added a note stating that I would prefer -ENODATA, and why,
but that this is not feasible due to thermal subsystem requirements. 

Thanks,
Guenter

> ---
>  drivers/hwmon/sparx5-temp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --
> 2.27.0
> 
> diff --git a/drivers/hwmon/sparx5-temp.c b/drivers/hwmon/sparx5-temp.c
> index 1a2b1026b026..98be48e3a22a 100644
> --- a/drivers/hwmon/sparx5-temp.c
> +++ b/drivers/hwmon/sparx5-temp.c
> @@ -56,7 +56,7 @@ static int s5_read(struct device *dev, enum hwmon_sensor_types type,
>  	case hwmon_temp_input:
>  		stat = readl_relaxed(hwmon->base + TEMP_STAT);
>  		if (!(stat & TEMP_STAT_VALID))
> -			return -EIO;
> +			return -EAGAIN;
>  		value = stat & TEMP_STAT_TEMP;
>  		/*
>  		 * From register documentation:

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-04 16:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-03 13:47 [PATCH] hwmon: sparx5: Fix initial reading of temperature Lars Povlsen
2020-09-03 13:47 ` Lars Povlsen
2020-09-03 13:52 ` Guenter Roeck
2020-09-03 13:52   ` Guenter Roeck
2020-09-04  8:33   ` Lars Povlsen
2020-09-04  8:33     ` Lars Povlsen
2020-09-04 14:08     ` Guenter Roeck
2020-09-04 14:08       ` Guenter Roeck
2020-09-04 16:40 ` Guenter Roeck
2020-09-04 16:40   ` Guenter Roeck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.