| Message ID | 20230217231647.968107-1-mrkiko.rs@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | net: phy: aquantia: clamp temperature value in aqr_hwmon_set | expand |
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 >
> 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
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
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
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
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;
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(-)