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

On Thu, Apr 16, 2026 at 10:05 PM Guenter Roeck <[email protected]> wrote:
>
> 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.

Sounds good. I might give the with_info conversion a try. Since it
would be my first time, I expect to hit a few roadblocks, so it
probably won't be a quick process.

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

Understood. If you prefer to drop this series and wait for a complete
rewrite, that's completely fine. Otherwise, if you think these simple
fixes are still useful in the meantime, feel free to apply them.

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

Thanks for the tip. I will use them in future patches.

Thanks.

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