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

On Thu, Apr 16, 2026 at 09:05:05PM +0800, Gui-Dong Han wrote:
> 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.
> 

FWIW, that happens a lot. The AI is surprisingly good at finding all
those old problems. Personally I'd prefer to convert all the old drivers
to the with_info hwmon API - that alone solves most of the locking
problems. Unfortunately I don't have enough time to do that.
The latest Claude/Gemini models actually do it quite well with the
appropriate prompts, but it still needs mnaual verification.

Either case, for me it only makes sense to touch those old drivers if
all their problems are fixed. This driver is a good example: Ignoring
I2C errors is much worse than its locking issues.

Also, please note that you can often use guard() or scoped_guard()
to add lock-protection to a function.

Thanks,
Guenter

> > --
> >
> > 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
>