date:20161208

[PATCH v2] hwmon: (adm1026) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Guenter Roeck

Fix overflows seen when writing large values into voltage limit,
temperature limit, temperature offset, and DAC attributes.

Overflows are seen due to unbound multiplications and additions.

While at it, change the low temperature limit to -128 degrees C,
since this is the minimum temperature accepted by the chip.

Signed-off-by: Guenter Roeck 
---
v2: Provide reason for changing lower temperature range.

 drivers/hwmon/adm1026.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/adm1026.c b/drivers/hwmon/adm1026.c
index e67b9a50ac7c..b2a5d9e5c590 100644
--- a/drivers/hwmon/adm1026.c
+++ b/drivers/hwmon/adm1026.c
@@ -197,8 +197,9 @@ static int adm1026_scaling[] = { /* .001 Volts */
};
 #define NEG12_OFFSET  16000
 #define SCALE(val, from, to) (((val)*(to) + ((from)/2))/(from))
-#define INS_TO_REG(n, val)  (clamp_val(SCALE(val, adm1026_scaling[n], 192),\
-   0, 255))
+#define INS_TO_REG(n, val) \
+   SCALE(clamp_val(val, 0, 255 * adm1026_scaling[n] / 192), \
+ adm1026_scaling[n], 192)
 #define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))
 
 /*
@@ -215,11 +216,11 @@ static int adm1026_scaling[] = { /* .001 Volts */
 #define DIV_TO_REG(val) ((val) >= 8 ? 3 : (val) >= 4 ? 2 : (val) >= 2 ? 1 : 0)
 
 /* Temperature is reported in 1 degC increments */
-#define TEMP_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \
-   / 1000, -127, 127))
+#define TEMP_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), \
+  1000)
 #define TEMP_FROM_REG(val) ((val) * 1000)
-#define OFFSET_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \
- / 1000, -127, 127))
+#define OFFSET_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), \
+1000)
 #define OFFSET_FROM_REG(val) ((val) * 1000)
 
 #define PWM_TO_REG(val) (clamp_val(val, 0, 255))
@@ -233,7 +234,8 @@ static int adm1026_scaling[] = { /* .001 Volts */
  *   indicates that the DAC could be used to drive the fans, but in our
  *   example board (Arima HDAMA) it isn't connected to the fans at all.
  */
-#define DAC_TO_REG(val) (clamp_val(val) * 255) + 500) / 2500), 0, 255))
+#define DAC_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, 0, 2500) * 255, \
+ 2500)
 #define DAC_FROM_REG(val) (((val) * 2500) / 255)
 
 /*
@@ -593,7 +595,10 @@ static ssize_t set_in16_min(struct device *dev, struct 
device_attribute *attr,
return err;
 
mutex_lock(>update_lock);
-   data->in_min[16] = INS_TO_REG(16, val + NEG12_OFFSET);
+   data->in_min[16] = INS_TO_REG(16,
+ clamp_val(val, INT_MIN,
+   INT_MAX - NEG12_OFFSET) +
+ NEG12_OFFSET);
adm1026_write_value(client, ADM1026_REG_IN_MIN[16], data->in_min[16]);
mutex_unlock(>update_lock);
return count;
@@ -618,7 +623,10 @@ static ssize_t set_in16_max(struct device *dev, struct 
device_attribute *attr,
return err;
 
mutex_lock(>update_lock);
-   data->in_max[16] = INS_TO_REG(16, val+NEG12_OFFSET);
+   data->in_max[16] = INS_TO_REG(16,
+ clamp_val(val, INT_MIN,
+   INT_MAX - NEG12_OFFSET) +
+ NEG12_OFFSET);
adm1026_write_value(client, ADM1026_REG_IN_MAX[16], data->in_max[16]);
mutex_unlock(>update_lock);
return count;
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/17] hwmon: (adt7470) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Guenter Roeck

On Thu, Dec 08, 2016 at 04:14:06PM +0100, Jean Delvare wrote:
> On Sun,  4 Dec 2016 20:55:31 -0800, Guenter Roeck wrote:
> > Fix overflows seen when writing large values into various temperature limit
> > attributes.
> > 
> > The input value passed to DIC_ROUND_CLOSEST() needs to be clamped to avoid
> > such overflows.
> > 
> > Signed-off-by: Guenter Roeck 
> > ---
> >  drivers/hwmon/adt7470.c | 9 +++--
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> > index 6e60ca53406e..8996120b8170 100644
> > --- a/drivers/hwmon/adt7470.c
> > +++ b/drivers/hwmon/adt7470.c
> > @@ -483,8 +483,7 @@ static ssize_t set_temp_min(struct device *dev,
> > if (kstrtol(buf, 10, ))
> > return -EINVAL;
> >  
> > -   temp = DIV_ROUND_CLOSEST(temp, 1000);
> > -   temp = clamp_val(temp, -128, 127);
> > +   temp = DIV_ROUND_CLOSEST(clamp_val(temp, -128000, 127000), 1000);
> >  
> > mutex_lock(>lock);
> > data->temp_min[attr->index] = temp;
> > @@ -517,8 +516,7 @@ static ssize_t set_temp_max(struct device *dev,
> > if (kstrtol(buf, 10, ))
> > return -EINVAL;
> >  
> > -   temp = DIV_ROUND_CLOSEST(temp, 1000);
> > -   temp = clamp_val(temp, -128, 127);
> > +   temp = DIV_ROUND_CLOSEST(clamp_val(temp, -128000, 127000), 1000);
> >  
> > mutex_lock(>lock);
> > data->temp_max[attr->index] = temp;
> > @@ -880,8 +878,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
> > if (kstrtol(buf, 10, ))
> > return -EINVAL;
> >  
> > -   temp = DIV_ROUND_CLOSEST(temp, 1000);
> > -   temp = clamp_val(temp, -128, 127);
> > +   temp = DIV_ROUND_CLOSEST(clamp_val(temp, -128000, 127000), 1000);
> >  
> > mutex_lock(>lock);
> > data->pwm_tmin[attr->index] = temp;
> 
> Seems more readable on 2 lines, but other than this:
> 
You are right. Split into two lines.

> Reviewed-by: Jean Delvare 
> 
Thanks a lot for the review!

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/17] hwmon: (adm1026) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Guenter Roeck


Hi Jean,

On 12/08/2016 06:33 AM, Jean Delvare wrote:

On Sun,  4 Dec 2016 20:55:29 -0800, Guenter Roeck wrote:

Fix overflows seen when writing large values into voltage limit,
temperature limit, temperature offset, and DAC attributes.

Overflows are seen due to unbound multiplications and additions.

Signed-off-by: Guenter Roeck 
---
 drivers/hwmon/adm1026.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/adm1026.c b/drivers/hwmon/adm1026.c
index e67b9a50ac7c..b2a5d9e5c590 100644
--- a/drivers/hwmon/adm1026.c
+++ b/drivers/hwmon/adm1026.c
@@ -197,8 +197,9 @@ static int adm1026_scaling[] = { /* .001 Volts */
};
 #define NEG12_OFFSET  16000
 #define SCALE(val, from, to) (((val)*(to) + ((from)/2))/(from))
-#define INS_TO_REG(n, val)  (clamp_val(SCALE(val, adm1026_scaling[n], 192),\
-   0, 255))
+#define INS_TO_REG(n, val) \
+   SCALE(clamp_val(val, 0, 255 * adm1026_scaling[n] / 192), \
+ adm1026_scaling[n], 192)
 #define INS_FROM_REG(n, val) (SCALE(val, 192, adm1026_scaling[n]))

 /*
@@ -215,11 +216,11 @@ static int adm1026_scaling[] = { /* .001 Volts */
 #define DIV_TO_REG(val) ((val) >= 8 ? 3 : (val) >= 4 ? 2 : (val) >= 2 ? 1 : 0)

 /* Temperature is reported in 1 degC increments */
-#define TEMP_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \
-   / 1000, -127, 127))
+#define TEMP_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), \
+  1000)
 #define TEMP_FROM_REG(val) ((val) * 1000)
-#define OFFSET_TO_REG(val) (clamp_val(((val) + ((val) < 0 ? -500 : 500)) \
- / 1000, -127, 127))
+#define OFFSET_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), \
+1000)


Sorry for nitpicking but the original code had -127 °C as the negative
limit. You are changing it to -128 °C without a justification. If it
matters, it should be at least documented in the commit message. If
not, it should be left as it was.


I had seen -128 as value reported by the chip with one of my register dumps,
which messes up my module tests because it tries to rewrite the value it read
into the attribute, and that fails. Also, the datasheet lists -128 degrees C
as a valid register value.

This is one of those philosophical questions, for which I don't have a really
good answer. Should we clamp to the register limits or to the chip 
specification ?
I tend to clamp to register limits, because I think that it should always be 
possible
to write back what was read, but we are highly inconsistent in the various 
drivers.
Any opinion ?


 #define OFFSET_FROM_REG(val) ((val) * 1000)

 #define PWM_TO_REG(val) (clamp_val(val, 0, 255))
@@ -233,7 +234,8 @@ static int adm1026_scaling[] = { /* .001 Volts */
  *   indicates that the DAC could be used to drive the fans, but in our
  *   example board (Arima HDAMA) it isn't connected to the fans at all.
  */
-#define DAC_TO_REG(val) (clamp_val(val) * 255) + 500) / 2500), 0, 255))
+#define DAC_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, 0, 2500) * 255, \
+ 2500)
 #define DAC_FROM_REG(val) (((val) * 2500) / 255)

 /*
@@ -593,7 +595,10 @@ static ssize_t set_in16_min(struct device *dev, struct 
device_attribute *attr,
return err;

mutex_lock(>update_lock);
-   data->in_min[16] = INS_TO_REG(16, val + NEG12_OFFSET);
+   data->in_min[16] = INS_TO_REG(16,
+ clamp_val(val, INT_MIN,
+   INT_MAX - NEG12_OFFSET) +
+ NEG12_OFFSET);
adm1026_write_value(client, ADM1026_REG_IN_MIN[16], data->in_min[16]);
mutex_unlock(>update_lock);
return count;
@@ -618,7 +623,10 @@ static ssize_t set_in16_max(struct device *dev, struct 
device_attribute *attr,
return err;

mutex_lock(>update_lock);
-   data->in_max[16] = INS_TO_REG(16, val+NEG12_OFFSET);
+   data->in_max[16] = INS_TO_REG(16,
+ clamp_val(val, INT_MIN,
+   INT_MAX - NEG12_OFFSET) +
+ NEG12_OFFSET);
adm1026_write_value(client, ADM1026_REG_IN_MAX[16], data->in_max[16]);
mutex_unlock(>update_lock);
return count;


On these code paths, you end up calling clamp_val() twice. This could
certainly be avoided, but I'm too lazy to do the math ;-)


I know. Problem here is that there are two overflows, one in the calling code
(since it does arithmetic on the input value), and another one in INS_TO_REG().
When I wrote this, I didn't have a good idea how to avoid the first overflow
without a second clamp.

One possibility would be to define INS_TO_REG_NOCLAMP(). Would that be worth it

Re: [PATCH 07/17] hwmon: (adt7462) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Jean Delvare

On Sun,  4 Dec 2016 20:55:30 -0800, Guenter Roeck wrote:
> Fix overflows seen when writing large values into temperature limit,
> voltage limit, and pwm hysteresis attributes.
> 
> The input parameter to DIV_ROUND_CLOSEST() needs to be clamped to avoid
> such overflows.
> 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/hwmon/adt7462.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7462.c b/drivers/hwmon/adt7462.c
> index 5929e126da63..0e9c8d3e8f5c 100644
> --- a/drivers/hwmon/adt7462.c
> +++ b/drivers/hwmon/adt7462.c
> @@ -810,8 +810,7 @@ static ssize_t set_temp_min(struct device *dev,
>   if (kstrtol(buf, 10, ) || !temp_enabled(data, attr->index))
>   return -EINVAL;
>  
> - temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
> - temp = clamp_val(temp, 0, 255);
> + temp = DIV_ROUND_CLOSEST(clamp_val(temp, -64000, 191000), 1000) + 64;
>  
>   mutex_lock(>lock);
>   data->temp_min[attr->index] = temp;
> @@ -848,8 +847,7 @@ static ssize_t set_temp_max(struct device *dev,
>   if (kstrtol(buf, 10, ) || !temp_enabled(data, attr->index))
>   return -EINVAL;
>  
> - temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
> - temp = clamp_val(temp, 0, 255);
> + temp = DIV_ROUND_CLOSEST(clamp_val(temp, -64000, 191000), 1000) + 64;
>  
>   mutex_lock(>lock);
>   data->temp_max[attr->index] = temp;

The original code had the division and the clamping on separate lines,
and I believe it makes the code more readable. It would also make the
patch smaller and more readable as well.

> @@ -912,6 +910,7 @@ static ssize_t set_volt_max(struct device *dev,
>   if (kstrtol(buf, 10, ) || !x)
>   return -EINVAL;
>  
> + temp = clamp_val(temp, 0, INT_MAX / 1000 - x);
>   temp *= 1000; /* convert mV to uV */
>   temp = DIV_ROUND_CLOSEST(temp, x);
>   temp = clamp_val(temp, 0, 255);
> @@ -954,6 +953,7 @@ static ssize_t set_volt_min(struct device *dev,
>   if (kstrtol(buf, 10, ) || !x)
>   return -EINVAL;
>  
> + temp = clamp_val(temp, 0, INT_MAX / 1000 - x);
>   temp *= 1000; /* convert mV to uV */
>   temp = DIV_ROUND_CLOSEST(temp, x);
>   temp = clamp_val(temp, 0, 255);

Any chance to get the new clamp_val()s such that the second ones are no
longer needed?

> @@ -1220,8 +1220,7 @@ static ssize_t set_pwm_hyst(struct device *dev,
>   if (kstrtol(buf, 10, ))
>   return -EINVAL;
>  
> - temp = DIV_ROUND_CLOSEST(temp, 1000);
> - temp = clamp_val(temp, 0, 15);
> + temp = DIV_ROUND_CLOSEST(clamp_val(temp, 0, 15000), 1000);
>  
>   /* package things up */
>   temp &= ADT7462_PWM_HYST_MASK;
> @@ -1306,8 +1305,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
>   if (kstrtol(buf, 10, ))
>   return -EINVAL;
>  
> - temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
> - temp = clamp_val(temp, 0, 255);
> + temp = DIV_ROUND_CLOSEST(clamp_val(temp, -64000, 191000), 1000) + 64;
>  
>   mutex_lock(>lock);
>   data->pwm_tmin[attr->index] = temp;

Same comment as first 2.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/17] hwmon: (adm9240) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Guenter Roeck


On 12/08/2016 05:29 AM, Jean Delvare wrote:

Hi Guenter,

On Sun,  4 Dec 2016 20:55:24 -0800, Guenter Roeck wrote:

Module test reports:

in0_min: Suspected overflow: [3320 vs. 0]
in0_max: Suspected overflow: [3320 vs. 0]
in4_min: Suspected overflow: [15938 vs. 0]
in4_max: Suspected overflow: [15938 vs. 0]
temp1_max: Suspected overflow: [127000 vs. 0]
temp1_max_hyst: Suspected overflow: [127000 vs. 0]
aout_output: Suspected overflow: [1250 vs. 0]

Code analysis reveals that the overflows are caused by conversions
from unsigned long to long to int, combined with multiplications on
passed values.

Signed-off-by: Guenter Roeck 
---
 drivers/hwmon/adm9240.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 2fe1828bd10b..347afacedcf5 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -98,12 +98,14 @@ static inline unsigned int IN_FROM_REG(u8 reg, int n)

 static inline u8 IN_TO_REG(unsigned long val, int n)
 {
+   val = clamp_val(val, 0, INT_MAX / 192 - 12000);
return clamp_val(SCALE(val, 192, nom_mv[n]), 0, 255);
 }


I understand the idea of clamping before the conversion to avoid the
overflow. However I would have hoped that clamping the input makes
clamping the output unneeded. Clamping is full of tests, which aren't
cheap as they break the CPU instruction prediction, so we should not
abuse it.


I am not that much concerned about this here, since the limits are not
usually set continuously. I agree though, since it is always better
to keep the code as simple as possible.


Would the following work?

static inline u8 IN_TO_REG(unsigned long val, int n)
{
val = clamp_val(val, 0, nom_mv[n] * 255 / 192);
return SCALE(val, 192, nom_mv[n]);
}

This should be more compact and faster.



 /* temperature range: -40..125, 127 disables temperature alarm */
 static inline s8 TEMP_TO_REG(long val)
 {
+   val = clamp_val(val, INT_MIN + 1000, INT_MAX - 1000);
return clamp_val(SCALE(val, 1, 1000), -40, 127);
 }

@@ -122,6 +124,7 @@ static inline unsigned int FAN_FROM_REG(u8 reg, u8 div)
 /* analog out 0..1250mV */
 static inline u8 AOUT_TO_REG(unsigned long val)
 {
+   val = clamp_val(val, 0, INT_MAX / 255 - 1250);
return clamp_val(SCALE(val, 255, 1250), 0, 255);
 }



Same comment and same suggested solution for these two functions:

/* temperature range: -40..125, 127 disables temperature alarm */
static inline s8 TEMP_TO_REG(long val)
{
val = clamp_val(val, -4, 127000);
return SCALE(val, 1, 1000);
}

/* analog out 0..1250mV */
static inline u8 AOUT_TO_REG(unsigned long val)
{
val = clamp_val(val, 0, 1250);
return SCALE(val, 255, 1250);
}


Should work. I'll give it a try.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/17] hwmon: (adm1025) Fix overflows seen when writing voltage limits

2016-12-08 Thread Jean Delvare

On Sun,  4 Dec 2016 20:55:28 -0800, Guenter Roeck wrote:
> Writes into voltage limit attributes can overflow due to an unbound
> multiplication.
> 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/hwmon/adm1025.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/adm1025.c b/drivers/hwmon/adm1025.c
> index d6c767ace916..1abb4609b412 100644
> --- a/drivers/hwmon/adm1025.c
> +++ b/drivers/hwmon/adm1025.c
> @@ -93,7 +93,7 @@ static const int in_scale[6] = { 2500, 2250, 3300, 5000, 
> 12000, 3300 };
>  
>  #define IN_FROM_REG(reg, scale)  (((reg) * (scale) + 96) / 192)
>  #define IN_TO_REG(val, scale)((val) <= 0 ? 0 : \
> -  (val) * 192 >= (scale) * 255 ? 255 : \
> +  (val) >= (scale) * 255 / 192 ? 255 : \
>((val) * 192 + (scale) / 2) / (scale))
>  
>  #define TEMP_FROM_REG(reg)   ((reg) * 1000)

Reviewed-by: Jean Delvare 

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/17] hwmon: (smsc47m192) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Jean Delvare

On Sun,  4 Dec 2016 20:55:27 -0800, Guenter Roeck wrote:
> Module test reports overflows when writing into temperature and voltage
> limit attributes
> 
> temp1_min: Suspected overflow: [127000 vs. 0]
> temp1_max: Suspected overflow: [127000 vs. 0]
> temp1_offset: Suspected overflow: [127000 vs. 0]
> temp2_min: Suspected overflow: [127000 vs. 0]
> temp2_max: Suspected overflow: [127000 vs. 0]
> temp2_offset: Suspected overflow: [127000 vs. 0]
> temp3_min: Suspected overflow: [127000 vs. 0]
> temp3_max: Suspected overflow: [127000 vs. 0]
> temp3_offset: Suspected overflow: [127000 vs. 0]
> in0_min: Suspected overflow: [3320 vs. 0]
> in0_max: Suspected overflow: [3320 vs. 0]
> in4_min: Suspected overflow: [15938 vs. 0]
> in4_max: Suspected overflow: [15938 vs. 0]
> in6_min: Suspected overflow: [1992 vs. 0]
> in6_max: Suspected overflow: [1992 vs. 0]
> in7_min: Suspected overflow: [2391 vs. 0]
> in7_max: Suspected overflow: [2391 vs. 0]
> 
> The problem is caused by conversions from unsigned long to long and
> from long to int.
> 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/hwmon/smsc47m192.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/smsc47m192.c b/drivers/hwmon/smsc47m192.c
> index 6ac7cda72d4c..202e167d6a59 100644
> --- a/drivers/hwmon/smsc47m192.c
> +++ b/drivers/hwmon/smsc47m192.c
> @@ -77,6 +77,7 @@ static inline unsigned int IN_FROM_REG(u8 reg, int n)
>  
>  static inline u8 IN_TO_REG(unsigned long val, int n)
>  {
> + val = clamp_val(val, 0, 65535);
>   return clamp_val(SCALE(val, 192, nom_mv[n]), 0, 255);
>  }

Same as for adm9240, I would suggest to remove the second clamp_val():

static inline u8 IN_TO_REG(unsigned long val, int n)
{
val = clamp_val(val, 0, nom_mv[n] * 255 / 192);
return SCALE(val, 192, nom_mv[n]);
}

>  
> @@ -84,7 +85,7 @@ static inline u8 IN_TO_REG(unsigned long val, int n)
>   * TEMP: 0.001 degC units (-128C to +127C)
>   * REG: 1C/bit, two's complement
>   */
> -static inline s8 TEMP_TO_REG(int val)
> +static inline s8 TEMP_TO_REG(long val)
>  {
>   return SCALE(clamp_val(val, -128000, 127000), 1, 1000);
>  }

Other than this:

Reviewed-by: Jean Delvare 

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/17] hwmon: (ds620) Fix overflows seen when writing temperature limits

2016-12-08 Thread Jean Delvare

On Sun,  4 Dec 2016 20:55:25 -0800, Guenter Roeck wrote:
> Module test reports:
> 
> temp1_max: Suspected overflow: [16 vs. 0]
> temp1_min: Suspected overflow: [16 vs. 0]
> 
> This is seen because the values passed when writing temperature limits
> are unbound.
> 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/hwmon/ds620.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/ds620.c b/drivers/hwmon/ds620.c
> index edf550fc4eef..0043a4c02b85 100644
> --- a/drivers/hwmon/ds620.c
> +++ b/drivers/hwmon/ds620.c
> @@ -166,7 +166,7 @@ static ssize_t set_temp(struct device *dev, struct 
> device_attribute *da,
>   if (res)
>   return res;
>  
> - val = (val * 10 / 625) * 8;
> + val = (clamp_val(val, -128000, 128000) * 10 / 625) * 8;
>  
>   mutex_lock(>update_lock);
>   data->temp[attr->index] = val;

Reviewed-by: Jean Delvare 
Fixes: 6099469805c2 ("hwmon: Support for Dallas Semiconductor DS620")

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/17] hwmon: (adm9240) Fix overflows seen when writing into limit attributes

2016-12-08 Thread Jean Delvare

Hi Guenter,

On Sun,  4 Dec 2016 20:55:24 -0800, Guenter Roeck wrote:
> Module test reports:
> 
> in0_min: Suspected overflow: [3320 vs. 0]
> in0_max: Suspected overflow: [3320 vs. 0]
> in4_min: Suspected overflow: [15938 vs. 0]
> in4_max: Suspected overflow: [15938 vs. 0]
> temp1_max: Suspected overflow: [127000 vs. 0]
> temp1_max_hyst: Suspected overflow: [127000 vs. 0]
> aout_output: Suspected overflow: [1250 vs. 0]
> 
> Code analysis reveals that the overflows are caused by conversions
> from unsigned long to long to int, combined with multiplications on
> passed values.
> 
> Signed-off-by: Guenter Roeck 
> ---
>  drivers/hwmon/adm9240.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
> index 2fe1828bd10b..347afacedcf5 100644
> --- a/drivers/hwmon/adm9240.c
> +++ b/drivers/hwmon/adm9240.c
> @@ -98,12 +98,14 @@ static inline unsigned int IN_FROM_REG(u8 reg, int n)
>  
>  static inline u8 IN_TO_REG(unsigned long val, int n)
>  {
> + val = clamp_val(val, 0, INT_MAX / 192 - 12000);
>   return clamp_val(SCALE(val, 192, nom_mv[n]), 0, 255);
>  }

I understand the idea of clamping before the conversion to avoid the
overflow. However I would have hoped that clamping the input makes
clamping the output unneeded. Clamping is full of tests, which aren't
cheap as they break the CPU instruction prediction, so we should not
abuse it.

Would the following work?

static inline u8 IN_TO_REG(unsigned long val, int n)
{
val = clamp_val(val, 0, nom_mv[n] * 255 / 192);
return SCALE(val, 192, nom_mv[n]);
}

This should be more compact and faster.

>  
>  /* temperature range: -40..125, 127 disables temperature alarm */
>  static inline s8 TEMP_TO_REG(long val)
>  {
> + val = clamp_val(val, INT_MIN + 1000, INT_MAX - 1000);
>   return clamp_val(SCALE(val, 1, 1000), -40, 127);
>  }
>  
> @@ -122,6 +124,7 @@ static inline unsigned int FAN_FROM_REG(u8 reg, u8 div)
>  /* analog out 0..1250mV */
>  static inline u8 AOUT_TO_REG(unsigned long val)
>  {
> + val = clamp_val(val, 0, INT_MAX / 255 - 1250);
>   return clamp_val(SCALE(val, 255, 1250), 0, 255);
>  }
>  

Same comment and same suggested solution for these two functions:

/* temperature range: -40..125, 127 disables temperature alarm */
static inline s8 TEMP_TO_REG(long val)
{
val = clamp_val(val, -4, 127000);
return SCALE(val, 1, 1000);
}

/* analog out 0..1250mV */
static inline u8 AOUT_TO_REG(unsigned long val)
{
val = clamp_val(val, 0, 1250);
return SCALE(val, 255, 1250);
}


-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] hwmon: (adm1026) Fix overflows seen when writing into limit attributes

Re: [PATCH 08/17] hwmon: (adt7470) Fix overflows seen when writing into limit attributes

Re: [PATCH 06/17] hwmon: (adm1026) Fix overflows seen when writing into limit attributes

Re: [PATCH 07/17] hwmon: (adt7462) Fix overflows seen when writing into limit attributes

Re: [PATCH 01/17] hwmon: (adm9240) Fix overflows seen when writing into limit attributes

Re: [PATCH 05/17] hwmon: (adm1025) Fix overflows seen when writing voltage limits

Re: [PATCH 04/17] hwmon: (smsc47m192) Fix overflows seen when writing into limit attributes

Re: [PATCH 02/17] hwmon: (ds620) Fix overflows seen when writing temperature limits

Re: [PATCH 01/17] hwmon: (adm9240) Fix overflows seen when writing into limit attributes

9 matches

Site Navigation

Mail list logo

Footer information