[PATCH 1/3] hwmon: (adm1031) Convert macros to functions to avoid TOCTOU

[PATCH 1/3] hwmon: (adm1031) Convert macros to functions to avoid TOCTOU

[Gui-Dong Han]

The macros TEMP_OFFSET_FROM_REG, FAN_FROM_REG, and
AUTO_TEMP_MAX_FROM_REG evaluate their arguments multiple times. When
used in lockless code accessing shared driver data, this can cause
Time-of-Check to Time-of-Use (TOCTOU) races. In the case of
FAN_FROM_REG, it can also result in a divide-by-zero error.

Convert those macros to static functions so that their arguments are
always evaluated only once.

Check the remaining conversion macros in the driver as well. Keep them
unchanged because they either do not evaluate arguments multiple times
or are only used from locked code paths.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: 49dc9efed05a ("hwmon: (adm1031) Add sysfs files for temperature offsets")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
While learning the hwmon driver code, I found a few more potential
TOCTOU problems in drivers still using the older non-_with_info() APIs.
Fix them.
---
 drivers/hwmon/adm1031.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 343118532cdb..0551f815233d 100644
--- a/drivers/hwmon/adm1031.c
+++ b/drivers/hwmon/adm1031.c
@@ -203,16 +203,24 @@ static struct adm1031_data *adm1031_update_device(struct device *dev)
 #define TEMP_FROM_REG_EXT(val, ext)	(TEMP_FROM_REG(val) + (ext) * 125)
 
 #define TEMP_OFFSET_TO_REG(val)		(TEMP_TO_REG(val) & 0x8f)
-#define TEMP_OFFSET_FROM_REG(val)	TEMP_FROM_REG((val) < 0 ? \
-						      (val) | 0x70 : (val))
 
-#define FAN_FROM_REG(reg, div)		((reg) ? \
-					 (11250 * 60) / ((reg) * (div)) : 0)
+static int temp_offset_from_reg(int val)
+{
+	return TEMP_FROM_REG(val < 0 ? val | 0x70 : val);
+}
+
+static int fan_from_reg(int reg, int div)
+{
+	if (!reg)
+		return 0;
+
+	return (11250 * 60) / (reg * div);
+}
 
 static int FAN_TO_REG(int reg, int div)
 {
 	int tmp;
-	tmp = FAN_FROM_REG(clamp_val(reg, 0, 65535), div);
+	tmp = fan_from_reg(clamp_val(reg, 0, 65535), div);
 	return tmp > 255 ? 255 : tmp;
 }
 
@@ -235,9 +243,10 @@ static int FAN_TO_REG(int reg, int div)
 #define AUTO_TEMP_OFF_FROM_REG(reg)		\
 	(AUTO_TEMP_MIN_FROM_REG(reg) - 5000)
 
-#define AUTO_TEMP_MAX_FROM_REG(reg)		\
-	(AUTO_TEMP_RANGE_FROM_REG(reg) +	\
-	AUTO_TEMP_MIN_FROM_REG(reg))
+static int auto_temp_max_from_reg(int reg)
+{
+	return AUTO_TEMP_RANGE_FROM_REG(reg) + AUTO_TEMP_MIN_FROM_REG(reg);
+}
 
 static int AUTO_TEMP_MAX_TO_REG(int val, int reg, int pwm)
 {
@@ -426,7 +435,7 @@ static ssize_t auto_temp_max_show(struct device *dev,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       AUTO_TEMP_MAX_FROM_REG(data->auto_temp[nr]));
+		       auto_temp_max_from_reg(data->auto_temp[nr]));
 }
 static ssize_t
 auto_temp_max_store(struct device *dev, struct device_attribute *attr,
@@ -559,7 +568,7 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *attr,
 	struct adm1031_data *data = adm1031_update_device(dev);
 	int value;
 
-	value = trust_fan_readings(data, nr) ? FAN_FROM_REG(data->fan[nr],
+	value = trust_fan_readings(data, nr) ? fan_from_reg(data->fan[nr],
 				 FAN_DIV_FROM_REG(data->fan_div[nr])) : 0;
 	return sprintf(buf, "%d\n", value);
 }
@@ -577,7 +586,7 @@ static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       FAN_FROM_REG(data->fan_min[nr],
+		       fan_from_reg(data->fan_min[nr],
 				    FAN_DIV_FROM_REG(data->fan_div[nr])));
 }
 static ssize_t fan_min_store(struct device *dev,
@@ -679,7 +688,7 @@ static ssize_t temp_offset_show(struct device *dev,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	return sprintf(buf, "%d\n",
-		       TEMP_OFFSET_FROM_REG(data->temp_offset[nr]));
+		       temp_offset_from_reg(data->temp_offset[nr]));
 }
 static ssize_t temp_min_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
-- 
2.43.0

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

[Gui-Dong Han]

The functions fan_show(), fan_min_show(), and temp_show() read shared
cached values multiple times without holding data->update_lock.
fan_auto_channel_store() also reads data->conf1 before taking the lock.
Those cached values can change in adm1031_update_device(), resulting in
inconsistent snapshots and TOCTOU races.

Hold data->update_lock across those reads so that the cached values stay
stable while the results are calculated.

Check the remaining functions in the driver as well. Keep them unchanged
because they either do not access shared cached values multiple times
or already do so under lock.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
While learning the hwmon driver code, I found a few more potential
TOCTOU problems in drivers still using the older non-_with_info() APIs.
Fix them.
---
 drivers/hwmon/adm1031.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 0551f815233d..887fba9ea149 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;
 
 	ret = get_fan_auto_nearest(data, nr, val, data->conf1);
 	if (ret < 0) {
@@ -568,8 +567,10 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *attr,
 	struct adm1031_data *data = adm1031_update_device(dev);
 	int value;
 
+	mutex_lock(&data->update_lock);
 	value = trust_fan_readings(data, nr) ? fan_from_reg(data->fan[nr],
 				 FAN_DIV_FROM_REG(data->fan_div[nr])) : 0;
+	mutex_unlock(&data->update_lock);
 	return sprintf(buf, "%d\n", value);
 }
 
@@ -585,9 +586,13 @@ static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
 {
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
-	return sprintf(buf, "%d\n",
-		       fan_from_reg(data->fan_min[nr],
-				    FAN_DIV_FROM_REG(data->fan_div[nr])));
+	int value;
+
+	mutex_lock(&data->update_lock);
+	value = fan_from_reg(data->fan_min[nr],
+			     FAN_DIV_FROM_REG(data->fan_div[nr]));
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", value);
 }
 static ssize_t fan_min_store(struct device *dev,
 			     struct device_attribute *attr, const char *buf,
@@ -677,10 +682,15 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
 	int nr = to_sensor_dev_attr(attr)->index;
 	struct adm1031_data *data = adm1031_update_device(dev);
 	int ext;
+	int temp;
+
+	mutex_lock(&data->update_lock);
 	ext = nr == 0 ?
 	    ((data->ext_temp[nr] >> 6) & 0x3) * 2 :
 	    (((data->ext_temp[nr] >> ((nr - 1) * 3)) & 7));
-	return sprintf(buf, "%d\n", TEMP_FROM_REG_EXT(data->temp[nr], ext));
+	temp = TEMP_FROM_REG_EXT(data->temp[nr], ext);
+	mutex_unlock(&data->update_lock);
+	return sprintf(buf, "%d\n", temp);
 }
 static ssize_t temp_offset_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
-- 
2.43.0

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

[Gui-Dong Han]

update_interval_store() updates the hardware rate bits in
ADM1031_REG_FAN_FILTER and then updates data->update_interval. Those
steps are currently split across unlocked and locked sections. This also
leaves the register read-modify-write sequence unprotected.

Hold data->update_lock across the whole sequence so that the register
update and the cached software state remain synchronized.

Fixes: 87c33daadbfe ("hwmon: (adm1031) Allow setting update rate")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
 drivers/hwmon/adm1031.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 887fba9ea149..a46db83471f2 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);
 	reg &= ~ADM1031_UPDATE_RATE_MASK;
 	reg |= i << ADM1031_UPDATE_RATE_SHIFT;
 	adm1031_write_value(client, ADM1031_REG_FAN_FILTER, reg);
 
-	mutex_lock(&data->update_lock);
 	data->update_interval = update_intervals[i];
 	mutex_unlock(&data->update_lock);
 
-- 
2.43.0

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

[sashiko-bot]

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

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

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

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

[sashiko-bot]

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

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

[Gui-Dong Han]

On Thu, Apr 16, 2026 at 7:57 PM <sashiko-bot@kernel.org> 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 <hanguidong02@gmail.com>
>
> 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/20260416091754.310-1-hanguidong02@gmail.com?part=2

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

[Gui-Dong Han]

On Thu, Apr 16, 2026 at 8:21 PM <sashiko-bot@kernel.org> 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 <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

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

[Guenter Roeck]

On Thu, Apr 16, 2026 at 09:05:05PM +0800, Gui-Dong Han wrote:
> On Thu, Apr 16, 2026 at 8:21 PM <sashiko-bot@kernel.org> 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 <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
> 

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

[Gui-Dong Han]

On Thu, Apr 16, 2026 at 10:05 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Thu, Apr 16, 2026 at 09:05:05PM +0800, Gui-Dong Han wrote:
> > On Thu, Apr 16, 2026 at 8:21 PM <sashiko-bot@kernel.org> 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 <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
> >