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

[Jean Delvare <jdelvare@suse.de>]

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