Re: [PATCH v2] hwmon: (corsair-psu) Fix linear11 calculation

On 8/6/26 08:34, Wilken Gottwalt wrote:
> On Mon,  3 Aug 2026 20:48:11 -0700
> Guenter Roeck <[email protected]> wrote:
> 
>> In corsairpsu_linear11_to_int(), the mantissa is extracted using bitwise
>> operations and cast to s16 before being shifted left:
>>
>> static int corsairpsu_linear11_to_int(const u16 val, const int scale)
>> {
>>      ...
>>      const int mant = (((s16)(val & 0x7ff)) << 5) >> 5;
>>      ...
>> }
>>
>> Due to C integer promotion rules, the masked value (which is always
>> positive) is promoted to a 32-bit integer before the left shift. As a
>> result, the sign bit is never extended to bit 31 of the promoted integer.
>>
>> When the device hardware reports a negative temperature in Linear11 format
>> (such as an ambient temperature probe reporting sub-zero), the negative
>> mantissa is parsed incorrectly as a massive positive value. For example,
>> -1 becomes 2047, which scales to 2047 degrees Celsius.
>>
>> Fix the problem by type casting the result of the left shift operation
>> to s16.
>>
>> Another problem is left-shifting of negative values. In C, the result of
>> left-shifting negative values is undefined. Use a multiplication instead
>> to avoid the problem.
>>
>> Also use a local s64 variable to store temporary results, change
>> the return value type from int to long, and clamp the final value
>> to LONG_MIN and LONG_MAX to avoid under- and overflow issues while
>> retaining as much information as possible.
>>
>> Reported-by: Sashiko <[email protected]>
>> Cc: Wilken Gottwalt <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> v2: Skip handling right-shift of negative values
>>      (since it is widely used in the kernel)
>>
>>   drivers/hwmon/corsair-psu.c | 23 ++++++++++++++---------
>>   1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
>> index 24100519cd83..c437b469de5c 100644
>> --- a/drivers/hwmon/corsair-psu.c
>> +++ b/drivers/hwmon/corsair-psu.c
>> @@ -137,13 +137,18 @@ struct corsairpsu_data {
>>   };
>>   
>>   /* some values are SMBus LINEAR11 data which need a conversion */
>> -static int corsairpsu_linear11_to_int(const u16 val, const int scale)
>> +static long corsairpsu_linear11_to_long(const u16 val, const int scale)
>>   {
>>   	const int exp = ((s16)val) >> 11;
>> -	const int mant = (((s16)(val & 0x7ff)) << 5) >> 5;
>> -	const int result = mant * scale;
>> +	const int mant = ((s16)((val & 0x7ff) << 5)) >> 5;
> 
> 
>> +	s64 result = mant * scale;
> 
> Uhm, this is a 32bit multiplicaiton, actully a C gotcha I explain the beginners
> in our company. https://godbolt.org/z/eM6TbGG5E
> 

It is, but that is ok and intentional: both mant and exp are guaranteed to be
no larger than s16, meaning the result is never larger than s32 and will never
overflow.

>> -	return (exp >= 0) ? (result << exp) : (result >> -exp);
>> +	if (exp >= 0)
>> +		result *= (int)(1UL << exp);

This is the calculation that can overflow, making it necessary for result to be s64.

Thanks,
Guenter