Re: [PATCH] hwmon: (max6621) fix negative temperature offset and crit readings

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

Pre-existing issues:
- [High] Writing the maximum allowed temperature of 128°C to hwmon_temp_crit silently wraps to -128°C in hardware.
- [Medium] Fractional temperature precision is discarded due to shifting and integer division before scaling.
--

--- Patch [1]: [PATCH] hwmon: (max6621) fix negative temperature offset and crit readings ---
commit a073f1d4838dda93adf9b46f8e3cc16f1b662f82
Author: Cong Nguyen <[email protected]>

hwmon: (max6621) fix negative temperature offset and crit readings

This patch fixes temperature reporting for the max6621 driver. It
addresses an issue where negative temperatures were reported as large
positive numbers due to missing sign extension before scaling the 16-bit
register values.

Cc: [email protected]

>       - temp_input used an s8 intermediate, which is correct for the
>         -127..127 range but reports the documented +128 degrees C maximum as
>         -128 degrees C.

[Severity: High]
This is a pre-existing issue, but while fixing the read path for the +128
degrees C maximum, is it possible that the write path still wraps to -128
degrees C?

When writing to hwmon_temp_crit, max6621_write() in drivers/hwmon/max6621.c
clamps the input to MAX6621_TEMP_INPUT_MAX (128000) and divides by 1000L:

drivers/hwmon/max6621.c:max6621_write() {
    ...
    case hwmon_temp_crit:
        ...
        /* Clamp to allowed range to prevent overflow. */
        val = clamp_val(val, MAX6621_TEMP_INPUT_MIN,
                MAX6621_TEMP_INPUT_MAX);
        val = val / 1000L;

        return regmap_write(data->regmap, reg, val);
}

If a user writes 128000 to temp_crit, it gets clamped and divided to 128
(0x80). For an 8-bit two's complement alert register in the MAX6621
hardware, 0x80 is interpreted as -128 degrees C.

Could this cause continuous false thermal alerts and potential unintended
system throttling?

> diff --git a/drivers/hwmon/max6621.c b/drivers/hwmon/max6621.c
> index e86ec6d237ca6..9becd098d840b 100644
> --- a/drivers/hwmon/max6621.c
> +++ b/drivers/hwmon/max6621.c
[ ... ]
> @@ -225,8 +224,8 @@ max6621_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  			 * The temperature is given in two's complement and 8
>  			 * bits is used for the register conversion.
>  			 */
> -			temp = (regval >> MAX6621_REG_TEMP_SHIFT);
> -			*val = temp * 1000L;
> +			*val = (sign_extend32(regval, 15) >>
> +				MAX6621_REG_TEMP_SHIFT) * 1000L;

[Severity: Medium]
This is a pre-existing issue, but does this arithmetic right shift discard
the hardware's fractional temperature precision?

By shifting right by MAX6621_REG_TEMP_SHIFT (6 bits) before multiplying by
1000L, the lower 6 fractional bits are lost, dropping up to 0.984 degrees C
of precision.

Would it be better to multiply by 1000L first and then shift or divide to
expose the full fractional precision provided by the MAX6621 device?

The same precision loss also happens when writing hwmon_temp_offset in
drivers/hwmon/max6621.c:

drivers/hwmon/max6621.c:max6621_temp_mc2reg() {
    return (val / 1000L) << MAX6621_REG_TEMP_SHIFT;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1