Re: [PATCH 16/17] hwmon: (gl518sm) Fix overflows seen when writing into limit attributes
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
> > ---
> > 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 : (48 / ((val) *
> > (div
> >
> > -#define IN_TO_REG(val) clamp_valval) + 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_valval) * 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
>
> --
> Jean Delvare
> SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] hwmon updates for v4.10
Hi Linus, Please pull hwmon updates for Linux v4.10 from signed tag: git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-for-linus-v4.10 Thanks, Guenter -- The following changes since commit e5517c2a5a49ed5e99047008629f1cd60246ea0e: Linux 4.9-rc7 (2016-11-27 13:08:04 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git tags/hwmon-for-linus-v4.10 for you to fetch changes up to 4fccd4a1e8944033bcd7693ea4e8fb478cd2059a: hwmon: (g762) Fix overflows and crash seen when writing limit attributes (2016-12-12 11:33:44 -0800) hwmon updates for v4.10 - new drivers for TMP108 and TC654 - hwmon core code cleanup - coretemp driver cleanup - fix overflow issues in several drivers - minor fixes, cleanups and enhancements in various drivers Chris Packham (1): hwmon: Add tc654 driver Clemens Gruber (3): hwmon: (mcp3021) add devicetree bindings documentation hwmon: (mcp3021) replace S_IRUGO with 0444 hwmon: (mcp3021) add devicetree support Guenter Roeck (21): hwmon: (adm9240) Fix overflows seen when writing into limit attributes hwmon: (ds620) Fix overflows seen when writing temperature limits hwmon: (smsc47m192) Fix overflows seen when writing into limit attributes hwmon: (core) Clarify when read and write callbacks are mandatory hwmon: (core) Add support for string attributes to new API hwmon: (core) Clarify use of chip attributes hwmon: (core) Deprecate hwmon_device_register() hwmon: (core) Make is_visible callback truly mandatory hwmon: (core) Explain why at least two attribute groups are allocated hwmon: (core) Rename groups parameter in API to extra_groups hwmon: (core) Simplify sysfs attribute name allocation hwmon: (adm1025) Fix overflows seen when writing voltage limits hwmon: (adm1026) Fix overflows seen when writing into limit attributes hwmon: (adt7462) Fix overflows seen when writing into limit attributes hwmon: (adt7470) Fix overflows seen when writing into limit attributes hwmon: (nct7802) Fix overflows seen when writing into limit attributes hwmon: (lm87) Fix overflow seen when writing voltage limit attributes hwmon: (lm85) Fix overflows seen when writing voltage limit attributes hwmon: (emc2103) Fix overflows seen when temperature limit attributes hwmon: (emcw201) Fix overflows seen when writing into limit attributes hwmon: (g762) Fix overflows and crash seen when writing limit attributes Jared Bents (1): hwmon: (amc6821) sign extension temperature Jason Gunthorpe (1): hwmon: (lm87) Use hwmon to create the sysfs groups Javier Martinez Canillas (1): hwmon: (scpi) Fix module autoload John Muir (2): hwmon: Add Texas Instruments TMP108 temperature sensor driver. devicetree: hwmon: Add documentation for TMP108 driver. Michael Walle (1): hwmon: (adt7411) update to new hwmon registration API Sebastian Andrzej Siewior (1): hwmon: (via-cputemp) Convert to hotplug state machine Thomas Gleixner (7): hwmon: (via-cputemp) Remove pointless CPU check on each CPU hwmon: (coretemp) Fixup target cpu for package when cpu is offlined hwmon: (coretemp) Simplify sibling management hwmon: (coretemp) Avoid redundant lookups hwmon: (coretemp) Convert to hotplug state machine hwmon: (coretemp) Use proper error codes in cpu online callback hwmon: (coretemp) Simplify package management Tobias Klauser (1): hwmon: (lm90) Mention support for TI TMP451 in Kconfig description Yi Li (1): hwmon: (adm1275) Enable adm1278 VOUT sampling .../devicetree/bindings/hwmon/mcp3021.txt | 21 + Documentation/devicetree/bindings/hwmon/tmp108.txt | 14 + .../devicetree/bindings/i2c/trivial-devices.txt| 2 + Documentation/hwmon/hwmon-kernel-api.txt | 58 ++- Documentation/hwmon/tc654 | 31 ++ Documentation/hwmon/tmp108 | 36 ++ drivers/hwmon/Kconfig | 26 +- drivers/hwmon/Makefile | 2 + drivers/hwmon/adm1025.c| 2 +- drivers/hwmon/adm1026.c| 26 +- drivers/hwmon/adm9240.c| 9 +- drivers/hwmon/adt7411.c| 301 +++- drivers/hwmon/adt7462.c| 12 +- drivers/hwmon/adt7470.c| 6 +- drivers/hwmon/amc6821.c| 4 +- drivers/hwmon/coretemp.c | 321 + drivers/hwmon/ds620.c | 2 +- drivers/hwmon/emc2103.c| 4 +- drivers/hwmon/em
Re: [PATCH 03/17] hwmon: (lm93) Fix overflows seen when writing into limit attributes
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 --- 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_auto_base. The offsets are expressed in cumulative -degrees C, with the value of offset for temperature value being +temperature offsets. The base temperature is expressed in milli-degrees C in +the sysfs files temp_auto_base. The offsets are expressed in cumulative +milli-degrees C, with the value of offset for temperature value being contained in the file temp_auto_offset. E.g. if the base temperature is 40C: offset # temp_auto_offset range pwm 1 0 -25.00% 2 0 -28.57% -3 1 40C - 41C32.14% -4 1 41C - 42C35.71% -5 2 42C - 44C39.29% -6 2 44C - 46C42.86% -7 2 48C - 50C46.43% -8 2 50C - 52C50.00% -9 2 52C - 54C53.57% - 10 2 54C - 56C57.14% - 11 2 56C - 58C71.43% - 12 2 58C - 60C85.71% +3 500 40C - 41C32.14% +4 500 41C - 42C35.71% +5 100042C - 44C39.29% +6 100044C - 46C42.86% +7 100048C - 50C46.43% +8 100050C - 52C50.00% +9 100052C - 54C53.57% + 10 100054C - 56C57.14% + 11 100056C - 58C71.43% + 12 100058C - 60C85.71% > 60C100.00% I'm a bit confused, I would have expected 1000 and 2000 for temp_auto_offset, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/17] hwmon: (gl520sm) Fix overflows seen when writing into limit attributes
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 --- 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_valval) * 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_valval) + 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, -13, 125000), \ + 1000) + 130) static ssize_t get_temp_input(struct device *dev, struct device_attribute *attr, char *buf) Reviewed-by: Jean Delvare 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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/17] hwmon: (lm93) Fix overflows seen when writing into limit attributes
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 > --- > 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_auto_base. The offsets are expressed in cumulative > -degrees C, with the value of offset for temperature value being > +temperature offsets. The base temperature is expressed in milli-degrees C in > +the sysfs files temp_auto_base. The offsets are expressed in cumulative > +milli-degrees C, with the value of offset for temperature value being > contained in the file temp_auto_offset. E.g. if the base temperature > is 40C: > > offset #temp_auto_offset range pwm >1 0 -25.00% >2 0 -28.57% > - 3 1 40C - 41C32.14% > - 4 1 41C - 42C35.71% > - 5 2 42C - 44C39.29% > - 6 2 44C - 46C42.86% > - 7 2 48C - 50C46.43% > - 8 2 50C - 52C50.00% > - 9 2 52C - 54C53.57% > - 10 2 54C - 56C57.14% > - 11 2 56C - 58C71.43% > - 12 2 58C - 60C85.71% > + 3 500 40C - 41C32.14% > + 4 500 41C - 42C35.71% > + 5 100042C - 44C39.29% > + 6 100044C - 46C42.86% > + 7 100048C - 50C46.43% > + 8 100050C - 52C50.00% > + 9 100052C - 54C53.57% > + 10 100054C - 56C57.14% > + 11 100056C - 58C71.43% > + 12 100058C - 60C85.71% > > 60C 100.00% I'm a bit confused, I would have expected 1000 and 2000 for temp_auto_offset, 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 -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 17/17] hwmon: (gl520sm) Fix overflows seen when writing into limit attributes
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 > --- > 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_valval) * 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_valval) + 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, -13, 125000), > \ > + 1000) + 130) > > static ssize_t get_temp_input(struct device *dev, struct device_attribute > *attr, > char *buf) Reviewed-by: Jean Delvare But I think FAN_TO_REG can overflow too? Input value is left-shifted without a prior check. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 16/17] hwmon: (gl518sm) Fix overflows seen when writing into limit attributes
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
> ---
> 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 : (48 / ((val) *
> (div
>
> -#define IN_TO_REG(val) clamp_valval) + 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_valval) * 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
--
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
