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

On Thu, 6 Aug 2026 08:52:41 -0700
Guenter Roeck <[email protected]> wrote:

> 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.

Yeah, it was just funny to see in the wild. It made my day. :D

greetings,
Wilken