Re: [PATCH] hwmon: (w83627ehf) Clamp voltage limits before scaling

[Guenter Roeck <linux@roeck-us.net>]

On 11/11/26 03:11, David Laight wrote:
> On Wed, 11 Nov 2026 14:48:31 +0800
> Zhu Jun <zhujun2@cmss.chinamobile.com> wrote:
> 
>> in_to_reg() narrows the sysfs value to u32 and multiplies it by 100
>> before clamping. A large positive voltage limit can consequently be
>> programmed as zero instead of the maximum; 4294967296 mV reproduces this
>> on a 64-bit system.
>>
>> Preserve the input width and clamp to the representable voltage range
>> before multiplication.
>>
>> Fixes: cf0676fe42c8 ("[PATCH] w83627ehf: Add voltage inputs support")
>> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
>> ---
>>   drivers/hwmon/w83627ehf.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
>> index 9c08ab662dc6..5c04b9c52b76 100644
>> --- a/drivers/hwmon/w83627ehf.c
>> +++ b/drivers/hwmon/w83627ehf.c
>> @@ -293,8 +293,9 @@ static inline long in_from_reg(u8 reg, u8 nr, const u16 *scale_in)
>>   	return DIV_ROUND_CLOSEST(reg * scale_in[nr], 100);
>>   }
>>   
>> -static inline u8 in_to_reg(u32 val, u8 nr, const u16 *scale_in)
>> +static inline u8 in_to_reg(unsigned long val, u8 nr, const u16 *scale_in)
>>   {
>> +	val = min(val, 255UL * scale_in[nr] / 100);
> 
> You need to think about these more.
> Either it really doesn't matter because no one would really try to write
> an over large value unless they are trying to break things - and they can
> just write zero, or you just need to stop the n * 100 + x overflowing.

Writing 0 would be intentional. Writing a large value, however, _can_
be accidental (or just someone trying to find the highest possible value),
so I accept such patches (and sometimes, if I have too much time, hunt
for it as well). My module tests actually depend on it - they on purpose
write a large value to see what happens.

Thanks,
Guenter

> So at least avoid the extra divide by using:
> 	val = min(val, (~0ul - 32768) / 100)
> 
>>   	return clamp_val(DIV_ROUND_CLOSEST(val * 100, scale_in[nr]), 0, 255);
> 
> Another clamp_val() that should never have been added.
> The type checks are there for a reason, not to be killed by casts.
> 
> David
> 
>>   }
>>   
>