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

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Missing I2C error handling leads to hardware register corruption and state desynchronization.
--

commit ffbce3cc3043a4bfb1c8f7ba4a9010b9c8a23c2c
Author: Gui-Dong Han <hanguidong02@gmail.com>

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/20260416091754.310-1-hanguidong02@gmail.com?part=3