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

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

[Guenter Roeck]

Sashiko is concerned that reading a PWM frequency multiplier outside
the supported range of [1, 31] might result in bad PWM values written
to the chip. Technically, the chip should never return a value with
the upper 3 bits set, so this should never happen. However, it is
unknown if there are LM63 variants where the upper bits of the register
can be written.

Clamp the PWM frequency multiplier value to [1, 31] when reading it
from the chip to avoid the problem.

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

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index e2a429e579ac..45a0efa8a39d 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -32,6 +32,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/hwmon.h>
 #include <linux/err.h>
+#include <linux/minmax.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/sysfs.h>
@@ -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);
 		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);
 
 	switch (data->kind) {
 	case lm63:
-- 
2.45.2

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

[sashiko-bot]

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

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/20260727202120.3710948-1-linux@roeck-us.net?part=1