* [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set
@ 2023-02-17 23:16 Enrico Mioso
2023-02-18 0:25 ` Guenter Roeck
2023-02-18 1:45 ` Andrew Lunn
0 siblings, 2 replies; 6+ messages in thread
From: Enrico Mioso @ 2023-02-17 23:16 UTC (permalink / raw)
To: linux-hwmon; +Cc: Enrico Mioso, Andrew Lunn, Russell King
React like otherdrivers do when an out-of-range value is passed from hwmon
core.
Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com>
CC: Andrew Lunn <andrew@lunn.ch>
CC: Russell King <linux@armlinux.org.uk>
---
I implemented this patch based on observing how other drivers are reacting,
and after experiencing the hwmon core passing -INT MAX. Based on a
cursory look at the hwmon code, I don't believe this to be a bug. If this is
instead the case, I hope I will be corrected and this patch rejected.
---
drivers/net/phy/aquantia_hwmon.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
index 19c4c280a6cd..6444055e720c 100644
--- a/drivers/net/phy/aquantia_hwmon.c
+++ b/drivers/net/phy/aquantia_hwmon.c
@@ -70,8 +70,7 @@ static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
{
int temp;
- if (value >= 128000 || value < -128000)
- return -ERANGE;
+ clamp_val(value, -128000, 128000);
temp = value * 256 / 1000;
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set
2023-02-17 23:16 [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set Enrico Mioso
@ 2023-02-18 0:25 ` Guenter Roeck
2023-02-18 1:45 ` Andrew Lunn
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2023-02-18 0:25 UTC (permalink / raw)
To: Enrico Mioso; +Cc: linux-hwmon, Andrew Lunn, Russell King
On Sat, Feb 18, 2023 at 12:16:47AM +0100, Enrico Mioso wrote:
> React like otherdrivers do when an out-of-range value is passed from hwmon
> core.
>
This is not an appropriate patch description.
> Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com>
> CC: Andrew Lunn <andrew@lunn.ch>
> CC: Russell King <linux@armlinux.org.uk>
> ---
> I implemented this patch based on observing how other drivers are reacting,
> and after experiencing the hwmon core passing -INT MAX. Based on a
I don't understand "after experiencing the hwmon core passing -INT MAX".
> cursory look at the hwmon code, I don't believe this to be a bug. If this is
> instead the case, I hope I will be corrected and this patch rejected.
The bug, if anything, in the current code is that it should return -EINVAL.
-ERANGE is "Math result not representable" which doesn't make sense here.
Other than that, it is up to the driver author to decide if they want to
return an error in this situation or if they want to clamp the range.
We recommend to clamp because users won't normally know the valid range,
but, again, it is up to the driver author to decide if they want to burden
users with figuring out the valid range.
Guenter
> ---
> drivers/net/phy/aquantia_hwmon.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
> index 19c4c280a6cd..6444055e720c 100644
> --- a/drivers/net/phy/aquantia_hwmon.c
> +++ b/drivers/net/phy/aquantia_hwmon.c
> @@ -70,8 +70,7 @@ static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
> {
> int temp;
>
> - if (value >= 128000 || value < -128000)
> - return -ERANGE;
> + clamp_val(value, -128000, 128000);
>
> temp = value * 256 / 1000;
>
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set
2023-02-17 23:16 [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set Enrico Mioso
2023-02-18 0:25 ` Guenter Roeck
@ 2023-02-18 1:45 ` Andrew Lunn
2023-02-18 14:42 ` Guenter Roeck
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2023-02-18 1:45 UTC (permalink / raw)
To: Enrico Mioso; +Cc: linux-hwmon, Russell King
> diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
> index 19c4c280a6cd..6444055e720c 100644
> --- a/drivers/net/phy/aquantia_hwmon.c
> +++ b/drivers/net/phy/aquantia_hwmon.c
> @@ -70,8 +70,7 @@ static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
> {
> int temp;
>
> - if (value >= 128000 || value < -128000)
> - return -ERANGE;
> + clamp_val(value, -128000, 128000);
It could be argued that value < -128000 should return
-EUNOBTAINABLE. I've had trouble getting DRAMS to work at -40C, even
those listed as industrial. Setting an alarm for -128C is pointless.
+128C is also a bit questionable. The aQuantia PHYs do run hot, you
often see a heat sink, and they are supposed to support up to 108C. So
an alarm for 128C probably also does not work.
Anyway, as Guenter suggested, please change -ERANGE to -EINVAL.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set
2023-02-18 1:45 ` Andrew Lunn
@ 2023-02-18 14:42 ` Guenter Roeck
2023-02-18 20:50 ` Enrico Mioso
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2023-02-18 14:42 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Enrico Mioso, linux-hwmon, Russell King
On Sat, Feb 18, 2023 at 02:45:43AM +0100, Andrew Lunn wrote:
> > diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
> > index 19c4c280a6cd..6444055e720c 100644
> > --- a/drivers/net/phy/aquantia_hwmon.c
> > +++ b/drivers/net/phy/aquantia_hwmon.c
> > @@ -70,8 +70,7 @@ static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
> > {
> > int temp;
> >
> > - if (value >= 128000 || value < -128000)
> > - return -ERANGE;
> > + clamp_val(value, -128000, 128000);
>
> It could be argued that value < -128000 should return
> -EUNOBTAINABLE. I've had trouble getting DRAMS to work at -40C, even
Hmm, I don't see that one anywhere.
> those listed as industrial. Setting an alarm for -128C is pointless.
>
> +128C is also a bit questionable. The aQuantia PHYs do run hot, you
> often see a heat sink, and they are supposed to support up to 108C. So
> an alarm for 128C probably also does not work.
>
It has been an endless argument if limits should be clamped to the
limits supported by the chip registers or to the limits supported by
the chip according to its datasheet. I personally see that as the
responsibility of the person actually configuring the limits, not the
driver. After all, there could be chip variants supporting different
limits, and who knows what chip variants are going to be available
in the future. Anyway, as I said, this has been an endless argument,
and it is another detail that I usually let driver developers decide
because they often feel passionate about it.
Guenter
> Anyway, as Guenter suggested, please change -ERANGE to -EINVAL.
>
> Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set
2023-02-18 14:42 ` Guenter Roeck
@ 2023-02-18 20:50 ` Enrico Mioso
2023-02-20 17:14 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Enrico Mioso @ 2023-02-18 20:50 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Andrew Lunn, linux-hwmon, Russell King
[-- Attachment #1: Type: text/plain, Size: 2272 bytes --]
On Sat, 18 Feb 2023, Guenter Roeck wrote:
> Date: Sat, 18 Feb 2023 15:42:34
> From: Guenter Roeck <linux@roeck-us.net>
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Enrico Mioso <mrkiko.rs@gmail.com>, linux-hwmon@vger.kernel.org,
> Russell King <linux@armlinux.org.uk>
> Subject: Re: [PATCH] net: phy: aquantia: clamp temperature value in
> aqr_hwmon_set
>
> On Sat, Feb 18, 2023 at 02:45:43AM +0100, Andrew Lunn wrote:
>>> diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
>>> index 19c4c280a6cd..6444055e720c 100644
>>> --- a/drivers/net/phy/aquantia_hwmon.c
>>> +++ b/drivers/net/phy/aquantia_hwmon.c
>>> @@ -70,8 +70,7 @@ static int aqr_hwmon_set(struct phy_device *phydev, int reg, long value)
>>> {
>>> int temp;
>>>
>>> - if (value >= 128000 || value < -128000)
>>> - return -ERANGE;
>>> + clamp_val(value, -128000, 128000);
>>
>> It could be argued that value < -128000 should return
>> -EUNOBTAINABLE. I've had trouble getting DRAMS to work at -40C, even
>
> Hmm, I don't see that one anywhere.
>
>> those listed as industrial. Setting an alarm for -128C is pointless.
>>
>> +128C is also a bit questionable. The aQuantia PHYs do run hot, you
>> often see a heat sink, and they are supposed to support up to 108C. So
>> an alarm for 128C probably also does not work.
>>
>
> It has been an endless argument if limits should be clamped to the
> limits supported by the chip registers or to the limits supported by
> the chip according to its datasheet. I personally see that as the
> responsibility of the person actually configuring the limits, not the
> driver. After all, there could be chip variants supporting different
> limits, and who knows what chip variants are going to be available
> in the future. Anyway, as I said, this has been an endless argument,
> and it is another detail that I usually let driver developers decide
> because they often feel passionate about it.
>
> Guenter
>
>> Anyway, as Guenter suggested, please change -ERANGE to -EINVAL.
>>
>> Andrew
>
Thanks a lot for your review and feedback.
I will send a new patch then, simply changing -ERANGE to -EINVAL.
Onanother side - if I want to set those limits to -10 °C and +90 °C, may you suggest me a DTS snippet? Thanks,
Enrico
Enrico
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set
2023-02-18 20:50 ` Enrico Mioso
@ 2023-02-20 17:14 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2023-02-20 17:14 UTC (permalink / raw)
To: Enrico Mioso; +Cc: Andrew Lunn, linux-hwmon, Russell King
On Sat, Feb 18, 2023 at 09:50:29PM +0100, Enrico Mioso wrote:
>
> Onanother side - if I want to set those limits to -10 °C and +90 °C, may you suggest me a DTS snippet? Thanks,
> Enrico
>
That would hae to be supported by the driver.
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-20 17:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17 23:16 [PATCH] net: phy: aquantia: clamp temperature value in aqr_hwmon_set Enrico Mioso
2023-02-18 0:25 ` Guenter Roeck
2023-02-18 1:45 ` Andrew Lunn
2023-02-18 14:42 ` Guenter Roeck
2023-02-18 20:50 ` Enrico Mioso
2023-02-20 17:14 ` 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).