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

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

[Guenter Roeck]

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 <linux@roeck-us.net>
---
 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);
 }
 
 /* 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);
 }
 
-- 
2.5.0

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

[Guenter Roeck]

Module test reports:

temp1_max: Suspected overflow: [160000 vs. 0]
temp1_min: Suspected overflow: [160000 vs. 0]

This is seen because the values passed when writing temperature limits
are unbound.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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(&data->update_lock);
 	data->temp[attr->index] = val;
-- 
2.5.0

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

[Jean Delvare]

On Sun,  4 Dec 2016 20:55:25 -0800, Guenter Roeck wrote:
> Module test reports:
> 
> temp1_max: Suspected overflow: [160000 vs. 0]
> temp1_min: Suspected overflow: [160000 vs. 0]
> 
> This is seen because the values passed when writing temperature limits
> are unbound.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  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(&data->update_lock);
>  	data->temp[attr->index] = val;

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Fixes: 6099469805c2 ("hwmon: Support for Dallas Semiconductor DS620")

-- 
Jean Delvare
SUSE L3 Support

[PATCH 03/17] hwmon: (lm93) Fix overflows seen when writing into limit attributes

[Guenter Roeck]

Module test shows a large number of overflows, caused by missing clamps
as well as various conversions between variable types.

Also fix temperature calculations for hysteresis and offset registers.
For those, temperature calculations were a mix of millisecond and second
based, causing reported and accepted hysteresis and offset temperatures
to be widely off target.

This also changes the offset and base temperature attributes to be
officially reported and set in milli-degrees C. This was already the case
for the base temperature attribute, even though it was documented to be
reported and set in degrees C.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/hwmon/lm93 | 26 ++++++++++++-------------
 drivers/hwmon/lm93.c     | 49 +++++++++++++++++++++++++-----------------------
 2 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/Documentation/hwmon/lm93 b/Documentation/hwmon/lm93
index f3b2ad2ceb01..7bda7f0291e4 100644
--- a/Documentation/hwmon/lm93
+++ b/Documentation/hwmon/lm93
@@ -174,25 +174,25 @@ a "0" disables it. The h/w default is 0x0f (all temperatures bound).
 
 The function y = f(x) takes a source temperature x to a PWM output y.  This
 function of the LM93 is derived from a base temperature and a table of 12
-temperature offsets.  The base temperature is expressed in degrees C in the
-sysfs files temp<n>_auto_base.  The offsets are expressed in cumulative
-degrees C, with the value of offset <i> for temperature value <n> being
+temperature offsets.  The base temperature is expressed in milli-degrees C in
+the sysfs files temp<n>_auto_base.  The offsets are expressed in cumulative
+milli-degrees C, with the value of offset <i> for temperature value <n> being
 contained in the file temp<n>_auto_offset<i>.  E.g. if the base temperature
 is 40C:
 
      offset #	temp<n>_auto_offset<i>	range		pwm
 	 1		0		-		 25.00%
 	 2		0		-		 28.57%
-	 3		1		40C - 41C	 32.14%
-	 4		1		41C - 42C	 35.71%
-	 5		2		42C - 44C	 39.29%
-	 6		2		44C - 46C	 42.86%
-	 7		2		48C - 50C	 46.43%
-	 8		2		50C - 52C	 50.00%
-	 9		2		52C - 54C	 53.57%
-	10		2		54C - 56C	 57.14%
-	11		2		56C - 58C	 71.43%
-	12		2		58C - 60C	 85.71%
+	 3		500		40C - 41C	 32.14%
+	 4		500		41C - 42C	 35.71%
+	 5		1000		42C - 44C	 39.29%
+	 6		1000		44C - 46C	 42.86%
+	 7		1000		48C - 50C	 46.43%
+	 8		1000		50C - 52C	 50.00%
+	 9		1000		52C - 54C	 53.57%
+	10		1000		54C - 56C	 57.14%
+	11		1000		56C - 58C	 71.43%
+	12		1000		58C - 60C	 85.71%
 					> 60C		100.00%
 
 Valid offsets are in the range 0C <= x <= 7.5C in 0.5C increments.
diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
index 90bb04858117..7b3152368e3b 100644
--- a/drivers/hwmon/lm93.c
+++ b/drivers/hwmon/lm93.c
@@ -368,7 +368,7 @@ static unsigned LM93_IN_FROM_REG(int nr, u8 reg)
  * IN: mV, limits determined by channel nr
  * REG: scaling determined by channel nr
  */
-static u8 LM93_IN_TO_REG(int nr, unsigned val)
+static u8 LM93_IN_TO_REG(int nr, unsigned long val)
 {
 	/* range limit */
 	const long mv = clamp_val(val,
@@ -407,9 +407,13 @@ static unsigned LM93_IN_REL_FROM_REG(u8 reg, int upper, int vid)
  * upper also determines which nibble of the register is returned
  * (the other nibble will be 0x0)
  */
-static u8 LM93_IN_REL_TO_REG(unsigned val, int upper, int vid)
+static u8 LM93_IN_REL_TO_REG(unsigned long val, int upper, int vid)
 {
-	long uv_offset = vid * 1000 - val * 10000;
+	long uv_offset;
+
+	val = clamp_val(val, 0, 1000000);
+	uv_offset = vid * 1000 - val * 10000;
+
 	if (upper) {
 		uv_offset = clamp_val(uv_offset, 12500, 200000);
 		return (u8)((uv_offset /  12500 - 1) << 4);
@@ -453,28 +457,23 @@ static int LM93_TEMP_OFFSET_MODE_FROM_REG(u8 sfc2, int nr)
  * This function is common to all 4-bit temperature offsets
  * reg is 4 bits right justified
  * mode 0 => 1C/bit, mode !0 => 0.5C/bit
+ * Return value in milli-degrees C.
  */
 static int LM93_TEMP_OFFSET_FROM_REG(u8 reg, int mode)
 {
-	return (reg & 0x0f) * (mode ? 5 : 10);
+	return (reg & 0x0f) * (mode ? 500 : 1000);
 }
 
-#define LM93_TEMP_OFFSET_MIN  (0)
-#define LM93_TEMP_OFFSET_MAX0 (150)
-#define LM93_TEMP_OFFSET_MAX1 (75)
-
 /*
  * This function is common to all 4-bit temperature offsets
  * returns 4 bits right justified
  * mode 0 => 1C/bit, mode !0 => 0.5C/bit
+ * Input value is in milli-degrees C.
  */
-static u8 LM93_TEMP_OFFSET_TO_REG(int off, int mode)
+static u8 LM93_TEMP_OFFSET_TO_REG(unsigned long off, int mode)
 {
-	int factor = mode ? 5 : 10;
-
-	off = clamp_val(off, LM93_TEMP_OFFSET_MIN,
-		mode ? LM93_TEMP_OFFSET_MAX1 : LM93_TEMP_OFFSET_MAX0);
-	return (u8)((off + factor/2) / factor);
+	off = clamp_val(off, 0, mode ? 7500 : 15000);
+	return DIV_ROUND_CLOSEST(off, mode ? 500 : 1000);
 }
 
 /* 0 <= nr <= 3 */
@@ -494,7 +493,8 @@ static int LM93_TEMP_AUTO_OFFSET_FROM_REG(u8 reg, int nr, int mode)
  * REG: 1.0C/bit (mode 0) or 0.5C/bit (mode non-zero)
  * 0 <= nr <= 3
  */
-static u8 LM93_TEMP_AUTO_OFFSET_TO_REG(u8 old, int off, int nr, int mode)
+static u8 LM93_TEMP_AUTO_OFFSET_TO_REG(u8 old, unsigned long off, int nr,
+				       int mode)
 {
 	u8 new = LM93_TEMP_OFFSET_TO_REG(off, mode);
 
@@ -532,10 +532,13 @@ static int LM93_AUTO_BOOST_HYST_FROM_REGS(struct lm93_data *data, int nr,
 			LM93_TEMP_OFFSET_FROM_REG(reg, mode);
 }
 
-static u8 LM93_AUTO_BOOST_HYST_TO_REG(struct lm93_data *data, long hyst,
-		int nr, int mode)
+static u8 LM93_AUTO_BOOST_HYST_TO_REG(struct lm93_data *data,
+				      long hyst, int nr, int mode)
 {
-	u8 reg = LM93_TEMP_OFFSET_TO_REG(
+	u8 reg;
+
+	hyst = clamp_val(hyst, LM93_TEMP_MIN, LM93_TEMP_MAX);
+	reg = LM93_TEMP_OFFSET_TO_REG(
 			(LM93_TEMP_FROM_REG(data->boost[nr]) - hyst), mode);
 
 	switch (nr) {
@@ -592,7 +595,7 @@ static int LM93_PWM_FROM_REG(u8 reg, enum pwm_freq freq)
 }
 
 /* round up to nearest match */
-static u8 LM93_PWM_TO_REG(int pwm, enum pwm_freq freq)
+static u8 LM93_PWM_TO_REG(unsigned long pwm, enum pwm_freq freq)
 {
 	int i;
 	for (i = 0; i < 13; i++)
@@ -613,7 +616,7 @@ static int LM93_FAN_FROM_REG(u16 regs)
  * RPM: (82.5 to 1350000)
  * REG: 14-bits, LE, *left* justified
  */
-static u16 LM93_FAN_TO_REG(long rpm)
+static u16 LM93_FAN_TO_REG(unsigned long rpm)
 {
 	u16 count, regs;
 
@@ -642,7 +645,7 @@ static int LM93_PWM_FREQ_FROM_REG(u8 reg)
 }
 
 /* round up to nearest match */
-static u8 LM93_PWM_FREQ_TO_REG(int freq)
+static u8 LM93_PWM_FREQ_TO_REG(unsigned long freq)
 {
 	int i;
 	for (i = 7; i > 0; i--)
@@ -1471,10 +1474,10 @@ static ssize_t store_temp_auto_boost_hyst(struct device *dev,
 	int nr = (to_sensor_dev_attr(attr))->index;
 	struct lm93_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	unsigned long val;
+	long val;
 	int err;
 
-	err = kstrtoul(buf, 10, &val);
+	err = kstrtol(buf, 10, &val);
 	if (err)
 		return err;
 
-- 
2.5.0

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

[Jean Delvare]

Hi Guetner,

I kept the more tasty one for last ;-)

On Sun,  4 Dec 2016 20:55:26 -0800, Guenter Roeck wrote:
> Module test shows a large number of overflows, caused by missing clamps
> as well as various conversions between variable types.
> 
> Also fix temperature calculations for hysteresis and offset registers.
> For those, temperature calculations were a mix of millisecond and second
> based, causing reported and accepted hysteresis and offset temperatures
> to be widely off target.
> 
> This also changes the offset and base temperature attributes to be
> officially reported and set in milli-degrees C. This was already the case
> for the base temperature attribute, even though it was documented to be
> reported and set in degrees C.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  Documentation/hwmon/lm93 | 26 ++++++++++++-------------
>  drivers/hwmon/lm93.c     | 49 +++++++++++++++++++++++++-----------------------
>  2 files changed, 39 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/hwmon/lm93 b/Documentation/hwmon/lm93
> index f3b2ad2ceb01..7bda7f0291e4 100644
> --- a/Documentation/hwmon/lm93
> +++ b/Documentation/hwmon/lm93
> @@ -174,25 +174,25 @@ a "0" disables it. The h/w default is 0x0f (all temperatures bound).
>  
>  The function y = f(x) takes a source temperature x to a PWM output y.  This
>  function of the LM93 is derived from a base temperature and a table of 12
> -temperature offsets.  The base temperature is expressed in degrees C in the
> -sysfs files temp<n>_auto_base.  The offsets are expressed in cumulative
> -degrees C, with the value of offset <i> for temperature value <n> being
> +temperature offsets.  The base temperature is expressed in milli-degrees C in
> +the sysfs files temp<n>_auto_base.  The offsets are expressed in cumulative
> +milli-degrees C, with the value of offset <i> for temperature value <n> being
>  contained in the file temp<n>_auto_offset<i>.  E.g. if the base temperature
>  is 40C:
>  
>       offset #	temp<n>_auto_offset<i>	range		pwm
>  	 1		0		-		 25.00%
>  	 2		0		-		 28.57%
> -	 3		1		40C - 41C	 32.14%
> -	 4		1		41C - 42C	 35.71%
> -	 5		2		42C - 44C	 39.29%
> -	 6		2		44C - 46C	 42.86%
> -	 7		2		48C - 50C	 46.43%
> -	 8		2		50C - 52C	 50.00%
> -	 9		2		52C - 54C	 53.57%
> -	10		2		54C - 56C	 57.14%
> -	11		2		56C - 58C	 71.43%
> -	12		2		58C - 60C	 85.71%
> +	 3		500		40C - 41C	 32.14%
> +	 4		500		41C - 42C	 35.71%
> +	 5		1000		42C - 44C	 39.29%
> +	 6		1000		44C - 46C	 42.86%
> +	 7		1000		48C - 50C	 46.43%
> +	 8		1000		50C - 52C	 50.00%
> +	 9		1000		52C - 54C	 53.57%
> +	10		1000		54C - 56C	 57.14%
> +	11		1000		56C - 58C	 71.43%
> +	12		1000		58C - 60C	 85.71%
>  					> 60C		100.00%

I'm a bit confused, I would have expected 1000 and 2000 for
temp<n>_auto_offset<i>, not 500 and 1000? Otherwise I can't see how you
get the announced ranges.

>  
>  Valid offsets are in the range 0C <= x <= 7.5C in 0.5C increments.
> diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
> index 90bb04858117..7b3152368e3b 100644
> --- a/drivers/hwmon/lm93.c
> +++ b/drivers/hwmon/lm93.c
> (...)

Code changes all look good to me, good job. Hairy code :-/

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

On 12/13/2016 06:01 AM, Jean Delvare wrote:
> Hi Guetner,
>
> I kept the more tasty one for last ;-)
>
> On Sun,  4 Dec 2016 20:55:26 -0800, Guenter Roeck wrote:
>> Module test shows a large number of overflows, caused by missing clamps
>> as well as various conversions between variable types.
>>
>> Also fix temperature calculations for hysteresis and offset registers.
>> For those, temperature calculations were a mix of millisecond and second
>> based, causing reported and accepted hysteresis and offset temperatures
>> to be widely off target.
>>
>> This also changes the offset and base temperature attributes to be
>> officially reported and set in milli-degrees C. This was already the case
>> for the base temperature attribute, even though it was documented to be
>> reported and set in degrees C.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  Documentation/hwmon/lm93 | 26 ++++++++++++-------------
>>  drivers/hwmon/lm93.c     | 49 +++++++++++++++++++++++++-----------------------
>>  2 files changed, 39 insertions(+), 36 deletions(-)
>>
>> diff --git a/Documentation/hwmon/lm93 b/Documentation/hwmon/lm93
>> index f3b2ad2ceb01..7bda7f0291e4 100644
>> --- a/Documentation/hwmon/lm93
>> +++ b/Documentation/hwmon/lm93
>> @@ -174,25 +174,25 @@ a "0" disables it. The h/w default is 0x0f (all temperatures bound).
>>
>>  The function y = f(x) takes a source temperature x to a PWM output y.  This
>>  function of the LM93 is derived from a base temperature and a table of 12
>> -temperature offsets.  The base temperature is expressed in degrees C in the
>> -sysfs files temp<n>_auto_base.  The offsets are expressed in cumulative
>> -degrees C, with the value of offset <i> for temperature value <n> being
>> +temperature offsets.  The base temperature is expressed in milli-degrees C in
>> +the sysfs files temp<n>_auto_base.  The offsets are expressed in cumulative
>> +milli-degrees C, with the value of offset <i> for temperature value <n> being
>>  contained in the file temp<n>_auto_offset<i>.  E.g. if the base temperature
>>  is 40C:
>>
>>       offset #	temp<n>_auto_offset<i>	range		pwm
>>  	 1		0		-		 25.00%
>>  	 2		0		-		 28.57%
>> -	 3		1		40C - 41C	 32.14%
>> -	 4		1		41C - 42C	 35.71%
>> -	 5		2		42C - 44C	 39.29%
>> -	 6		2		44C - 46C	 42.86%
>> -	 7		2		48C - 50C	 46.43%
>> -	 8		2		50C - 52C	 50.00%
>> -	 9		2		52C - 54C	 53.57%
>> -	10		2		54C - 56C	 57.14%
>> -	11		2		56C - 58C	 71.43%
>> -	12		2		58C - 60C	 85.71%
>> +	 3		500		40C - 41C	 32.14%
>> +	 4		500		41C - 42C	 35.71%
>> +	 5		1000		42C - 44C	 39.29%
>> +	 6		1000		44C - 46C	 42.86%
>> +	 7		1000		48C - 50C	 46.43%
>> +	 8		1000		50C - 52C	 50.00%
>> +	 9		1000		52C - 54C	 53.57%
>> +	10		1000		54C - 56C	 57.14%
>> +	11		1000		56C - 58C	 71.43%
>> +	12		1000		58C - 60C	 85.71%
>>  					> 60C		100.00%
>
> I'm a bit confused, I would have expected 1000 and 2000 for
> temp<n>_auto_offset<i>, not 500 and 1000? Otherwise I can't see how you
> get the announced ranges.
>
Let me look into it again. The driver has some other issues
with voltage limit ranges which I'll need to address as well.
I'll hold this patch until after the rc - no need to hurry.

>>
>>  Valid offsets are in the range 0C <= x <= 7.5C in 0.5C increments.
>> diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
>> index 90bb04858117..7b3152368e3b 100644
>> --- a/drivers/hwmon/lm93.c
>> +++ b/drivers/hwmon/lm93.c
>> (...)
>
> Code changes all look good to me, good job. Hairy code :-/
>
It most definitely is. Thanks!

Guenter

> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

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

[Guenter Roeck]

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 <linux@roeck-us.net>
---
 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);
 }
 
@@ -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);
 }
-- 
2.5.0

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

[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 <linux@roeck-us.net>
> ---
>  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 <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

On Thu, Dec 08, 2016 at 02:57:00PM +0100, Jean Delvare wrote:
> 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 <linux@roeck-us.net>
> > ---
> >  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]);
> }
> 
I updated the patch accordingly.

Thanks,
Guenter

> >  
> > @@ -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 <jdelvare@suse.de>
> 
> -- 
> Jean Delvare
> SUSE L3 Support
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

Writes into voltage limit attributes can overflow due to an unbound
multiplication.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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)
-- 
2.5.0

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

[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 <linux@roeck-us.net>
> ---
>  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 <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

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

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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(&data->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(&data->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(&data->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(&data->update_lock);
 	return count;
-- 
2.5.0

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

[Jean Delvare]

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 <linux@roeck-us.net>
> ---
>  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.

>  #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(&data->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(&data->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(&data->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(&data->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 ;-)

-- 
Jean Delvare
SUSE L3 Support

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

[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 <linux@roeck-us.net>
>> ---
>>  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(&data->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(&data->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(&data->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(&data->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 ?

Thanks,
Guenter

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

[Jean Delvare]

Hi Guenter,

On Thu, 8 Dec 2016 07:34:35 -0800, Guenter Roeck wrote:
> On 12/08/2016 06:33 AM, Jean Delvare wrote:
> > On Sun,  4 Dec 2016 20:55:29 -0800, Guenter Roeck wrote:
> >> @@ -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.

OK, I understand.

> 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 ?

I have noticed that inconsistency too. Given that we do not have
attributes to expose the limits to user-space, I consider it a nice
feature to clamp the requested limits to what the chip is actually
specified for. But I don't have a strong opinion on it either, and am
not going to spend time "fixing" all drivers not doing it. And you have
a point with being able to write back what was read, especially if the
power-on value isn't withing the specified limits.

> >> (...)
> >> @@ -593,7 +595,10 @@ static ssize_t set_in16_min(struct device *dev, struct device_attribute *attr,
> >>  		return err;
> >>
> >>  	mutex_lock(&data->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(&data->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(&data->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(&data->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 ?

No, don't bother. God only knows if there's any user left for this
driver ;-)

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

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 <linux@roeck-us.net>
---
 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) || !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(&data->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) || !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(&data->lock);
 	data->temp_max[attr->index] = temp;
@@ -912,6 +910,7 @@ static ssize_t set_volt_max(struct device *dev,
 	if (kstrtol(buf, 10, &temp) || !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, &temp) || !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);
@@ -1220,8 +1220,7 @@ static ssize_t set_pwm_hyst(struct device *dev,
 	if (kstrtol(buf, 10, &temp))
 		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, &temp))
 		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(&data->lock);
 	data->pwm_tmin[attr->index] = temp;
-- 
2.5.0

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

[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 <linux@roeck-us.net>
> ---
>  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) || !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(&data->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) || !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(&data->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, &temp) || !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, &temp) || !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, &temp))
>  		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, &temp))
>  		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(&data->lock);
>  	data->pwm_tmin[attr->index] = temp;

Same comment as first 2.

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

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 <linux@roeck-us.net>
---
 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, &temp))
 		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(&data->lock);
 	data->temp_min[attr->index] = temp;
@@ -517,8 +516,7 @@ static ssize_t set_temp_max(struct device *dev,
 	if (kstrtol(buf, 10, &temp))
 		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(&data->lock);
 	data->temp_max[attr->index] = temp;
@@ -880,8 +878,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
 	if (kstrtol(buf, 10, &temp))
 		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(&data->lock);
 	data->pwm_tmin[attr->index] = temp;
-- 
2.5.0

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

[Jean Delvare]

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 <linux@roeck-us.net>
> ---
>  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, &temp))
>  		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(&data->lock);
>  	data->temp_min[attr->index] = temp;
> @@ -517,8 +516,7 @@ static ssize_t set_temp_max(struct device *dev,
>  	if (kstrtol(buf, 10, &temp))
>  		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(&data->lock);
>  	data->temp_max[attr->index] = temp;
> @@ -880,8 +878,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
>  	if (kstrtol(buf, 10, &temp))
>  		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(&data->lock);
>  	data->pwm_tmin[attr->index] = temp;

Seems more readable on 2 lines, but other than this:

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[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 <linux@roeck-us.net>
> > ---
> >  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, &temp))
> >  		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(&data->lock);
> >  	data->temp_min[attr->index] = temp;
> > @@ -517,8 +516,7 @@ static ssize_t set_temp_max(struct device *dev,
> >  	if (kstrtol(buf, 10, &temp))
> >  		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(&data->lock);
> >  	data->temp_max[attr->index] = temp;
> > @@ -880,8 +878,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
> >  	if (kstrtol(buf, 10, &temp))
> >  		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(&data->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 <jdelvare@suse.de>
> 
Thanks a lot for the review!

Guenter

[PATCH 09/17] hwmon: (nct7802) Fix overflows seen when writing into limit attributes

[Guenter Roeck]

Fix overflows seen when writing voltage and temperature limit attributes.

The value passed to DIV_ROUND_CLOSEST() needs to be clamped.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/nct7802.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index 3ce33d244cc0..6fe71cfe0320 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -326,8 +326,9 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index,
 	int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr];
 	int err;
 
-	voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]);
-	voltage = clamp_val(voltage, 0, 0x3ff);
+	voltage = DIV_ROUND_CLOSEST(clamp_val(voltage, 0,
+					      0x3ff * nct7802_vmul[nr]),
+				    nct7802_vmul[nr]);
 
 	mutex_lock(&data->access_lock);
 	err = regmap_write(data->regmap,
@@ -402,7 +403,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr,
 	if (err < 0)
 		return err;
 
-	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
 
 	err = regmap_write(data->regmap, nr, val & 0xff);
 	return err ? : count;
-- 
2.5.0

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

[Jean Delvare]

Hi Guenter,

On Sun,  4 Dec 2016 20:55:32 -0800, Guenter Roeck wrote:
> Fix overflows seen when writing voltage and temperature limit attributes.
> 
> The value passed to DIV_ROUND_CLOSEST() needs to be clamped.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

I think you can also add:

Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y")

> ---
>  drivers/hwmon/nct7802.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
> index 3ce33d244cc0..6fe71cfe0320 100644
> --- a/drivers/hwmon/nct7802.c
> +++ b/drivers/hwmon/nct7802.c
> @@ -326,8 +326,9 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index,
>  	int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr];
>  	int err;
>  
> -	voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]);
> -	voltage = clamp_val(voltage, 0, 0x3ff);
> +	voltage = DIV_ROUND_CLOSEST(clamp_val(voltage, 0,
> +					      0x3ff * nct7802_vmul[nr]),
> +				    nct7802_vmul[nr]);

As for previous patches, I think separate lines make the code more
readable.

>  
>  	mutex_lock(&data->access_lock);
>  	err = regmap_write(data->regmap,
> @@ -402,7 +403,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr,
>  	if (err < 0)
>  		return err;
>  
> -	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);

You change the low limit from -128 to -127 without justification.

>  
>  	err = regmap_write(data->regmap, nr, val & 0xff);
>  	return err ? : count;

Looking at function nct7802_write_fan_min() I think an overflow can
happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val().
Any reason why you did not fix that one?

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

Hi Jean,

On 12/09/2016 01:49 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sun,  4 Dec 2016 20:55:32 -0800, Guenter Roeck wrote:
>> Fix overflows seen when writing voltage and temperature limit attributes.
>>
>> The value passed to DIV_ROUND_CLOSEST() needs to be clamped.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> I think you can also add:
>
> Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y")
>
>> ---
>>  drivers/hwmon/nct7802.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
>> index 3ce33d244cc0..6fe71cfe0320 100644
>> --- a/drivers/hwmon/nct7802.c
>> +++ b/drivers/hwmon/nct7802.c
>> @@ -326,8 +326,9 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index,
>>  	int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr];
>>  	int err;
>>
>> -	voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]);
>> -	voltage = clamp_val(voltage, 0, 0x3ff);
>> +	voltage = DIV_ROUND_CLOSEST(clamp_val(voltage, 0,
>> +					      0x3ff * nct7802_vmul[nr]),
>> +				    nct7802_vmul[nr]);
>
> As for previous patches, I think separate lines make the code more
> readable.
>
Ok

>>
>>  	mutex_lock(&data->access_lock);
>>  	err = regmap_write(data->regmap,
>> @@ -402,7 +403,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr,
>>  	if (err < 0)
>>  		return err;
>>
>> -	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
>> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
>
> You change the low limit from -128 to -127 without justification.
>
There is none. This is a bug.

>>
>>  	err = regmap_write(data->regmap, nr, val & 0xff);
>>  	return err ? : count;
>
> Looking at function nct7802_write_fan_min() I think an overflow can
> happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val().
> Any reason why you did not fix that one?
>
Not really. The call is
	DIV_ROUND_CLOSEST(1350000U, limit);
and thus won't overflow.

Thanks,
Guenter

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

[Jean Delvare]

On Fri, 9 Dec 2016 06:22:43 -0800, Guenter Roeck wrote:
> On 12/09/2016 01:49 AM, Jean Delvare wrote:
> > Looking at function nct7802_write_fan_min() I think an overflow can
> > happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val().
> > Any reason why you did not fix that one?
> >
> Not really. The call is
> 	DIV_ROUND_CLOSEST(1350000U, limit);
> and thus won't overflow.

Limit is originally parsed by kstrtoul into an unsigned long, however
the nct7802_write_fan_min function parameter is an unsigned int, so it
is implicitly cast to an unsigned int. On a 32-bit system, that may not
fit?

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

On Fri, Dec 09, 2016 at 04:25:48PM +0100, Jean Delvare wrote:
> On Fri, 9 Dec 2016 06:22:43 -0800, Guenter Roeck wrote:
> > On 12/09/2016 01:49 AM, Jean Delvare wrote:
> > > Looking at function nct7802_write_fan_min() I think an overflow can
> > > happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val().
> > > Any reason why you did not fix that one?
> > >
> > Not really. The call is
> > 	DIV_ROUND_CLOSEST(1350000U, limit);
> > and thus won't overflow.
> 
> Limit is originally parsed by kstrtoul into an unsigned long, however
> the nct7802_write_fan_min function parameter is an unsigned int, so it
> is implicitly cast to an unsigned int. On a 32-bit system, that may not
> fit?
> 
Good point. My module test doesn't catch that. Let me have a look
if I can somehow trigger that failure.

Thanks,
Guenter

[PATCH 10/17] hwmon: (lm87) Fix overflow seen when writing voltage limit attributes

[Guenter Roeck]

Writes into voltage limit attributes can overflow due to an unbound
multiplication.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm87.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
index a5e295826aea..24f971b75e46 100644
--- a/drivers/hwmon/lm87.c
+++ b/drivers/hwmon/lm87.c
@@ -121,7 +121,7 @@ static u8 LM87_REG_TEMP_LOW[3] = { 0x3A, 0x38, 0x2C };
 
 #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)
-- 
2.5.0

Re: [PATCH 10/17] hwmon: (lm87) Fix overflow seen when writing voltage limit attributes

[Jean Delvare]

On Sun,  4 Dec 2016 20:55:33 -0800, Guenter Roeck wrote:
> Writes into voltage limit attributes can overflow due to an unbound
> multiplication.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm87.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
> index a5e295826aea..24f971b75e46 100644
> --- a/drivers/hwmon/lm87.c
> +++ b/drivers/hwmon/lm87.c
> @@ -121,7 +121,7 @@ static u8 LM87_REG_TEMP_LOW[3] = { 0x3A, 0x38, 0x2C };
>  
>  #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 <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

[PATCH 11/17] hwmon: (lm85) Fix overflows seen when writing voltage limit attributes

[Guenter Roeck]

Writes into voltage limit attributes can overflow due to an unbound
multiplication.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm85.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c
index 6ff773fcaefb..29c8136ce9c5 100644
--- a/drivers/hwmon/lm85.c
+++ b/drivers/hwmon/lm85.c
@@ -136,7 +136,8 @@ static const int lm85_scaling[] = {  /* .001 Volts */
 #define SCALE(val, from, to)	(((val) * (to) + ((from) / 2)) / (from))
 
 #define INS_TO_REG(n, val)	\
-		clamp_val(SCALE(val, lm85_scaling[n], 192), 0, 255)
+		SCALE(clamp_val(val, 0, 255 * lm85_scaling[n] / 192), \
+		      lm85_scaling[n], 192)
 
 #define INSEXT_FROM_REG(n, val, ext)	\
 		SCALE(((val) << 4) + (ext), 192 << 4, lm85_scaling[n])
-- 
2.5.0

Re: [PATCH 11/17] hwmon: (lm85) Fix overflows seen when writing voltage limit attributes

[Jean Delvare]

On Sun,  4 Dec 2016 20:55:34 -0800, Guenter Roeck wrote:
> Writes into voltage limit attributes can overflow due to an unbound
> multiplication.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm85.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c
> index 6ff773fcaefb..29c8136ce9c5 100644
> --- a/drivers/hwmon/lm85.c
> +++ b/drivers/hwmon/lm85.c
> @@ -136,7 +136,8 @@ static const int lm85_scaling[] = {  /* .001 Volts */
>  #define SCALE(val, from, to)	(((val) * (to) + ((from) / 2)) / (from))
>  
>  #define INS_TO_REG(n, val)	\
> -		clamp_val(SCALE(val, lm85_scaling[n], 192), 0, 255)
> +		SCALE(clamp_val(val, 0, 255 * lm85_scaling[n] / 192), \
> +		      lm85_scaling[n], 192)
>  
>  #define INSEXT_FROM_REG(n, val, ext)	\
>  		SCALE(((val) << 4) + (ext), 192 << 4, lm85_scaling[n])

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

[PATCH 12/17] hwmon: (dme1737) Fix overflows seen when writing into limit attributes

[Guenter Roeck]

Writes into voltage limit, temperature limit, and temperature zone
attributes can overflow due to unchecked parameters to multiplications
and additions.

Cc: Juerg Haefliger <juergh@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/dme1737.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dme1737.c b/drivers/hwmon/dme1737.c
index 8763c4a8280c..29d082c12c74 100644
--- a/drivers/hwmon/dme1737.c
+++ b/drivers/hwmon/dme1737.c
@@ -279,7 +279,8 @@ static inline int IN_FROM_REG(int reg, int nominal, int res)
 
 static inline int IN_TO_REG(long val, int nominal)
 {
-	return clamp_val((val * 192 + nominal / 2) / nominal, 0, 255);
+	return DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * nominal / 192) * 192,
+				 nominal);
 }
 
 /*
@@ -295,7 +296,7 @@ static inline int TEMP_FROM_REG(int reg, int res)
 
 static inline int TEMP_TO_REG(long val)
 {
-	return clamp_val((val < 0 ? val - 500 : val + 500) / 1000, -128, 127);
+	return DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
 }
 
 /* Temperature range */
@@ -1028,6 +1029,8 @@ static ssize_t set_zone(struct device *dev, struct device_attribute *attr,
 	if (err)
 		return err;
 
+	val = clamp_val(val, -256000, 255000);
+
 	mutex_lock(&data->update_lock);
 	switch (fn) {
 	case SYS_ZONE_AUTO_POINT1_TEMP_HYST:
-- 
2.5.0

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

[Jean Delvare]

Hi Guenter,

On Sun,  4 Dec 2016 20:55:35 -0800, Guenter Roeck wrote:
> Writes into voltage limit, temperature limit, and temperature zone
> attributes can overflow due to unchecked parameters to multiplications
> and additions.
> 
> Cc: Juerg Haefliger <juergh@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/dme1737.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/dme1737.c b/drivers/hwmon/dme1737.c
> index 8763c4a8280c..29d082c12c74 100644
> --- a/drivers/hwmon/dme1737.c
> +++ b/drivers/hwmon/dme1737.c
> @@ -279,7 +279,8 @@ static inline int IN_FROM_REG(int reg, int nominal, int res)
>  
>  static inline int IN_TO_REG(long val, int nominal)
>  {
> -	return clamp_val((val * 192 + nominal / 2) / nominal, 0, 255);
> +	return DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * nominal / 192) * 192,
> +				 nominal);
>  }
>  
>  /*
> @@ -295,7 +296,7 @@ static inline int TEMP_FROM_REG(int reg, int res)
>  
>  static inline int TEMP_TO_REG(long val)
>  {
> -	return clamp_val((val < 0 ? val - 500 : val + 500) / 1000, -128, 127);
> +	return DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
>  }
>  
>  /* Temperature range */
> @@ -1028,6 +1029,8 @@ static ssize_t set_zone(struct device *dev, struct device_attribute *attr,
>  	if (err)
>  		return err;
>  
> +	val = clamp_val(val, -256000, 255000);

Where do these values come from? I would have naively expected the
auto-pwm temperature values to have the same range as the temperature
channels themselves.

Also in the case of SYS_ZONE_AUTO_POINT1_TEMP and
SYS_ZONE_AUTO_POINT3_TEMP, TEMP_TO_REG() is called, which already
performs the clamping, so it is redundant. So maybe it would be better
to have a "dedicated" clamp for the SYS_ZONE_AUTO_POINT1_TEMP_HYST and
SYS_ZONE_AUTO_POINT2_TEMP cases?

> +
>  	mutex_lock(&data->update_lock);
>  	switch (fn) {
>  	case SYS_ZONE_AUTO_POINT1_TEMP_HYST:


-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

On 12/12/2016 01:33 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sun,  4 Dec 2016 20:55:35 -0800, Guenter Roeck wrote:
>> Writes into voltage limit, temperature limit, and temperature zone
>> attributes can overflow due to unchecked parameters to multiplications
>> and additions.
>>
>> Cc: Juerg Haefliger <juergh@gmail.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  drivers/hwmon/dme1737.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/dme1737.c b/drivers/hwmon/dme1737.c
>> index 8763c4a8280c..29d082c12c74 100644
>> --- a/drivers/hwmon/dme1737.c
>> +++ b/drivers/hwmon/dme1737.c
>> @@ -279,7 +279,8 @@ static inline int IN_FROM_REG(int reg, int nominal, int res)
>>
>>  static inline int IN_TO_REG(long val, int nominal)
>>  {
>> -	return clamp_val((val * 192 + nominal / 2) / nominal, 0, 255);
>> +	return DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * nominal / 192) * 192,
>> +				 nominal);
>>  }
>>
>>  /*
>> @@ -295,7 +296,7 @@ static inline int TEMP_FROM_REG(int reg, int res)
>>
>>  static inline int TEMP_TO_REG(long val)
>>  {
>> -	return clamp_val((val < 0 ? val - 500 : val + 500) / 1000, -128, 127);
>> +	return DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
>>  }
>>
>>  /* Temperature range */
>> @@ -1028,6 +1029,8 @@ static ssize_t set_zone(struct device *dev, struct device_attribute *attr,
>>  	if (err)
>>  		return err;
>>
>> +	val = clamp_val(val, -256000, 255000);
>
> Where do these values come from? I would have naively expected the
> auto-pwm temperature values to have the same range as the temperature
> channels themselves.

Not related to real limits, just a "basic" clamp since the real clamp happens later
anyway.

>
> Also in the case of SYS_ZONE_AUTO_POINT1_TEMP and
> SYS_ZONE_AUTO_POINT3_TEMP, TEMP_TO_REG() is called, which already
> performs the clamping, so it is redundant. So maybe it would be better
> to have a "dedicated" clamp for the SYS_ZONE_AUTO_POINT1_TEMP_HYST and
> SYS_ZONE_AUTO_POINT2_TEMP cases?
>
Yrd, I noticed. Makes sense. Let me think about it.

Thanks,
Guenter

>> +
>>  	mutex_lock(&data->update_lock);
>>  	switch (fn) {
>>  	case SYS_ZONE_AUTO_POINT1_TEMP_HYST:
>
>

[PATCH 13/17] hwmon: (emc2103) Fix overflows seen when temperature limit attributes

[Guenter Roeck]

Writes into temperature limit attributes can overflow due to unbound
values passed to DIV_ROUND_CLOSEST().

Cc: Steve Glendinning <steve.glendinning@shawell.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/emc2103.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/emc2103.c b/drivers/hwmon/emc2103.c
index 24e395c5907d..4b870ee9b0d3 100644
--- a/drivers/hwmon/emc2103.c
+++ b/drivers/hwmon/emc2103.c
@@ -251,7 +251,7 @@ static ssize_t set_temp_min(struct device *dev, struct device_attribute *da,
 	if (result < 0)
 		return result;
 
-	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -63, 127);
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -63000, 127000), 1000);
 
 	mutex_lock(&data->update_lock);
 	data->temp_min[nr] = val;
@@ -273,7 +273,7 @@ static ssize_t set_temp_max(struct device *dev, struct device_attribute *da,
 	if (result < 0)
 		return result;
 
-	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -63, 127);
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -63000, 127000), 1000);
 
 	mutex_lock(&data->update_lock);
 	data->temp_max[nr] = val;
-- 
2.5.0

Re: [PATCH 13/17] hwmon: (emc2103) Fix overflows seen when temperature limit attributes

[Jean Delvare]

On Sun,  4 Dec 2016 20:55:36 -0800, Guenter Roeck wrote:
> Writes into temperature limit attributes can overflow due to unbound
> values passed to DIV_ROUND_CLOSEST().
> 
> Cc: Steve Glendinning <steve.glendinning@shawell.net>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/emc2103.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/emc2103.c b/drivers/hwmon/emc2103.c
> index 24e395c5907d..4b870ee9b0d3 100644
> --- a/drivers/hwmon/emc2103.c
> +++ b/drivers/hwmon/emc2103.c
> @@ -251,7 +251,7 @@ static ssize_t set_temp_min(struct device *dev, struct device_attribute *da,
>  	if (result < 0)
>  		return result;
>  
> -	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -63, 127);
> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -63000, 127000), 1000);
>  
>  	mutex_lock(&data->update_lock);
>  	data->temp_min[nr] = val;
> @@ -273,7 +273,7 @@ static ssize_t set_temp_max(struct device *dev, struct device_attribute *da,
>  	if (result < 0)
>  		return result;
>  
> -	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -63, 127);
> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -63000, 127000), 1000);
>  
>  	mutex_lock(&data->update_lock);
>  	data->temp_max[nr] = val;

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

[PATCH 14/17] hwmon: (emcw201) Fix overflows seen when writing into limit attributes

[Guenter Roeck]

Writes into temperature and voltage limit attributes can overflow
due to multiplications with unchecked parameters. Also, the input
parameter to DIV_ROUND_CLOSEST() needis to be range checked.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/emc6w201.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/emc6w201.c b/drivers/hwmon/emc6w201.c
index f37fe2011640..f8c776c49df9 100644
--- a/drivers/hwmon/emc6w201.c
+++ b/drivers/hwmon/emc6w201.c
@@ -215,12 +215,13 @@ static ssize_t set_in(struct device *dev, struct device_attribute *devattr,
 	if (err < 0)
 		return err;
 
-	val = DIV_ROUND_CLOSEST(val * 0xC0, nominal_mv[nr]);
+	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * nominal_mv[nr] / 192)
+				* 192, nominal_mv[nr]);
 	reg = (sf == min) ? EMC6W201_REG_IN_LOW(nr)
 			  : EMC6W201_REG_IN_HIGH(nr);
 
 	mutex_lock(&data->update_lock);
-	data->in[sf][nr] = clamp_val(val, 0, 255);
+	data->in[sf][nr] = val;
 	err = emc6w201_write8(client, reg, data->in[sf][nr]);
 	mutex_unlock(&data->update_lock);
 
@@ -252,12 +253,12 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
 	if (err < 0)
 		return err;
 
-	val = DIV_ROUND_CLOSEST(val, 1000);
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
 	reg = (sf == min) ? EMC6W201_REG_TEMP_LOW(nr)
 			  : EMC6W201_REG_TEMP_HIGH(nr);
 
 	mutex_lock(&data->update_lock);
-	data->temp[sf][nr] = clamp_val(val, -127, 127);
+	data->temp[sf][nr] = val;
 	err = emc6w201_write8(client, reg, data->temp[sf][nr]);
 	mutex_unlock(&data->update_lock);
 
-- 
2.5.0

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

[Jean Delvare]

On Sun,  4 Dec 2016 20:55:37 -0800, Guenter Roeck wrote:
> Writes into temperature and voltage limit attributes can overflow
> due to multiplications with unchecked parameters. Also, the input
> parameter to DIV_ROUND_CLOSEST() needis to be range checked.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/emc6w201.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/emc6w201.c b/drivers/hwmon/emc6w201.c
> index f37fe2011640..f8c776c49df9 100644
> --- a/drivers/hwmon/emc6w201.c
> +++ b/drivers/hwmon/emc6w201.c
> @@ -215,12 +215,13 @@ static ssize_t set_in(struct device *dev, struct device_attribute *devattr,
>  	if (err < 0)
>  		return err;
>  
> -	val = DIV_ROUND_CLOSEST(val * 0xC0, nominal_mv[nr]);
> +	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * nominal_mv[nr] / 192)
> +				* 192, nominal_mv[nr]);
>  	reg = (sf == min) ? EMC6W201_REG_IN_LOW(nr)
>  			  : EMC6W201_REG_IN_HIGH(nr);
>  
>  	mutex_lock(&data->update_lock);
> -	data->in[sf][nr] = clamp_val(val, 0, 255);
> +	data->in[sf][nr] = val;
>  	err = emc6w201_write8(client, reg, data->in[sf][nr]);
>  	mutex_unlock(&data->update_lock);
>  
> @@ -252,12 +253,12 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
>  	if (err < 0)
>  		return err;
>  
> -	val = DIV_ROUND_CLOSEST(val, 1000);
> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
>  	reg = (sf == min) ? EMC6W201_REG_TEMP_LOW(nr)
>  			  : EMC6W201_REG_TEMP_HIGH(nr);
>  
>  	mutex_lock(&data->update_lock);
> -	data->temp[sf][nr] = clamp_val(val, -127, 127);
> +	data->temp[sf][nr] = val;
>  	err = emc6w201_write8(client, reg, data->temp[sf][nr]);
>  	mutex_unlock(&data->update_lock);
>  

Keep on separate lines? Other than this, looks good:

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

On 12/12/2016 02:48 AM, Jean Delvare wrote:
> On Sun,  4 Dec 2016 20:55:37 -0800, Guenter Roeck wrote:
>> Writes into temperature and voltage limit attributes can overflow
>> due to multiplications with unchecked parameters. Also, the input
>> parameter to DIV_ROUND_CLOSEST() needis to be range checked.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  drivers/hwmon/emc6w201.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/emc6w201.c b/drivers/hwmon/emc6w201.c
>> index f37fe2011640..f8c776c49df9 100644
>> --- a/drivers/hwmon/emc6w201.c
>> +++ b/drivers/hwmon/emc6w201.c
>> @@ -215,12 +215,13 @@ static ssize_t set_in(struct device *dev, struct device_attribute *devattr,
>>  	if (err < 0)
>>  		return err;
>>
>> -	val = DIV_ROUND_CLOSEST(val * 0xC0, nominal_mv[nr]);
>> +	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * nominal_mv[nr] / 192)
>> +				* 192, nominal_mv[nr]);
>>  	reg = (sf == min) ? EMC6W201_REG_IN_LOW(nr)
>>  			  : EMC6W201_REG_IN_HIGH(nr);
>>
>>  	mutex_lock(&data->update_lock);
>> -	data->in[sf][nr] = clamp_val(val, 0, 255);
>> +	data->in[sf][nr] = val;
>>  	err = emc6w201_write8(client, reg, data->in[sf][nr]);
>>  	mutex_unlock(&data->update_lock);
>>
>> @@ -252,12 +253,12 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
>>  	if (err < 0)
>>  		return err;
>>
>> -	val = DIV_ROUND_CLOSEST(val, 1000);
>> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
>>  	reg = (sf == min) ? EMC6W201_REG_TEMP_LOW(nr)
>>  			  : EMC6W201_REG_TEMP_HIGH(nr);
>>
>>  	mutex_lock(&data->update_lock);
>> -	data->temp[sf][nr] = clamp_val(val, -127, 127);
>> +	data->temp[sf][nr] = val;
>>  	err = emc6w201_write8(client, reg, data->temp[sf][nr]);
>>  	mutex_unlock(&data->update_lock);
>>
>
> Keep on separate lines? Other than this, looks good:
>
Yes, makes sense. Done.

Thanks,
Guenter

> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

[PATCH 15/17] hwmln: (g762) Fix overflows and crash seen when writing limit attributes

[Guenter Roeck]

Fix overflows seen when writing into fan speed limit attributes.
Also fix crash due to division by zero, seen when certain very
large values (such as 4611686018427387904, or 0x4000000000000000)
are written into fan speed limit attributes.

Fixes: 594fbe713bf60 ("Add support for GMT G762/G763 PWM fan controllers")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/g762.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/g762.c b/drivers/hwmon/g762.c
index b96a2a9e4df7..584f9f755a92 100644
--- a/drivers/hwmon/g762.c
+++ b/drivers/hwmon/g762.c
@@ -193,12 +193,13 @@ static inline unsigned int rpm_from_cnt(u8 cnt, u32 clk_freq, u16 p,
  * Convert fan RPM value from sysfs into count value for fan controller
  * register (FAN_SET_CNT).
  */
-static inline unsigned char cnt_from_rpm(u32 rpm, u32 clk_freq, u16 p,
+static inline unsigned char cnt_from_rpm(unsigned long rpm, u32 clk_freq, u16 p,
 					 u8 clk_div, u8 gear_mult)
 {
-	if (!rpm)         /* to stop the fan, set cnt to 255 */
+	if (!rpm || !p || !clk_div)	/* to stop the fan, set cnt to 255 */
 		return 0xff;
 
+	rpm = clamp_val(rpm, 1, ULONG_MAX / (p * clk_div));
 	return clamp_val(((clk_freq * 30 * gear_mult) / (rpm * p * clk_div)),
 			 0, 255);
 }
-- 
2.5.0

Re: [PATCH 15/17] hwmln: (g762) Fix overflows and crash seen when writing limit attributes

[Jean Delvare]

Hi Guenter,

There's a typo in the subject (hwmln -> hwmon.)

On Sun,  4 Dec 2016 20:55:38 -0800, Guenter Roeck wrote:
> Fix overflows seen when writing into fan speed limit attributes.
> Also fix crash due to division by zero, seen when certain very
> large values (such as 4611686018427387904, or 0x4000000000000000)
> are written into fan speed limit attributes.
> 
> Fixes: 594fbe713bf60 ("Add support for GMT G762/G763 PWM fan controllers")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/g762.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/g762.c b/drivers/hwmon/g762.c
> index b96a2a9e4df7..584f9f755a92 100644
> --- a/drivers/hwmon/g762.c
> +++ b/drivers/hwmon/g762.c
> @@ -193,12 +193,13 @@ static inline unsigned int rpm_from_cnt(u8 cnt, u32 clk_freq, u16 p,
>   * Convert fan RPM value from sysfs into count value for fan controller
>   * register (FAN_SET_CNT).
>   */
> -static inline unsigned char cnt_from_rpm(u32 rpm, u32 clk_freq, u16 p,
> +static inline unsigned char cnt_from_rpm(unsigned long rpm, u32 clk_freq, u16 p,
>  					 u8 clk_div, u8 gear_mult)
>  {
> -	if (!rpm)         /* to stop the fan, set cnt to 255 */
> +	if (!rpm || !p || !clk_div)	/* to stop the fan, set cnt to 255 */
>  		return 0xff;

Looking at the definitions of G762_PULSE_FROM_REG and
G762_CLKDIV_FROM_REG, I can't see how either p or clk_div would be 0.
So this change seems unneeded to me. The division by zero you have seen
would be the consequence of an overflow (fixed below.)

>  
> +	rpm = clamp_val(rpm, 1, ULONG_MAX / (p * clk_div));
>  	return clamp_val(((clk_freq * 30 * gear_mult) / (rpm * p * clk_div)),
>  			 0, 255);
>  }

If my math is right, this can be simplified to:

	rpm = clamp_val(rpm, 1, (clk_freq * 30 * gear_mult) / (255  * p * clk_div));
	return (clk_freq * 30 * gear_mult) / (rpm * p * clk_div);

which may or may not actually be more simple ;-) as it avoids the
double clamping only at the cost of extra arithmetic. Probably only
worth it if we store the result of clk_freq * 30 * gear_mult and p *
clk_div locally so we don't have to compute them again.

Probably doesn't matter much in the end, so it's your call.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 15/17] hwmln: (g762) Fix overflows and crash seen when writing limit attributes

[Guenter Roeck]

Hi Jean,

On 12/12/2016 03:14 AM, Jean Delvare wrote:
> Hi Guenter,
>
> There's a typo in the subject (hwmln -> hwmon.)
>
> On Sun,  4 Dec 2016 20:55:38 -0800, Guenter Roeck wrote:
>> Fix overflows seen when writing into fan speed limit attributes.
>> Also fix crash due to division by zero, seen when certain very
>> large values (such as 4611686018427387904, or 0x4000000000000000)
>> are written into fan speed limit attributes.
>>
>> Fixes: 594fbe713bf60 ("Add support for GMT G762/G763 PWM fan controllers")
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  drivers/hwmon/g762.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwmon/g762.c b/drivers/hwmon/g762.c
>> index b96a2a9e4df7..584f9f755a92 100644
>> --- a/drivers/hwmon/g762.c
>> +++ b/drivers/hwmon/g762.c
>> @@ -193,12 +193,13 @@ static inline unsigned int rpm_from_cnt(u8 cnt, u32 clk_freq, u16 p,
>>   * Convert fan RPM value from sysfs into count value for fan controller
>>   * register (FAN_SET_CNT).
>>   */
>> -static inline unsigned char cnt_from_rpm(u32 rpm, u32 clk_freq, u16 p,
>> +static inline unsigned char cnt_from_rpm(unsigned long rpm, u32 clk_freq, u16 p,
>>  					 u8 clk_div, u8 gear_mult)
>>  {
>> -	if (!rpm)         /* to stop the fan, set cnt to 255 */
>> +	if (!rpm || !p || !clk_div)	/* to stop the fan, set cnt to 255 */
>>  		return 0xff;
>
> Looking at the definitions of G762_PULSE_FROM_REG and
> G762_CLKDIV_FROM_REG, I can't see how either p or clk_div would be 0.
> So this change seems unneeded to me. The division by zero you have seen
> would be the consequence of an overflow (fixed below.)
>
Yes, you are right. I dropped that part.

>>
>> +	rpm = clamp_val(rpm, 1, ULONG_MAX / (p * clk_div));
>>  	return clamp_val(((clk_freq * 30 * gear_mult) / (rpm * p * clk_div)),
>>  			 0, 255);
>>  }
>
> If my math is right, this can be simplified to:
>
I wrote test code to compare the old calculation and the new one for the complete
value range (I didn't trust my math skills :-), and it reports the same results.
So it should be correct.

> 	rpm = clamp_val(rpm, 1, (clk_freq * 30 * gear_mult) / (255  * p * clk_div));
> 	return (clk_freq * 30 * gear_mult) / (rpm * p * clk_div);
>
> which may or may not actually be more simple ;-) as it avoids the
> double clamping only at the cost of extra arithmetic. Probably only
> worth it if we store the result of clk_freq * 30 * gear_mult and p *
> clk_div locally so we don't have to compute them again.
>
Excellent idea. I'll make that change.

Thanks,
Guenter

> Probably doesn't matter much in the end, so it's your call.
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

[PATCH 16/17] hwmon: (gl518sm) Fix overflows seen when writing into limit attributes

[Guenter Roeck]

Writes into limit attributes can overflow due to additions and
multiplications with unchecked parameters.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gl518sm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/gl518sm.c b/drivers/hwmon/gl518sm.c
index 0212c8317bca..0a11a550c2a2 100644
--- a/drivers/hwmon/gl518sm.c
+++ b/drivers/hwmon/gl518sm.c
@@ -86,9 +86,8 @@ enum chips { gl518sm_r00, gl518sm_r80 };
 #define BOOL_FROM_REG(val)	((val) ? 0 : 1)
 #define BOOL_TO_REG(val)	((val) ? 0 : 1)
 
-#define TEMP_TO_REG(val)	clamp_val(((((val) < 0 ? \
-				(val) - 500 : \
-				(val) + 500) / 1000) + 119), 0, 255)
+#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -119000, 136000), \
+					    1000) + 119)
 #define TEMP_FROM_REG(val)	(((val) - 119) * 1000)
 
 static inline u8 FAN_TO_REG(long rpm, int div)
@@ -101,10 +100,11 @@ static inline u8 FAN_TO_REG(long rpm, int div)
 }
 #define FAN_FROM_REG(val, div)	((val) == 0 ? 0 : (480000 / ((val) * (div))))
 
-#define IN_TO_REG(val)		clamp_val((((val) + 9) / 19), 0, 255)
+#define IN_TO_REG(val)	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19), 19)
 #define IN_FROM_REG(val)	((val) * 19)
 
-#define VDD_TO_REG(val)		clamp_val((((val) * 4 + 47) / 95), 0, 255)
+#define VDD_TO_REG(val) \
+	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
 #define VDD_FROM_REG(val)	(((val) * 95 + 2) / 4)
 
 #define DIV_FROM_REG(val)	(1 << (val))
-- 
2.5.0

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

[Jean Delvare]

Hi Guenter,

On Sun,  4 Dec 2016 20:55:39 -0800, Guenter Roeck wrote:
> Writes into limit attributes can overflow due to additions and
> multiplications with unchecked parameters.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/gl518sm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/gl518sm.c b/drivers/hwmon/gl518sm.c
> index 0212c8317bca..0a11a550c2a2 100644
> --- a/drivers/hwmon/gl518sm.c
> +++ b/drivers/hwmon/gl518sm.c
> @@ -86,9 +86,8 @@ enum chips { gl518sm_r00, gl518sm_r80 };
>  #define BOOL_FROM_REG(val)	((val) ? 0 : 1)
>  #define BOOL_TO_REG(val)	((val) ? 0 : 1)
>  
> -#define TEMP_TO_REG(val)	clamp_val(((((val) < 0 ? \
> -				(val) - 500 : \
> -				(val) + 500) / 1000) + 119), 0, 255)
> +#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -119000, 136000), \
> +					    1000) + 119)
>  #define TEMP_FROM_REG(val)	(((val) - 119) * 1000)
>  
>  static inline u8 FAN_TO_REG(long rpm, int div)
> @@ -101,10 +100,11 @@ static inline u8 FAN_TO_REG(long rpm, int div)
>  }
>  #define FAN_FROM_REG(val, div)	((val) == 0 ? 0 : (480000 / ((val) * (div))))
>  
> -#define IN_TO_REG(val)		clamp_val((((val) + 9) / 19), 0, 255)
> +#define IN_TO_REG(val)	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19), 19)
>  #define IN_FROM_REG(val)	((val) * 19)
>  
> -#define VDD_TO_REG(val)		clamp_val((((val) * 4 + 47) / 95), 0, 255)
> +#define VDD_TO_REG(val) \
> +	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
>  #define VDD_FROM_REG(val)	(((val) * 95 + 2) / 4)
>  
>  #define DIV_FROM_REG(val)	(1 << (val))

Code looks good. Alignment is a bit clumsy though. Also it feels
strange now that you are using DIV_ROUND_CLOSEST for VDD_TO_REG but not
VDD_FROM_REG.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

Hi Jean,

On Tue, Dec 13, 2016 at 10:48:29AM +0100, Jean Delvare wrote:
> Hi Guenter,
> 
> On Sun,  4 Dec 2016 20:55:39 -0800, Guenter Roeck wrote:
> > Writes into limit attributes can overflow due to additions and
> > multiplications with unchecked parameters.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/hwmon/gl518sm.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hwmon/gl518sm.c b/drivers/hwmon/gl518sm.c
> > index 0212c8317bca..0a11a550c2a2 100644
> > --- a/drivers/hwmon/gl518sm.c
> > +++ b/drivers/hwmon/gl518sm.c
> > @@ -86,9 +86,8 @@ enum chips { gl518sm_r00, gl518sm_r80 };
> >  #define BOOL_FROM_REG(val)	((val) ? 0 : 1)
> >  #define BOOL_TO_REG(val)	((val) ? 0 : 1)
> >  
> > -#define TEMP_TO_REG(val)	clamp_val(((((val) < 0 ? \
> > -				(val) - 500 : \
> > -				(val) + 500) / 1000) + 119), 0, 255)
> > +#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -119000, 136000), \
> > +					    1000) + 119)
> >  #define TEMP_FROM_REG(val)	(((val) - 119) * 1000)
> >  
> >  static inline u8 FAN_TO_REG(long rpm, int div)
> > @@ -101,10 +100,11 @@ static inline u8 FAN_TO_REG(long rpm, int div)
> >  }
> >  #define FAN_FROM_REG(val, div)	((val) == 0 ? 0 : (480000 / ((val) * (div))))
> >  
> > -#define IN_TO_REG(val)		clamp_val((((val) + 9) / 19), 0, 255)
> > +#define IN_TO_REG(val)	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19), 19)
> >  #define IN_FROM_REG(val)	((val) * 19)
> >  
> > -#define VDD_TO_REG(val)		clamp_val((((val) * 4 + 47) / 95), 0, 255)
> > +#define VDD_TO_REG(val) \
> > +	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
> >  #define VDD_FROM_REG(val)	(((val) * 95 + 2) / 4)
> >  
> >  #define DIV_FROM_REG(val)	(1 << (val))
> 
> Code looks good. Alignment is a bit clumsy though. Also it feels
> strange now that you are using DIV_ROUND_CLOSEST for VDD_TO_REG but not
> VDD_FROM_REG.
> 
I cleaned up the alignment, and added DIV_ROUND_CLOSEST() to VDD_TO_REG().
I'll resend an updated version.

Thanks,
Guenter

> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
> -- 
> Jean Delvare
> SUSE L3 Support

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

[Guenter Roeck]

Writes into limit attributes can overflow due to multplications
and additions with unbound input values.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gl520sm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/gl520sm.c b/drivers/hwmon/gl520sm.c
index dee93ec87d02..4bb37d7234b1 100644
--- a/drivers/hwmon/gl520sm.c
+++ b/drivers/hwmon/gl520sm.c
@@ -209,10 +209,11 @@ static ssize_t get_cpu_vid(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR(cpu0_vid, S_IRUGO, get_cpu_vid, NULL);
 
 #define VDD_FROM_REG(val) (((val) * 95 + 2) / 4)
-#define VDD_TO_REG(val) clamp_val((((val) * 4 + 47) / 95), 0, 255)
+#define VDD_TO_REG(val) \
+	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
 
 #define IN_FROM_REG(val) ((val) * 19)
-#define IN_TO_REG(val) clamp_val((((val) + 9) / 19), 0, 255)
+#define IN_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19), 19)
 
 static ssize_t get_in_input(struct device *dev, struct device_attribute *attr,
 			    char *buf)
@@ -514,8 +515,8 @@ static DEVICE_ATTR(fan1_off, S_IRUGO | S_IWUSR,
 		get_fan_off, set_fan_off);
 
 #define TEMP_FROM_REG(val) (((val) - 130) * 1000)
-#define TEMP_TO_REG(val) clamp_val(((((val) < 0 ? \
-			(val) - 500 : (val) + 500) / 1000) + 130), 0, 255)
+#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -130000, 125000), \
+					    1000) + 130)
 
 static ssize_t get_temp_input(struct device *dev, struct device_attribute *attr,
 			      char *buf)
-- 
2.5.0

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

[Jean Delvare]

On Sun,  4 Dec 2016 20:55:40 -0800, Guenter Roeck wrote:
> Writes into limit attributes can overflow due to multplications
> and additions with unbound input values.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/gl520sm.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/gl520sm.c b/drivers/hwmon/gl520sm.c
> index dee93ec87d02..4bb37d7234b1 100644
> --- a/drivers/hwmon/gl520sm.c
> +++ b/drivers/hwmon/gl520sm.c
> @@ -209,10 +209,11 @@ static ssize_t get_cpu_vid(struct device *dev, struct device_attribute *attr,
>  static DEVICE_ATTR(cpu0_vid, S_IRUGO, get_cpu_vid, NULL);
>  
>  #define VDD_FROM_REG(val) (((val) * 95 + 2) / 4)
> -#define VDD_TO_REG(val) clamp_val((((val) * 4 + 47) / 95), 0, 255)
> +#define VDD_TO_REG(val) \
> +	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
>  
>  #define IN_FROM_REG(val) ((val) * 19)
> -#define IN_TO_REG(val) clamp_val((((val) + 9) / 19), 0, 255)
> +#define IN_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19), 19)
>  
>  static ssize_t get_in_input(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
> @@ -514,8 +515,8 @@ static DEVICE_ATTR(fan1_off, S_IRUGO | S_IWUSR,
>  		get_fan_off, set_fan_off);
>  
>  #define TEMP_FROM_REG(val) (((val) - 130) * 1000)
> -#define TEMP_TO_REG(val) clamp_val(((((val) < 0 ? \
> -			(val) - 500 : (val) + 500) / 1000) + 130), 0, 255)
> +#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -130000, 125000), \
> +					    1000) + 130)
>  
>  static ssize_t get_temp_input(struct device *dev, struct device_attribute *attr,
>  			      char *buf)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

But I think FAN_TO_REG can overflow too? Input value is left-shifted
without a prior check.

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

On 12/13/2016 01:56 AM, Jean Delvare wrote:
> On Sun,  4 Dec 2016 20:55:40 -0800, Guenter Roeck wrote:
>> Writes into limit attributes can overflow due to multplications
>> and additions with unbound input values.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  drivers/hwmon/gl520sm.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/gl520sm.c b/drivers/hwmon/gl520sm.c
>> index dee93ec87d02..4bb37d7234b1 100644
>> --- a/drivers/hwmon/gl520sm.c
>> +++ b/drivers/hwmon/gl520sm.c
>> @@ -209,10 +209,11 @@ static ssize_t get_cpu_vid(struct device *dev, struct device_attribute *attr,
>>  static DEVICE_ATTR(cpu0_vid, S_IRUGO, get_cpu_vid, NULL);
>>
>>  #define VDD_FROM_REG(val) (((val) * 95 + 2) / 4)
>> -#define VDD_TO_REG(val) clamp_val((((val) * 4 + 47) / 95), 0, 255)
>> +#define VDD_TO_REG(val) \
>> +	DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 95 / 4) * 4, 95)
>>
>>  #define IN_FROM_REG(val) ((val) * 19)
>> -#define IN_TO_REG(val) clamp_val((((val) + 9) / 19), 0, 255)
>> +#define IN_TO_REG(val) DIV_ROUND_CLOSEST(clamp_val(val, 0, 255 * 19), 19)
>>
>>  static ssize_t get_in_input(struct device *dev, struct device_attribute *attr,
>>  			    char *buf)
>> @@ -514,8 +515,8 @@ static DEVICE_ATTR(fan1_off, S_IRUGO | S_IWUSR,
>>  		get_fan_off, set_fan_off);
>>
>>  #define TEMP_FROM_REG(val) (((val) - 130) * 1000)
>> -#define TEMP_TO_REG(val) clamp_val(((((val) < 0 ? \
>> -			(val) - 500 : (val) + 500) / 1000) + 130), 0, 255)
>> +#define TEMP_TO_REG(val) (DIV_ROUND_CLOSEST(clamp_val(val, -130000, 125000), \
>> +					    1000) + 130)
>>
>>  static ssize_t get_temp_input(struct device *dev, struct device_attribute *attr,
>>  			      char *buf)
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>
> But I think FAN_TO_REG can overflow too? Input value is left-shifted
> without a prior check.
>
You are right. My older script didn't detect that because the overflow happens
with a very low value, and the script just concluded that the value range was [0,0].

After improving my test script, the driver generates KASAN bad memory reports.
Outch. I'll have to look into that.

Thanks,
Guenter

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

[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 <linux@roeck-us.net>
> ---
>  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, -40000, 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

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

[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 <linux@roeck-us.net>
>> ---
>>  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, -40000, 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