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

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

[Guenter Roeck]

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 shifting of negative values. In C, the result of
left-shifting negative values is undefined, and the result of
right-shifting negative values is implementation dependent. Use
multiply and divide operations instead to avoid both problems.

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 <sashiko-bot@kernel.org>
Cc: Wilken Gottwalt <wilken.gottwalt@posteo.net>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
RESEND: Forgot to copy Wilken

 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..6e12325e0a3b 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;
 
-	return (exp >= 0) ? (result << exp) : (result >> -exp);
+	if (exp >= 0)
+		result *= (int)(1UL << exp);
+	else
+		result /= (int)(1UL << -exp);
+
+	return clamp(result, LONG_MIN, LONG_MAX);
 }
 
 /* the micro-controller uses percentage values to control pwm */
@@ -263,13 +268,13 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
 	case PSU_CMD_RAIL_AMPS:
 	case PSU_CMD_TEMP0:
 	case PSU_CMD_TEMP1:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1000);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1000);
 		break;
 	case PSU_CMD_FAN:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1);
 		break;
 	case PSU_CMD_FAN_PWM_ENABLE:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1);
 		/*
 		 * 0 = automatic mode, means the micro-controller controls the fan using a plan
 		 *     which can be modified, but changing this plan is not supported by this
@@ -283,12 +288,12 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
 			*val = 2;
 		break;
 	case PSU_CMD_FAN_PWM:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1);
 		*val = corsairpsu_dutycycle_to_pwm(*val);
 		break;
 	case PSU_CMD_RAIL_WATTS:
 	case PSU_CMD_TOTAL_WATTS:
-		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1000000);
+		*val = corsairpsu_linear11_to_long(tmp & 0xFFFF, 1000000);
 		break;
 	case PSU_CMD_TOTAL_UPTIME:
 	case PSU_CMD_UPTIME:
-- 
2.45.2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver lacks synchronization for the shared `priv->cmd_buffer` and `wait_completion` when accessed concurrently via debugfs and sysfs, violating subsystem guidelines.
--

--- Patch [1]: [PATCH RESEND] hwmon: (corsair-psu) Fix linear11 calculation ---
(Note: The format of this report is altered to a free-form summary without inline diff quoting due to recitation restrictions.)

commit 1b6cfe916e69e567bfe7403168ee73ecb94923c7
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (corsair-psu) Fix linear11 calculation

This commit fixes the parsing of negative temperatures in Linear11 format by
correctly casting the shifted mantissa to s16. It also avoids undefined
behavior by replacing bitwise shifts on negative values with multiplication and
division, and clamps the final output to prevent underflow and overflow.

[Severity: High]
This is a pre-existing issue, but does the driver lack synchronization for the
shared priv->cmd_buffer and wait_completion primitives when accessed
concurrently?

Looking at corsairpsu_usb_cmd() in drivers/hwmon/corsair-psu.c, the shared
command buffer and completion structure are modified and sent without mutual
exclusion. Meanwhile, corsairpsu_debugfs_init() creates debugfs attributes
(such as "uptime") that bypass the hwmon core lock.

If a user or monitoring tool reads these debugfs attributes concurrently with
standard sysfs hwmon attributes, could this trigger a data race on the HID
command buffer, causing corrupted commands to be sent to the device? 

Since the subsystem guidelines mandate that drivers must implement their own
locking (e.g., hwmon_lock/hwmon_unlock) for attributes registered outside the
hwmon core, and given that wrong configurations can potentially cause the PSU
to shut down, should a lock be added to protect these shared HID structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803232156.2340048-1-linux@roeck-us.net?part=1

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

[Ali Ahmet Memis]

On Mon,  3 Aug 2026 16:21:55 -0700 Guenter Roeck wrote:
> +	if (exp >= 0)
> +		result *= (int)(1UL << exp);
> +	else
> +		result /= (int)(1UL << -exp);

This breaks the build on 32 bit. result is s64 and the divisor is only known
at runtime, so the compiler emits a libgcc call instead of a shift, and
__divdi3 is only provided by sparc and parisc. SENSORS_CORSAIR_PSU just
depends on HID, so i386 and 32 bit arm are reachable.

On this patch applied to 2d2338c93da7:

  $ make ARCH=i386 defconfig
  $ ./scripts/config --set-val CONFIG_SENSORS_CORSAIR_PSU y
  $ make ARCH=i386 olddefconfig
  $ make ARCH=i386 vmlinux
  [...]
    LD      .tmp_vmlinux1
  ld: drivers/hwmon/corsair-psu.o: in function `corsairpsu_get_value':
  corsair-psu.c:(.text+0xa0b): undefined reference to `__divdi3'
  ld: corsair-psu.c:(.text+0xa63): undefined reference to `__divdi3'
  ld: corsair-psu.c:(.text+0xa83): undefined reference to `__divdi3'
  ld: corsair-psu.c:(.text+0xaa3): undefined reference to `__divdi3'
  make[2]: *** [scripts/Makefile.vmlinux:72: vmlinux.unstripped] Error 1

Only the divide is a problem, the multiply builds fine. div_s64() would do
it, or keeping a shift on the negative branch, since the divisor is a power
of two anyway.

The mantissa fix itself looks right to me. ((s16)((val & 0x7ff) << 5)) >> 5
puts bit 10 of the mantissa into the sign bit of the s16 before the
arithmetic shift brings it back, which is what the old order failed to do.

One small thing on the changelog: it says right shifting negative values is
implementation defined and that multiply and divide are used to avoid that,
but exp and mant are still computed with >> on values that can be negative.
That is the normal sign extension idiom and fine in practice, it just reads
as if those had been converted too.

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

[Guenter Roeck]

On 8/3/26 17:15, Ali Ahmet Memis wrote:
> On Mon,  3 Aug 2026 16:21:55 -0700 Guenter Roeck wrote:
>> +	if (exp >= 0)
>> +		result *= (int)(1UL << exp);
>> +	else
>> +		result /= (int)(1UL << -exp);
> 
> This breaks the build on 32 bit. result is s64 and the divisor is only known
> at runtime, so the compiler emits a libgcc call instead of a shift, and
> __divdi3 is only provided by sparc and parisc. SENSORS_CORSAIR_PSU just
> depends on HID, so i386 and 32 bit arm are reachable.
> 
> On this patch applied to 2d2338c93da7:
> 
>    $ make ARCH=i386 defconfig
>    $ ./scripts/config --set-val CONFIG_SENSORS_CORSAIR_PSU y
>    $ make ARCH=i386 olddefconfig
>    $ make ARCH=i386 vmlinux
>    [...]
>      LD      .tmp_vmlinux1
>    ld: drivers/hwmon/corsair-psu.o: in function `corsairpsu_get_value':
>    corsair-psu.c:(.text+0xa0b): undefined reference to `__divdi3'
>    ld: corsair-psu.c:(.text+0xa63): undefined reference to `__divdi3'
>    ld: corsair-psu.c:(.text+0xa83): undefined reference to `__divdi3'
>    ld: corsair-psu.c:(.text+0xaa3): undefined reference to `__divdi3'
>    make[2]: *** [scripts/Makefile.vmlinux:72: vmlinux.unstripped] Error 1
> 
> Only the divide is a problem, the multiply builds fine. div_s64() would do
> it, or keeping a shift on the negative branch, since the divisor is a power
> of two anyway.
> 
> The mantissa fix itself looks right to me. ((s16)((val & 0x7ff) << 5)) >> 5
> puts bit 10 of the mantissa into the sign bit of the s16 before the
> arithmetic shift brings it back, which is what the old order failed to do.
> 
> One small thing on the changelog: it says right shifting negative values is
> implementation defined and that multiply and divide are used to avoid that,
> but exp and mant are still computed with >> on values that can be negative.
> That is the normal sign extension idiom and fine in practice, it just reads
> as if those had been converted too.

Good point. Supposedly is implementation defined, but even sign_extend32()
uses it, so I'll just undo that part.

Thanks,
Guenter