Re: [PATCH 3/3] hwmon: (adm1031) Serialize update rate changes

On Thu, Apr 16, 2026 at 8:21 PM <[email protected]> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] Missing I2C error handling leads to hardware register corruption and state desynchronization.

Thanks for the review.

I will look into the I2C error handling, but I cannot confirm the
exact fix just yet. If there is an issue, I will send a new patch to
resolve it.

Again, this is a pre-existing issue with the current codebase, not
something introduced by this patch.

> --
>
> commit ffbce3cc3043a4bfb1c8f7ba4a9010b9c8a23c2c
> Author: Gui-Dong Han <[email protected]>
>
> hwmon: (adm1031) Serialize update rate changes
>
> This commit expands the locked section in update_interval_store() to cover
> the hardware register read-modify-write sequence and the software state
> update. This ensures the hardware updates and cached state updates occur
> together.
>
> > diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
> > index 887fba9ea149f..a46db83471f2c 100644
> > --- a/drivers/hwmon/adm1031.c
> > +++ b/drivers/hwmon/adm1031.c
> [ ... ]
> > @@ -897,13 +897,14 @@ static ssize_t update_interval_store(struct device *dev,
> >       }
> >       /* if not found, we point to the last entry (lowest update interval) */
> >
> > +     mutex_lock(&data->update_lock);
> > +
> >       /* set the new update rate while preserving other settings */
> >       reg = adm1031_read_value(client, ADM1031_REG_FAN_FILTER);
>
> Could the truncation of the error code lead to register corruption?
>
> Looking at adm1031_read_value(), it returns a u8:
>
> static inline u8 adm1031_read_value(struct i2c_client *client, u8 reg)
> {
>         return i2c_smbus_read_byte_data(client, reg);
> }
>
> If i2c_smbus_read_byte_data() returns a negative error code like -EIO, it gets
> silently truncated to an 8-bit value (like 0xFB). This garbage value is then
> masked, modified, and written back to the register.
>
> >       reg &= ~ADM1031_UPDATE_RATE_MASK;
> >       reg |= i << ADM1031_UPDATE_RATE_SHIFT;
> >       adm1031_write_value(client, ADM1031_REG_FAN_FILTER, reg);
>
> Does ignoring the return value of adm1031_write_value() leave the software
> and hardware states desynchronized?
>
> If the I2C write fails, data->update_interval is updated anyway, meaning
> the cached software state no longer matches the actual hardware state
> despite the expanded lock.
>
> > -     mutex_lock(&data->update_lock);
> >       data->update_interval = update_intervals[i];
> >       mutex_unlock(&data->update_lock);
> >
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3