[PATCH] hwmon: (lm87) Propagate analog output write errors

[PATCH] hwmon: (lm87) Propagate analog output write errors

[pocj8ur4in]

From: "Baek Kyu, Kim" <pocj8ur4in@gmail.com>

The aout_output store path updates its cached value before writing the
register and ignores the SMBus result. A rejected write therefore returns
a successful byte count and reports the requested value until the cache
expires. With a cached 980 mV, a rejected write of 1960 mV returned 5 and
read back as 1960 mV although the register still held 980 mV.

Check the write result and update the cache only after success. Return the
bus error to the sysfs caller.

Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
Tested on ARM64 QEMU/KVM through the lm87 and adm1024 sysfs paths,
using an emulated SMBus adapter. With 980 mV cached, a pre-apply
-EIO rejection of a 1960 mV write returned 5 and read back 1960 mV
before the change; after the change, it returned -EIO and read back
980 mV. The same behavior was checked with -EREMOTEIO. No physical
chip or analog voltage was measured.

 drivers/hwmon/lm87.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
index c09074f44708..ea23206b3c81 100644
--- a/drivers/hwmon/lm87.c
+++ b/drivers/hwmon/lm87.c
@@ -640,6 +640,7 @@ static ssize_t aout_output_store(struct device *dev,
 {
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
+	u8 reg;
 	long val;
 	int err;
 
@@ -647,11 +648,14 @@ static ssize_t aout_output_store(struct device *dev,
 	if (err)
 		return err;
 
+	reg = AOUT_TO_REG(val);
+
 	mutex_lock(&data->update_lock);
-	data->aout = AOUT_TO_REG(val);
-	lm87_write_value(client, LM87_REG_AOUT, data->aout);
+	err = lm87_write_value(client, LM87_REG_AOUT, reg);
+	if (!err)
+		data->aout = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 static DEVICE_ATTR_RW(aout_output);
 

Re: [PATCH] hwmon: (lm87) Propagate analog output write errors

[sashiko-bot]

> From: "Baek Kyu, Kim" <pocj8ur4in@gmail.com>
> 
> The aout_output store path updates its cached value before writing the
> register and ignores the SMBus result. A rejected write therefore returns
> a successful byte count and reports the requested value until the cache
> expires. With a cached 980 mV, a rejected write of 1960 mV returned 5 and
> read back as 1960 mV although the register still held 980 mV.
> 
> Check the write result and update the cache only after success. Return the
> bus error to the sysfs caller.
> 
> Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025150800.49041-1-pocj8ur4in@gmail.com?part=1

Re: [PATCH] hwmon: (lm87) Propagate analog output write errors

[Guenter Roeck]

On Mon, Oct 26, 2026 at 12:08:00AM +0900, pocj8ur4in wrote:
> From: "Baek Kyu, Kim" <pocj8ur4in@gmail.com>
> 
> The aout_output store path updates its cached value before writing the
> register and ignores the SMBus result. A rejected write therefore returns
> a successful byte count and reports the requested value until the cache
> expires. With a cached 980 mV, a rejected write of 1960 mV returned 5 and
> read back as 1960 mV although the register still held 980 mV.
> 
> Check the write result and update the cache only after success. Return the
> bus error to the sysfs caller.
> 
> Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>

NACK, for a simple reason:

This driver has 23 instances of unchecked I2C writes. This patch fixes
_one_ of them. I am not going to waste my time reviewing 23 separate
patches, each of them fixing one instance of an unchecked write, plus
another set of patches fixing all the unchecked read operations.

Guenter

[PATCH v2 0/3] hwmon: (lm87) Handle I2C read and write errors

[Baek Kyu, Kim]

The LM87 driver does not check I2C transfer results in its sysfs stores,
sensor cache refresh, and device setup. As a result, failed writes can
be reported as successful and failed reads can be used as sensor data.

This series propagates transfer errors to sysfs and probe callers.
Cached settings change only after successful writes; a failed cache
refresh is retried, with alarms already read retained for the retry.

Under ARM64 QEMU/KVM, 46 emulated SMBus scenarios passed for LM87 and
ADM1024. The earlier aout_output regression suite passed all 118 checks.

Baek Kyu, Kim (3):
  hwmon: (lm87) Propagate sysfs write errors
  hwmon: (lm87) Propagate cached sensor read errors
  hwmon: (lm87) Check I2C errors during detection and setup

 drivers/hwmon/lm87.c | 384 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 284 insertions(+), 100 deletions(-)

---
Changes in v2:
  - Address the other unchecked I2C writes and reads noted in the v1
    review, beyond the original aout_output store.
  - Group the changes into sysfs writes, cache refresh, and device
    setup instead of fixing one call site per patch.
  - Preserve alarms if a cache refresh fails after an alarm read.

Link to v1: https://lore.kernel.org/linux-hwmon/20261025150800.49041-1-pocj8ur4in@gmail.com/

-- 
2.55.0

[PATCH v2 1/3] hwmon: (lm87) Propagate sysfs write errors

[Baek Kyu, Kim]

Check the I2C result before updating cached values for writable
attributes. Handle both writes in fan divider changes separately so a
failed second write does not hide the first one.

Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
 drivers/hwmon/lm87.c | 96 ++++++++++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
index c09074f44708..c118a7ff718b 100644
--- a/drivers/hwmon/lm87.c
+++ b/drivers/hwmon/lm87.c
@@ -305,6 +305,7 @@ static ssize_t in_min_store(struct device *dev, struct device_attribute *attr,
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	int nr = to_sensor_dev_attr(attr)->index;
+	u8 reg;
 	long val;
 	int err;
 
@@ -312,12 +313,14 @@ static ssize_t in_min_store(struct device *dev, struct device_attribute *attr,
 	if (err)
 		return err;
 
+	reg = IN_TO_REG(val, data->in_scale[nr]);
 	mutex_lock(&data->update_lock);
-	data->in_min[nr] = IN_TO_REG(val, data->in_scale[nr]);
-	lm87_write_value(client, nr < 6 ? LM87_REG_IN_MIN(nr) :
-			 LM87_REG_AIN_MIN(nr - 6), data->in_min[nr]);
+	err = lm87_write_value(client, nr < 6 ? LM87_REG_IN_MIN(nr) :
+			       LM87_REG_AIN_MIN(nr - 6), reg);
+	if (!err)
+		data->in_min[nr] = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 
 static ssize_t in_max_store(struct device *dev, struct device_attribute *attr,
@@ -326,6 +329,7 @@ static ssize_t in_max_store(struct device *dev, struct device_attribute *attr,
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	int nr = to_sensor_dev_attr(attr)->index;
+	u8 reg;
 	long val;
 	int err;
 
@@ -333,12 +337,14 @@ static ssize_t in_max_store(struct device *dev, struct device_attribute *attr,
 	if (err)
 		return err;
 
+	reg = IN_TO_REG(val, data->in_scale[nr]);
 	mutex_lock(&data->update_lock);
-	data->in_max[nr] = IN_TO_REG(val, data->in_scale[nr]);
-	lm87_write_value(client, nr < 6 ? LM87_REG_IN_MAX(nr) :
-			 LM87_REG_AIN_MAX(nr - 6), data->in_max[nr]);
+	err = lm87_write_value(client, nr < 6 ? LM87_REG_IN_MAX(nr) :
+			       LM87_REG_AIN_MAX(nr - 6), reg);
+	if (!err)
+		data->in_max[nr] = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 
 static SENSOR_DEVICE_ATTR_RO(in0_input, in_input, 0);
@@ -402,6 +408,7 @@ static ssize_t temp_low_store(struct device *dev,
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	int nr = to_sensor_dev_attr(attr)->index;
+	u8 reg;
 	long val;
 	int err;
 
@@ -409,11 +416,13 @@ static ssize_t temp_low_store(struct device *dev,
 	if (err)
 		return err;
 
+	reg = TEMP_TO_REG(val);
 	mutex_lock(&data->update_lock);
-	data->temp_low[nr] = TEMP_TO_REG(val);
-	lm87_write_value(client, LM87_REG_TEMP_LOW[nr], data->temp_low[nr]);
+	err = lm87_write_value(client, LM87_REG_TEMP_LOW[nr], reg);
+	if (!err)
+		data->temp_low[nr] = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 
 static ssize_t temp_high_store(struct device *dev,
@@ -423,6 +432,7 @@ static ssize_t temp_high_store(struct device *dev,
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	int nr = to_sensor_dev_attr(attr)->index;
+	u8 reg;
 	long val;
 	int err;
 
@@ -430,11 +440,13 @@ static ssize_t temp_high_store(struct device *dev,
 	if (err)
 		return err;
 
+	reg = TEMP_TO_REG(val);
 	mutex_lock(&data->update_lock);
-	data->temp_high[nr] = TEMP_TO_REG(val);
-	lm87_write_value(client, LM87_REG_TEMP_HIGH[nr], data->temp_high[nr]);
+	err = lm87_write_value(client, LM87_REG_TEMP_HIGH[nr], reg);
+	if (!err)
+		data->temp_high[nr] = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 
 static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_input, 0);
@@ -502,6 +514,7 @@ static ssize_t fan_min_store(struct device *dev,
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	int nr = to_sensor_dev_attr(attr)->index;
+	u8 reg;
 	long val;
 	int err;
 
@@ -510,11 +523,12 @@ static ssize_t fan_min_store(struct device *dev,
 		return err;
 
 	mutex_lock(&data->update_lock);
-	data->fan_min[nr] = FAN_TO_REG(val,
-			    FAN_DIV_FROM_REG(data->fan_div[nr]));
-	lm87_write_value(client, LM87_REG_FAN_MIN(nr), data->fan_min[nr]);
+	reg = FAN_TO_REG(val, FAN_DIV_FROM_REG(data->fan_div[nr]));
+	err = lm87_write_value(client, LM87_REG_FAN_MIN(nr), reg);
+	if (!err)
+		data->fan_min[nr] = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 
 /*
@@ -533,7 +547,7 @@ static ssize_t fan_div_store(struct device *dev,
 	long val;
 	int err;
 	unsigned long min;
-	u8 reg;
+	u8 reg, div, fan_min;
 
 	err = kstrtol(buf, 10, &val);
 	if (err)
@@ -545,39 +559,47 @@ static ssize_t fan_div_store(struct device *dev,
 
 	switch (val) {
 	case 1:
-		data->fan_div[nr] = 0;
+		div = 0;
 		break;
 	case 2:
-		data->fan_div[nr] = 1;
+		div = 1;
 		break;
 	case 4:
-		data->fan_div[nr] = 2;
+		div = 2;
 		break;
 	case 8:
-		data->fan_div[nr] = 3;
+		div = 3;
 		break;
 	default:
 		mutex_unlock(&data->update_lock);
 		return -EINVAL;
 	}
 
-	reg = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
+	err = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
+	if (err < 0)
+		goto out;
+	reg = err;
 	switch (nr) {
 	case 0:
-	    reg = (reg & 0xCF) | (data->fan_div[0] << 4);
+	    reg = (reg & 0xCF) | (div << 4);
 	    break;
 	case 1:
-	    reg = (reg & 0x3F) | (data->fan_div[1] << 6);
+	    reg = (reg & 0x3F) | (div << 6);
 	    break;
 	}
-	lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
-
-	data->fan_min[nr] = FAN_TO_REG(min, val);
-	lm87_write_value(client, LM87_REG_FAN_MIN(nr),
-			 data->fan_min[nr]);
+	err = lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
+	if (err < 0)
+		goto out;
+	data->fan_div[nr] = div;
+
+	fan_min = FAN_TO_REG(min, val);
+	err = lm87_write_value(client, LM87_REG_FAN_MIN(nr), fan_min);
+	if (!err)
+		data->fan_min[nr] = fan_min;
+out:
 	mutex_unlock(&data->update_lock);
 
-	return count;
+	return err < 0 ? err : count;
 }
 
 static SENSOR_DEVICE_ATTR_RO(fan1_input, fan_input, 0);
@@ -640,6 +662,7 @@ static ssize_t aout_output_store(struct device *dev,
 {
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
+	u8 reg;
 	long val;
 	int err;
 
@@ -647,11 +670,14 @@ static ssize_t aout_output_store(struct device *dev,
 	if (err)
 		return err;
 
+	reg = AOUT_TO_REG(val);
+
 	mutex_lock(&data->update_lock);
-	data->aout = AOUT_TO_REG(val);
-	lm87_write_value(client, LM87_REG_AOUT, data->aout);
+	err = lm87_write_value(client, LM87_REG_AOUT, reg);
+	if (!err)
+		data->aout = reg;
 	mutex_unlock(&data->update_lock);
-	return count;
+	return err < 0 ? err : count;
 }
 static DEVICE_ATTR_RW(aout_output);
 
-- 
2.55.0

[PATCH v2 2/3] hwmon: (lm87) Propagate cached sensor read errors

[Baek Kyu, Kim]

Abort cache refresh when an I2C read fails and pass the error to
sysfs readers instead of returning stale or invalid sensor values.

Read the alarm registers last, since reading them can clear latched
alarms. Preserve the first alarm byte if reading the second fails,
so a retry can still report the captured alarm.

Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
 drivers/hwmon/lm87.c | 202 +++++++++++++++++++++++++++++++++----------
 1 file changed, 158 insertions(+), 44 deletions(-)

diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
index c118a7ff718b..a66d486b314c 100644
--- a/drivers/hwmon/lm87.c
+++ b/drivers/hwmon/lm87.c
@@ -170,6 +170,7 @@ struct lm87_data {
 	u8 aout;		/* register value */
 
 	u16 alarms;		/* register values, combined */
+	u16 pending_alarms;	/* alarms read before an I2C error */
 	u8 vid;			/* register values, combined */
 	u8 vrm;
 
@@ -186,75 +187,138 @@ static inline int lm87_write_value(struct i2c_client *client, u8 reg, u8 value)
 	return i2c_smbus_write_byte_data(client, reg, value);
 }
 
+static int lm87_read_reg(struct i2c_client *client, u8 reg, u8 *value)
+{
+	int ret = lm87_read_value(client, reg);
+
+	if (ret < 0)
+		return ret;
+	*value = ret;
+	return 0;
+}
+
+static int lm87_read_temp(struct i2c_client *client, u8 reg, s8 *value)
+{
+	int ret = lm87_read_value(client, reg);
+
+	if (ret < 0)
+		return ret;
+	*value = ret;
+	return 0;
+}
+
 static struct lm87_data *lm87_update_device(struct device *dev)
 {
 	struct i2c_client *client = dev_get_drvdata(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
+	int i, j, ret;
+	u8 first, second;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		int i, j;
-
 		dev_dbg(&client->dev, "Updating data.\n");
 
 		i = (data->channel & CHAN_TEMP3) ? 1 : 0;
 		j = (data->channel & CHAN_TEMP3) ? 5 : 6;
 		for (; i < j; i++) {
-			data->in[i] = lm87_read_value(client,
-				      LM87_REG_IN(i));
-			data->in_min[i] = lm87_read_value(client,
-					  LM87_REG_IN_MIN(i));
-			data->in_max[i] = lm87_read_value(client,
-					  LM87_REG_IN_MAX(i));
+			ret = lm87_read_reg(client, LM87_REG_IN(i), &data->in[i]);
+			if (ret < 0)
+				goto error;
+			ret = lm87_read_reg(client, LM87_REG_IN_MIN(i),
+					    &data->in_min[i]);
+			if (ret < 0)
+				goto error;
+			ret = lm87_read_reg(client, LM87_REG_IN_MAX(i),
+					    &data->in_max[i]);
+			if (ret < 0)
+				goto error;
 		}
 
 		for (i = 0; i < 2; i++) {
 			if (data->channel & CHAN_NO_FAN(i)) {
-				data->in[6+i] = lm87_read_value(client,
-						LM87_REG_AIN(i));
-				data->in_max[6+i] = lm87_read_value(client,
-						    LM87_REG_AIN_MAX(i));
-				data->in_min[6+i] = lm87_read_value(client,
-						    LM87_REG_AIN_MIN(i));
-
+				ret = lm87_read_reg(client, LM87_REG_AIN(i),
+						    &data->in[6 + i]);
+				if (ret < 0)
+					goto error;
+				ret = lm87_read_reg(client, LM87_REG_AIN_MAX(i),
+						    &data->in_max[6 + i]);
+				if (ret < 0)
+					goto error;
+				ret = lm87_read_reg(client, LM87_REG_AIN_MIN(i),
+						    &data->in_min[6 + i]);
+				if (ret < 0)
+					goto error;
 			} else {
-				data->fan[i] = lm87_read_value(client,
-					       LM87_REG_FAN(i));
-				data->fan_min[i] = lm87_read_value(client,
-						   LM87_REG_FAN_MIN(i));
+				ret = lm87_read_reg(client, LM87_REG_FAN(i),
+						    &data->fan[i]);
+				if (ret < 0)
+					goto error;
+				ret = lm87_read_reg(client, LM87_REG_FAN_MIN(i),
+						    &data->fan_min[i]);
+				if (ret < 0)
+					goto error;
 			}
 		}
 
 		j = (data->channel & CHAN_TEMP3) ? 3 : 2;
 		for (i = 0 ; i < j; i++) {
-			data->temp[i] = lm87_read_value(client,
-					LM87_REG_TEMP[i]);
-			data->temp_high[i] = lm87_read_value(client,
-					     LM87_REG_TEMP_HIGH[i]);
-			data->temp_low[i] = lm87_read_value(client,
-					    LM87_REG_TEMP_LOW[i]);
+			ret = lm87_read_temp(client, LM87_REG_TEMP[i],
+					     &data->temp[i]);
+			if (ret < 0)
+				goto error;
+			ret = lm87_read_temp(client, LM87_REG_TEMP_HIGH[i],
+					     &data->temp_high[i]);
+			if (ret < 0)
+				goto error;
+			ret = lm87_read_temp(client, LM87_REG_TEMP_LOW[i],
+					     &data->temp_low[i]);
+			if (ret < 0)
+				goto error;
 		}
 
-		i = lm87_read_value(client, LM87_REG_TEMP_HW_INT_LOCK);
-		j = lm87_read_value(client, LM87_REG_TEMP_HW_INT);
-		data->temp_crit_int = min(i, j);
-
-		i = lm87_read_value(client, LM87_REG_TEMP_HW_EXT_LOCK);
-		j = lm87_read_value(client, LM87_REG_TEMP_HW_EXT);
-		data->temp_crit_ext = min(i, j);
-
-		i = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
-		data->fan_div[0] = (i >> 4) & 0x03;
-		data->fan_div[1] = (i >> 6) & 0x03;
-		data->vid = (i & 0x0F)
-			  | (lm87_read_value(client, LM87_REG_VID4) & 0x01)
-			     << 4;
-
-		data->alarms = lm87_read_value(client, LM87_REG_ALARMS1)
-			     | (lm87_read_value(client, LM87_REG_ALARMS2)
-				<< 8);
-		data->aout = lm87_read_value(client, LM87_REG_AOUT);
+		ret = lm87_read_reg(client, LM87_REG_TEMP_HW_INT_LOCK,
+				    &first);
+		if (ret < 0)
+			goto error;
+		ret = lm87_read_reg(client, LM87_REG_TEMP_HW_INT, &second);
+		if (ret < 0)
+			goto error;
+		data->temp_crit_int = min(first, second);
+
+		ret = lm87_read_reg(client, LM87_REG_TEMP_HW_EXT_LOCK,
+				    &first);
+		if (ret < 0)
+			goto error;
+		ret = lm87_read_reg(client, LM87_REG_TEMP_HW_EXT, &second);
+		if (ret < 0)
+			goto error;
+		data->temp_crit_ext = min(first, second);
+
+		ret = lm87_read_reg(client, LM87_REG_VID_FAN_DIV, &first);
+		if (ret < 0)
+			goto error;
+		data->fan_div[0] = (first >> 4) & 0x03;
+		data->fan_div[1] = (first >> 6) & 0x03;
+		ret = lm87_read_reg(client, LM87_REG_VID4, &second);
+		if (ret < 0)
+			goto error;
+		data->vid = (first & 0x0F) | (second & 0x01) << 4;
+
+		ret = lm87_read_reg(client, LM87_REG_AOUT, &data->aout);
+		if (ret < 0)
+			goto error;
+
+		ret = lm87_read_reg(client, LM87_REG_ALARMS1, &first);
+		if (ret < 0)
+			goto error;
+		ret = lm87_read_reg(client, LM87_REG_ALARMS2, &second);
+		if (ret < 0) {
+			data->pending_alarms |= first;
+			goto error;
+		}
+		data->alarms = data->pending_alarms | first | second << 8;
+		data->pending_alarms = 0;
 
 		data->last_updated = jiffies;
 		data->valid = true;
@@ -263,6 +327,11 @@ static struct lm87_data *lm87_update_device(struct device *dev)
 	mutex_unlock(&data->update_lock);
 
 	return data;
+
+error:
+	data->valid = false;
+	mutex_unlock(&data->update_lock);
+	return ERR_PTR(ret);
 }
 
 /*
@@ -275,6 +344,9 @@ static ssize_t in_input_show(struct device *dev,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%u\n", IN_FROM_REG(data->in[nr],
 		       data->in_scale[nr]));
 }
@@ -285,6 +357,9 @@ static ssize_t in_min_show(struct device *dev, struct device_attribute *attr,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_min[nr],
 		       data->in_scale[nr]));
 }
@@ -295,6 +370,9 @@ static ssize_t in_max_show(struct device *dev, struct device_attribute *attr,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%u\n", IN_FROM_REG(data->in_max[nr],
 		       data->in_scale[nr]));
 }
@@ -378,6 +456,9 @@ static ssize_t temp_input_show(struct device *dev,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr]));
 }
 
@@ -387,6 +468,9 @@ static ssize_t temp_low_show(struct device *dev,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n",
 		       TEMP_FROM_REG(data->temp_low[nr]));
 }
@@ -397,6 +481,9 @@ static ssize_t temp_high_show(struct device *dev,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n",
 		       TEMP_FROM_REG(data->temp_high[nr]));
 }
@@ -463,6 +550,9 @@ static ssize_t temp1_crit_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	struct lm87_data *data = lm87_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_crit_int));
 }
 
@@ -470,6 +560,9 @@ static ssize_t temp2_crit_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	struct lm87_data *data = lm87_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_crit_ext));
 }
 
@@ -483,6 +576,9 @@ static ssize_t fan_input_show(struct device *dev,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", fan_from_reg(data->fan[nr],
 		       FAN_DIV_FROM_REG(data->fan_div[nr])));
 }
@@ -493,6 +589,9 @@ static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", fan_from_reg(data->fan_min[nr],
 		       FAN_DIV_FROM_REG(data->fan_div[nr])));
 }
@@ -503,6 +602,9 @@ static ssize_t fan_div_show(struct device *dev, struct device_attribute *attr,
 	struct lm87_data *data = lm87_update_device(dev);
 	int nr = to_sensor_dev_attr(attr)->index;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n",
 		       FAN_DIV_FROM_REG(data->fan_div[nr]));
 }
@@ -613,6 +715,9 @@ static ssize_t alarms_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
 	struct lm87_data *data = lm87_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", data->alarms);
 }
 static DEVICE_ATTR_RO(alarms);
@@ -621,6 +726,9 @@ static ssize_t cpu0_vid_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
 	struct lm87_data *data = lm87_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", vid_from_reg(data->vid, data->vrm));
 }
 static DEVICE_ATTR_RO(cpu0_vid);
@@ -654,6 +762,9 @@ static ssize_t aout_output_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct lm87_data *data = lm87_update_device(dev);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%d\n", AOUT_FROM_REG(data->aout));
 }
 static ssize_t aout_output_store(struct device *dev,
@@ -686,6 +797,9 @@ static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
 {
 	struct lm87_data *data = lm87_update_device(dev);
 	int bitnr = to_sensor_dev_attr(attr)->index;
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	return sprintf(buf, "%u\n", (data->alarms >> bitnr) & 1);
 }
 static SENSOR_DEVICE_ATTR_RO(in0_alarm, alarm, 0);
-- 
2.55.0

[PATCH v2 3/3] hwmon: (lm87) Check I2C errors during detection and setup

[Baek Kyu, Kim]

Reject devices whose identification registers cannot be read, fail
initialization on transfer errors, and report configuration restore
failures.

Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
 drivers/hwmon/lm87.c | 86 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
index a66d486b314c..32ec44f583eb 100644
--- a/drivers/hwmon/lm87.c
+++ b/drivers/hwmon/lm87.c
@@ -955,17 +955,22 @@ static int lm87_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
 	struct i2c_adapter *adapter = client->adapter;
 	const char *name;
-	u8 cid, rev;
+	int cid, rev, config;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
 
-	if (lm87_read_value(client, LM87_REG_CONFIG) & 0x80)
+	config = lm87_read_value(client, LM87_REG_CONFIG);
+	if (config < 0 || config & 0x80)
 		return -ENODEV;
 
 	/* Now, we do the remaining detection. */
 	cid = lm87_read_value(client, LM87_REG_COMPANY_ID);
+	if (cid < 0)
+		return -ENODEV;
 	rev = lm87_read_value(client, LM87_REG_REVISION);
+	if (rev < 0)
+		return -ENODEV;
 
 	if (cid == 0x02			/* National Semiconductor */
 	 && (rev >= 0x01 && rev <= 0x08))
@@ -989,7 +994,12 @@ static void lm87_restore_config(void *arg)
 	struct i2c_client *client = arg;
 	struct lm87_data *data = i2c_get_clientdata(client);
 
-	lm87_write_value(client, LM87_REG_CONFIG, data->config);
+	int err;
+
+	err = lm87_write_value(client, LM87_REG_CONFIG, data->config);
+	if (err < 0)
+		dev_warn(&client->dev, "Failed to restore configuration: %d\n",
+			 err);
 }
 
 static int lm87_init_client(struct i2c_client *client)
@@ -1013,16 +1023,26 @@ static int lm87_init_client(struct i2c_client *client)
 				val |= CHAN_VCC_5V;
 		}
 		data->channel = val;
-		lm87_write_value(client,
-				LM87_REG_CHANNEL_MODE, data->channel);
+		rc = lm87_write_value(client, LM87_REG_CHANNEL_MODE,
+				      data->channel);
+		if (rc < 0)
+			return rc;
 	} else if (dev_get_platdata(&client->dev)) {
 		data->channel = *(u8 *)dev_get_platdata(&client->dev);
-		lm87_write_value(client,
-				 LM87_REG_CHANNEL_MODE, data->channel);
+		rc = lm87_write_value(client, LM87_REG_CHANNEL_MODE,
+				      data->channel);
+		if (rc < 0)
+			return rc;
 	} else {
-		data->channel = lm87_read_value(client, LM87_REG_CHANNEL_MODE);
+		rc = lm87_read_value(client, LM87_REG_CHANNEL_MODE);
+		if (rc < 0)
+			return rc;
+		data->channel = rc;
 	}
-	data->config = lm87_read_value(client, LM87_REG_CONFIG) & 0x6F;
+	rc = lm87_read_value(client, LM87_REG_CONFIG);
+	if (rc < 0)
+		return rc;
+	data->config = rc & 0x6F;
 
 	rc = devm_add_action(&client->dev, lm87_restore_config, client);
 	if (rc)
@@ -1033,28 +1053,52 @@ static int lm87_init_client(struct i2c_client *client)
 
 		/* Limits are left uninitialized after power-up */
 		for (i = 1; i < 6; i++) {
-			lm87_write_value(client, LM87_REG_IN_MIN(i), 0x00);
-			lm87_write_value(client, LM87_REG_IN_MAX(i), 0xFF);
+			rc = lm87_write_value(client, LM87_REG_IN_MIN(i), 0x00);
+			if (rc < 0)
+				return rc;
+			rc = lm87_write_value(client, LM87_REG_IN_MAX(i), 0xFF);
+			if (rc < 0)
+				return rc;
 		}
 		for (i = 0; i < 2; i++) {
-			lm87_write_value(client, LM87_REG_TEMP_HIGH[i], 0x7F);
-			lm87_write_value(client, LM87_REG_TEMP_LOW[i], 0x00);
-			lm87_write_value(client, LM87_REG_AIN_MIN(i), 0x00);
-			lm87_write_value(client, LM87_REG_AIN_MAX(i), 0xFF);
+			rc = lm87_write_value(client, LM87_REG_TEMP_HIGH[i],
+					      0x7F);
+			if (rc < 0)
+				return rc;
+			rc = lm87_write_value(client, LM87_REG_TEMP_LOW[i],
+					      0x00);
+			if (rc < 0)
+				return rc;
+			rc = lm87_write_value(client, LM87_REG_AIN_MIN(i), 0x00);
+			if (rc < 0)
+				return rc;
+			rc = lm87_write_value(client, LM87_REG_AIN_MAX(i), 0xFF);
+			if (rc < 0)
+				return rc;
 		}
 		if (data->channel & CHAN_TEMP3) {
-			lm87_write_value(client, LM87_REG_TEMP_HIGH[2], 0x7F);
-			lm87_write_value(client, LM87_REG_TEMP_LOW[2], 0x00);
+			rc = lm87_write_value(client, LM87_REG_TEMP_HIGH[2],
+					      0x7F);
+			if (rc < 0)
+				return rc;
+			rc = lm87_write_value(client, LM87_REG_TEMP_LOW[2],
+					      0x00);
+			if (rc < 0)
+				return rc;
 		} else {
-			lm87_write_value(client, LM87_REG_IN_MIN(0), 0x00);
-			lm87_write_value(client, LM87_REG_IN_MAX(0), 0xFF);
+			rc = lm87_write_value(client, LM87_REG_IN_MIN(0), 0x00);
+			if (rc < 0)
+				return rc;
+			rc = lm87_write_value(client, LM87_REG_IN_MAX(0), 0xFF);
+			if (rc < 0)
+				return rc;
 		}
 	}
 
 	/* Make sure Start is set and INT#_Clear is clear */
 	if ((data->config & 0x09) != 0x01)
-		lm87_write_value(client, LM87_REG_CONFIG,
-				 (data->config & 0x77) | 0x01);
+		return lm87_write_value(client, LM87_REG_CONFIG,
+					(data->config & 0x77) | 0x01);
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH v2 1/3] hwmon: (lm87) Propagate sysfs write errors

[sashiko-bot]

> Check the I2C result before updating cached values for writable
> attributes. Handle both writes in fan divider changes separately so a
> failed second write does not hide the first one.
> 
> Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029164016.338450-1-pocj8ur4in@gmail.com?part=1

Re: [PATCH v2 2/3] hwmon: (lm87) Propagate cached sensor read errors

[sashiko-bot]

> Abort cache refresh when an I2C read fails and pass the error to
> sysfs readers instead of returning stale or invalid sensor values.
> 
> Read the alarm registers last, since reading them can clear latched
> alarms. Preserve the first alarm byte if reading the second fails,
> so a retry can still report the captured alarm.
> 
> Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029164016.338450-1-pocj8ur4in@gmail.com?part=2

Re: [PATCH v2 3/3] hwmon: (lm87) Check I2C errors during detection and setup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] hwmon: (lm87) premature devm_add_action() in lm87_init_client() on error path
--

commit c6264ca6c303f980a5123962ced0900012b6d8d6
Author: Baek Kyu, Kim <pocj8ur4in@gmail.com>

hwmon: (lm87) Check I2C errors during detection and setup

This commit adds error checking for SMBus read and write operations in
lm87_detect(), lm87_restore_config(), and lm87_init_client().

> diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
> index a66d486b314cf..32ec44f583eb8 100644
> --- a/drivers/hwmon/lm87.c
> +++ b/drivers/hwmon/lm87.c

[ ... ]

> @@ -989,7 +994,12 @@ static void lm87_restore_config(void *arg)
>  	struct i2c_client *client = arg;
>  	struct lm87_data *data = i2c_get_clientdata(client);
>  
> -	lm87_write_value(client, LM87_REG_CONFIG, data->config);
> +	int err;
> +
> +	err = lm87_write_value(client, LM87_REG_CONFIG, data->config);
> +	if (err < 0)
> +		dev_warn(&client->dev, "Failed to restore configuration: %d\n",
> +			 err);
>  }
>  
>  static int lm87_init_client(struct i2c_client *client)
> @@ -1013,16 +1023,26 @@ static int lm87_init_client(struct i2c_client *client)

[ ... ]

> -	data->config = lm87_read_value(client, LM87_REG_CONFIG) & 0x6F;
> +	rc = lm87_read_value(client, LM87_REG_CONFIG);
> +	if (rc < 0)
> +		return rc;
> +	data->config = rc & 0x6F;
>  
>  	rc = devm_add_action(&client->dev, lm87_restore_config, client);
>  	if (rc)
> @@ -1033,28 +1053,52 @@ static int lm87_init_client(struct i2c_client *client)
>  
>  		/* Limits are left uninitialized after power-up */
>  		for (i = 1; i < 6; i++) {
> -			lm87_write_value(client, LM87_REG_IN_MIN(i), 0x00);
> -			lm87_write_value(client, LM87_REG_IN_MAX(i), 0xFF);
> +			rc = lm87_write_value(client, LM87_REG_IN_MIN(i), 0x00);
> +			if (rc < 0)
> +				return rc;
> +			rc = lm87_write_value(client, LM87_REG_IN_MAX(i), 0xFF);
> +			if (rc < 0)
> +				return rc;
>  		}

[ ... ]

>  	/* Make sure Start is set and INT#_Clear is clear */
>  	if ((data->config & 0x09) != 0x01)
> -		lm87_write_value(client, LM87_REG_CONFIG,
> -				 (data->config & 0x77) | 0x01);
> +		return lm87_write_value(client, LM87_REG_CONFIG,
> +					(data->config & 0x77) | 0x01);
>  	return 0;
>  }

[Severity: Low]
In drivers/hwmon/lm87.c, lm87_init_client() registers lm87_restore_config()
via devm_add_action() before writing the sensor limit registers and before
updating LM87_REG_CONFIG at the end of the function:

	/* Make sure Start is set and INT#_Clear is clear */
	if ((data->config & 0x09) != 0x01)
		return lm87_write_value(client, LM87_REG_CONFIG,
					(data->config & 0x77) | 0x01);

Now that lm87_init_client() returns early when any of the limit register
writes or the final LM87_REG_CONFIG write fails, devres cleanup will run
lm87_restore_config() during probe unwind even though LM87_REG_CONFIG was
never modified. On an already failing bus, that issues an extra write to
LM87_REG_CONFIG and logs a "Failed to restore configuration" warning.

Should devm_add_action() (or devm_add_action_or_reset()) only be called
after LM87_REG_CONFIG has been successfully modified?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029164016.338450-1-pocj8ur4in@gmail.com?part=3

Re: [PATCH v2 0/3] hwmon: (lm87) Handle I2C read and write errors

[Guenter Roeck]

Hi,

On 10/29/26 09:40, Baek Kyu, Kim wrote:
> The LM87 driver does not check I2C transfer results in its sysfs stores,
> sensor cache refresh, and device setup. As a result, failed writes can
> be reported as successful and failed reads can be used as sensor data.
> 
> This series propagates transfer errors to sysfs and probe callers.
> Cached settings change only after successful writes; a failed cache
> refresh is retried, with alarms already read retained for the retry.
> 
> Under ARM64 QEMU/KVM, 46 emulated SMBus scenarios passed for LM87 and
> ADM1024. The earlier aout_output regression suite passed all 118 checks.
> 

This driver is more than 20 years old. It survived all that time without
i2c error checks, and it is very questionable if it is still active on
any actual hardware.

Please refrain from touching such drivers unless there is a real problem,
observed in the field.

Thanks,
Guenter