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

On Sat, Aug 8, 2026 at 4:19 PM Nguyễn Công <[email protected]> wrote:
>
> 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.

Following up after checking the datasheet (MAX6621AUB):

All temperature values use 16-bit two's complement, left-shifted so that
the +1 C bit is bit 6 (1/64 C per LSB). The alternate integer
representation is optional and enabled via CONFIG0 bit 6, which this
driver never sets, so the device stays in the default shifted format.

The offset path already accounts for this: it shifts by
MAX6621_REG_TEMP_SHIFT on read and uses max6621_temp_mc2reg() (which
left-shifts by 6) on write. The temp_crit path does not -- it uses a
plain "regval * 1000" on read and "val / 1000" on write, i.e. the math
for the alternate/integer format that isn't enabled.

As far as I can tell the alert-threshold registers are in that same
shifted format (the datasheet doesn't give a separate worked example for
them, but they're compared against PECI temperatures in the current data
format, and CONFIG2/offset is explicitly "represented in the current data
format"). If so, the threshold is programmed 64x too low: a 95 C crit
stores 0x005f, which the device interprets as 0x5f/64 ~= 1.5 C, so ALERT
can assert almost immediately. It round-trips through sysfs (the read
undoes the write), which hides it from a simple read-after-write check.

So this is a scaling bug rather than the clamp/+128 wrap I mentioned
earlier -- with the shift in place, 128 C encodes as 0x2000 and there is
no wrap.

I'll send a patch scaling the crit read/write like the offset path. It
builds on the sign-extension fix in this patch, so I'd send it as a
follow-up on top -- happy to fold both crit changes into a v2 instead if
you'd prefer. Confirmation from someone with the hardware would be
welcome, since I've only verified this against the datasheet.

The 1/64 C precision point stands as a separate enhancement.