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

On Sat, Aug 8, 2026 at 3:58 PM <[email protected]> wrote:
>
> 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

Thanks for the review. Both points are pre-existing and on the
write/precision side, so I'd like to keep this patch focused on the
sign-extension read fix and handle them separately:

- temp_crit write / +128 °C: agreed this is a pre-existing write-path
issue. max6621_write() clamps to MAX6621_TEMP_INPUT_MAX (128000), so
128 °C lands in the (non-shifted) alert register as 0x80; if that
register is interpreted as signed, it wraps. I'll send a separate
patch to tighten the clamp once I've confirmed the alert register
width/sign against the datasheet — it's orthogonal to this read-side
fix.
- Fractional precision: also pre-existing — both the read (>>
MAX6621_REG_TEMP_SHIFT before scaling) and the write
(max6621_temp_mc2reg() divides by 1000 before shifting) discard the 6
fractional bits, so the driver has always reported at 1 °C resolution.
Exposing the 1/64 °C resolution is a behavioural change rather than a
fix, so I'd prefer to do that as a follow-up.

This patch only restores the sign of the existing values, which were
being reported as large positives for negative offsets and crit
thresholds.