Re: [PATCH] hwmon: (lm63) Clamp PWM frequency multiplier to [1, 31]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using clamp() instead of masking incorrectly handles reserved bits, potentially corrupting the PWM frequency multiplier.
--

--- Patch [1]: [PATCH] hwmon: (lm63) Clamp PWM frequency multiplier to [1, 31] ---
commit 7a1685b6b2f22a4e89624bbad02ad4ecbb43a849
Author: Guenter Roeck <[email protected]>

hwmon: (lm63) Clamp PWM frequency multiplier to [1, 31]

This commit updates the lm63 driver to clamp the PWM frequency multiplier
read from the chip to the range [1, 31]. This is intended to prevent writing
bad PWM values if the upper bits of the register are unexpectedly set on
some variants.

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c

[ ... ]

> @@ -249,8 +250,7 @@ static struct lm63_data *lm63_update_device(struct device *dev)
>  
>  		data->pwm1_freq = i2c_smbus_read_byte_data(client,
>  				  LM63_REG_PWM_FREQ);
> -		if (data->pwm1_freq == 0)
> -			data->pwm1_freq = 1;
> +		data->pwm1_freq = clamp(data->pwm1_freq, 1, 31);

[Severity: High]
If the upper bits of the LM63_REG_PWM_FREQ register happen to be set on some
LM63 variants, won't clamp() treat the entire byte as a magnitude?

For example, if the register reads 0x81, clamp(129, 1, 31) will evaluate
to 31, rather than isolating the lower 5-bit multiplier of 1.

Would it be more correct to mask out the reserved bits with & 0x1F before
checking for zero or clamping?

>  		data->pwm1[0] = i2c_smbus_read_byte_data(client,
>  				LM63_REG_PWM_VALUE);
>  

[ ... ]

> @@ -1188,8 +1188,7 @@ static void lm63_init_client(struct lm63_data *data)
>  
>  	/* We may need pwm1_freq before ever updating the client data */
>  	data->pwm1_freq = i2c_smbus_read_byte_data(client, LM63_REG_PWM_FREQ);
> -	if (data->pwm1_freq == 0)
> -		data->pwm1_freq = 1;
> +	data->pwm1_freq = clamp(data->pwm1_freq, 1, 31);

[Severity: High]
Does this have the same risk as lm63_update_device() above, where reserved
upper bits being set will cause the multiplier to be incorrectly calculated
and clamped to 31?

>  
>  	switch (data->kind) {
>  	case lm63:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1