Re: [PATCH 2/3] hwmon: (adm1031) Hold lock while reading cached data

On Thu, Apr 16, 2026 at 7:57 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] Store functions like fan_auto_channel_store() read uninitialized cached values, which can silently disable hardware monitoring.
> - [High] auto_temp_max_store() corrupts the temp_max cache and causes register desynchronization.

Thanks for the review.

The existing code here does look a bit weird, but I need to study the
driver more carefully to be certain. If there are indeed bugs, I will
address them in a new, separate patch.

Please note that these are pre-existing issues and were not introduced
by this patch.

> --
>
> commit 8f551aed0a746551e7fda3272d5c7e3fcaa2b090
> Author: Gui-Dong Han <[email protected]>
>
> hwmon: (adm1031) Hold lock while reading cached data
>
> The patch addresses TOCTOU races by holding data->update_lock while
> accessing cached values in sysfs show and store functions.
>
> Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
>
> > diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
> > index 0551f815233d9..887fba9ea149f 100644
> > --- a/drivers/hwmon/adm1031.c
> > +++ b/drivers/hwmon/adm1031.c
> > @@ -350,9 +350,8 @@ fan_auto_channel_store(struct device *dev, struct device_attribute *attr,
> >       if (ret)
> >               return ret;
> >
> > -     old_fan_mode = data->conf1;
> > -
> >       mutex_lock(&data->update_lock);
> > +     old_fan_mode = data->conf1;
>
> This is not a regression from this patch, but does this store function read
> uninitialized cached values if it is called before adm1031_update_device()
> has populated them?
>
> The driver allocates its data structure with devm_kzalloc() during probe,
> initializing the cache variables (like data->conf1) to zero, but
> adm1031_update_device() is not called during probe to populate them.
>
> If a user writes to this sysfs attribute before any reads, the store
> function performs a read-modify-write using the zeroed data->conf1. When it
> writes data->conf1 back to the ADM1031_REG_CONF1 register, it clears the
> ADM1031_CONF1_MONITOR_ENABLE bit (0x01).
>
> Could this silently disable the chip's hardware monitoring and thermal
> protection?
>
> [ ... ]
>
> Additionally, there appears to be a separate pre-existing regression in
> auto_temp_max_store():
>
> drivers/hwmon/adm1031.c:auto_temp_max_store() {
>     ...
>         val = clamp_val(val, 0, 127000);
>         mutex_lock(&data->update_lock);
>         data->temp_max[nr] = AUTO_TEMP_MAX_TO_REG(val, data->auto_temp[nr],
>                                                   data->pwm[nr]);
>         adm1031_write_value(client, ADM1031_REG_AUTO_TEMP(nr),
>                             data->temp_max[nr]);
>         mutex_unlock(&data->update_lock);
>     ...
> }
>
> Does this corrupt the temp_max cache and cause register desynchronization?
>
> The calculated new automatic temperature limit is assigned to
> data->temp_max[nr] instead of data->auto_temp[nr], corrupting the cache
> used by temp_max_show().
>
> Because data->auto_temp[nr] is left stale, if auto_temp_min_store() is
> called before the cache expires, it will read the old data->auto_temp[nr]
> value and write it back to ADM1031_REG_AUTO_TEMP.
>
> Could this cause the hardware to silently revert the max temperature limit
> just set by the user?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2