[PATCH 0/5] hwmon: (pmbus/core) Introduce and use pmbus_read_smbus_i2c_block_data()

[PATCH 0/5] hwmon: (pmbus/core) Introduce and use pmbus_read_smbus_i2c_block_data()

[Guenter Roeck]

PMBus drivers need support for reading SMBus block data. Unfortunately,
not all i2C controllers support this command.

Implement pmbus_read_smbus_i2c_block_data() which first tries to use
i2c_smbus_read_block_data(). If not supported, try to emulate it by calling
i2c_smbus_read_i2c_block_data(). Export the new function for use in PMBus
drivers, and use it in affected drivers.

----------------------------------------------------------------
Guenter Roeck (5):
      hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()
      hwmon: (pmbus/adm1275) Use pmbus_read_smbus_i2c_block_data for block commands
      hwmon: (pmbus/ltc2978) Use pmbus_read_smbus_i2c_block_data for block commands
      hwmon: (pmbus/max20830) Use pmbus_read_smbus_i2c_block_data for block commands
      hwmon: (pmbus/ir36021) Use pmbus_read_smbus_i2c_block_data for block commands

 drivers/hwmon/pmbus/adm1275.c    |   9 +---
 drivers/hwmon/pmbus/ir36021.c    |  22 +++------
 drivers/hwmon/pmbus/ltc2978.c    |  21 ++++----
 drivers/hwmon/pmbus/max20830.c   |  34 ++-----------
 drivers/hwmon/pmbus/pmbus.h      |   1 +
 drivers/hwmon/pmbus/pmbus_core.c | 102 ++++++++++++++++++++++++++++++++++++---
 6 files changed, 117 insertions(+), 72 deletions(-)

[PATCH 1/5] hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()

[Guenter Roeck]

PMBus drivers need support for reading SMBus block data. Unfortunately,
not all i2C controllers support this command.

Implement pmbus_read_smbus_i2c_block_data() which first tries to use
i2c_smbus_read_block_data(). If not supported, try to emulate it by calling
i2c_smbus_read_i2c_block_data(). Export the new function for use in PMBus
drivers.

Cc: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/pmbus.h      |   1 +
 drivers/hwmon/pmbus/pmbus_core.c | 102 ++++++++++++++++++++++++++++---
 2 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 5fe2c415eada..2cd3216b3cd9 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -561,6 +561,7 @@ void pmbus_set_update(struct i2c_client *client, u8 reg, bool update);
 void pmbus_wait(struct i2c_client *client);
 void pmbus_update_ts(struct i2c_client *client, int op);
 int pmbus_set_page(struct i2c_client *client, int page, int phase);
+int pmbus_read_smbus_i2c_block_data(struct i2c_client *client, u8 reg, char *data_buf);
 int pmbus_read_word_data(struct i2c_client *client, int page, int phase,
 			 u8 reg);
 int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 6991c7f8d125..1e09abbc05d3 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -182,7 +182,12 @@ EXPORT_SYMBOL_NS_GPL(pmbus_set_update, "PMBUS");
 void pmbus_wait(struct i2c_client *client)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
-	s64 delay = ktime_us_delta(data->next_access_backoff, ktime_get());
+	s64 delay;
+
+	if (!data)
+		return;
+
+	delay = ktime_us_delta(data->next_access_backoff, ktime_get());
 
 	if (delay > 0)
 		fsleep(delay);
@@ -193,8 +198,14 @@ EXPORT_SYMBOL_NS_GPL(pmbus_wait, "PMBUS");
 void pmbus_update_ts(struct i2c_client *client, int op)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
-	const struct pmbus_driver_info *info = data->info;
-	int delay = info->access_delay;
+	const struct pmbus_driver_info *info;
+	int delay;
+
+	if (!data)
+		return;
+
+	info = data->info;
+	delay = info->access_delay;
 
 	if (op & PMBUS_OP_WRITE)
 		delay = max(delay, info->write_delay);
@@ -518,6 +529,85 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
 }
 EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, "PMBUS");
 
+/**
+ * pmbus_read_smbus_i2c_block_data() - Read SMBus/I2C block data
+ * @client:	Handle to slave device
+ * @reg:	Byte interpreted by slave
+ * @data_buf:	Byte array into which data will be read
+ * Return:	Negative errno or number of bytes read
+ *
+ * PMBus internal function to read a SMBus block from a PMBus chip.
+ *
+ * PMBus chips report various properties using SMBus block read operations.
+ * However, not all I2C controllers support this operation.
+ *
+ * Execute SMBus block read if supported. If not supported, but SMBus I2C block
+ * read is supported, use it instead. Note that at most 31 data bytes can be
+ * read from the device if i2c_smbus_read_i2c_block_data() is used to read the
+ * data. This is a SMBUs protocol limit which can not be avoided.
+ *
+ * Return -EOPNOTSUPP if neither I2C_FUNC_SMBUS_READ_BLOCK_DATA nor
+ * I2C_FUNC_SMBUS_READ_I2C_BLOCK is supported.
+ *
+ * Callers must hold pmbus_lock or execute calls from the probe function.
+ */
+int pmbus_read_smbus_i2c_block_data(struct i2c_client *client, u8 reg, char *data_buf)
+{
+	u8 buf[I2C_SMBUS_BLOCK_MAX];
+	int blen, len, ret;
+
+	if (i2c_check_functionality(client->adapter,
+				    I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
+		pmbus_wait(client);
+		ret = i2c_smbus_read_block_data(client, reg, data_buf);
+		pmbus_update_ts(client, 0);
+		return ret;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+		dev_err_once(&client->dev, "I2C adapter does not support I2C_FUNC_SMBUS_READ_I2C_BLOCK\n");
+		return -EOPNOTSUPP;
+	}
+
+	/*
+	 * If the returned data is valid SMBus block data, the first byte
+	 * must be the data length.
+	 *
+	 * i2c_smbus_read_i2c_block_data() may return an error if the chip
+	 * sends NACK before the number of requested bytes is received.
+	 * Handle this by reading the data length first, then reading the
+	 * entire message up to I2C_SMBUS_BLOCK_MAX bytes. This ensures
+	 * that requested number of bytes never exceeds the number of
+	 * bytes sent by the chip.
+	 */
+	pmbus_wait(client);
+	ret = i2c_smbus_read_i2c_block_data(client, reg, 1, buf);
+	pmbus_update_ts(client, 0);
+	if (ret < 0)
+		return ret;
+	if (ret != 1)
+		return -EIO;
+
+	len = buf[0];
+	if (len == 0)
+		return 0;
+	blen = len;
+	if (len >= I2C_SMBUS_BLOCK_MAX)
+		len = I2C_SMBUS_BLOCK_MAX - 1;
+	pmbus_wait(client);
+	ret = i2c_smbus_read_i2c_block_data(client, reg, len + 1, buf);
+	pmbus_update_ts(client, 0);
+	if (ret < 0)
+		return ret;
+	if (ret != len + 1)
+		return -EIO;
+	if (buf[0] != blen)
+		return -EIO;
+	memcpy(data_buf, buf + 1, len);
+	return len;
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_read_smbus_i2c_block_data, "PMBUS");
+
 static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
 				 char *data_buf)
 {
@@ -527,11 +617,7 @@ static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
 	if (rv < 0)
 		return rv;
 
-	pmbus_wait(client);
-	rv = i2c_smbus_read_block_data(client, reg, data_buf);
-	pmbus_update_ts(client, 0);
-
-	return rv;
+	return pmbus_read_smbus_i2c_block_data(client, reg, data_buf);
 }
 
 static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
-- 
2.45.2

[PATCH 2/5] hwmon: (pmbus/adm1275) Use pmbus_read_smbus_i2c_block_data for block commands

[Guenter Roeck]

The driver uses i2c_smbus_read_block_data() for PMBus block commands.
Use pmbus_read_smbus_i2c_block_data() instead to enable the driver to work
with I2C controllers which do not support i2c_smbus_read_block_data().

Drop the functionality check to get an explicit error message if the
required SMBus commands are not supported.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/adm1275.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index 174f9bcba7e2..beb32a878d1c 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -553,12 +553,7 @@ static int adm1275_probe(struct i2c_client *client)
 	u32 shunt;
 	u32 avg;
 
-	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_READ_BYTE_DATA
-				     | I2C_FUNC_SMBUS_BLOCK_DATA))
-		return -ENODEV;
-
-	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, block_buffer);
+	ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_MFR_ID, block_buffer);
 	if (ret < 0) {
 		dev_err(&client->dev, "Failed to read Manufacturer ID\n");
 		return ret;
@@ -570,7 +565,7 @@ static int adm1275_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
-	ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, block_buffer);
+	ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_MFR_MODEL, block_buffer);
 	if (ret < 0) {
 		dev_err(&client->dev, "Failed to read Manufacturer Model\n");
 		return ret;
-- 
2.45.2

[PATCH 3/5] hwmon: (pmbus/ltc2978) Use pmbus_read_smbus_i2c_block_data for block commands

[Guenter Roeck]

The driver uses a mix of pmbus_read_smbus_i2c_block_data() and
i2c_smbus_read_i2c_block_data() for PMBus block commands.

Use pmbus_read_smbus_i2c_block_data() instead to enable the driver to work
I2C controllers which do not support both block commands.

Drop the functionality check to simplify the code and to trigger a return
with -EOPNOTSUPP if SMBus block commands are not supported.

As part of this patch, rework detection of LT7170 and LT7171. The return
length of pmbus_read_smbus_i2c_block_data() may be less than the requested
number of bytes, meaning the return length needs to be checked. Also,
checking for "LT7170-1" after checking for "LT7170" and checking for
"LT7171-1" after checking for "LT7171" is pointless since the first check
will already produce a match, so drop the second part of the check.

Cc: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/ltc2978.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 10877b0867fd..aea7b6c93e74 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -611,17 +611,13 @@ static int ltc2978_get_id(struct i2c_client *client)
 		u8 buf[I2C_SMBUS_BLOCK_MAX];
 		int ret;
 
-		if (!i2c_check_functionality(client->adapter,
-					     I2C_FUNC_SMBUS_READ_BLOCK_DATA))
-			return -ENODEV;
-
-		ret = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
+		ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_MFR_ID, buf);
 		if (ret < 0)
 			return ret;
 		if (ret < 3 || (strncmp(buf, "LTC", 3) && strncmp(buf, "ADI", 3)))
 			return -ENODEV;
 
-		ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
+		ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_MFR_MODEL, buf);
 		if (ret < 0)
 			return ret;
 		for (id = &ltc2978_id[0]; strlen(id->name); id++) {
@@ -637,16 +633,17 @@ static int ltc2978_get_id(struct i2c_client *client)
 		u8 buf[I2C_SMBUS_BLOCK_MAX];
 		int ret;
 
-		ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
-						    sizeof(buf), buf);
+		ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
+						      buf);
 		if (ret < 0)
 			return ret;
 
-		if (!strncmp(buf + 1, "LT7170", 6) ||
-		    !strncmp(buf + 1, "LT7170-1", 8))
+		if (ret < 6)
+			return -ENODEV;
+
+		if (!strncmp(buf, "LT7170", 6))
 			return lt7170;
-		if (!strncmp(buf + 1, "LT7171", 6) ||
-		    !strncmp(buf + 1, "LT7171-1", 8))
+		if (!strncmp(buf, "LT7171", 6))
 			return lt7171;
 
 		return -ENODEV;
-- 
2.45.2

[PATCH 4/5] hwmon: (pmbus/max20830) Use pmbus_read_smbus_i2c_block_data for block commands

[Guenter Roeck]

The driver tries to use both pmbus_read_smbus_i2c_block_data() and
i2c_smbus_read_i2c_block_data() for PMBus block commands.

Use pmbus_read_smbus_i2c_block_data() instead to simplify the code.

Drop the functionality check to simplify the code and to trigger a return
with -EOPNOTSUPP if SMBus block commands are not supported.

Cc: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/max20830.c | 34 ++++------------------------------
 1 file changed, 4 insertions(+), 30 deletions(-)

diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
index 689fc6f0fe01..4d25a084af09 100644
--- a/drivers/hwmon/pmbus/max20830.c
+++ b/drivers/hwmon/pmbus/max20830.c
@@ -101,36 +101,10 @@ static int max20830_probe(struct i2c_client *client)
 	device_property_read_u32(&client->dev, "adi,vout-rfb1-ohms", &data->vout_rfb1);
 	device_property_read_u32(&client->dev, "adi,vout-rfb2-ohms", &data->vout_rfb2);
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA) &&
-	    !i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK))
-		return -ENODEV;
-
-	/*
-	 * Use i2c_smbus_read_block_data() if supported, otherwise fall back
-	 * to i2c_smbus_read_i2c_block_data() to support I2C controllers
-	 * which do not support SMBus block reads.
-	 */
-	if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
-		ret = i2c_smbus_read_block_data(client, PMBUS_IC_DEVICE_ID, buf);
-		if (ret < 0)
-			return dev_err_probe(&client->dev, ret,
-					     "Failed to read IC_DEVICE_ID\n");
-	} else {
-		/* Reads 1 length byte + data bytes */
-		ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
-						    MAX20830_IC_DEVICE_ID_LENGTH + 1,
-						    buf);
-		if (ret < 0)
-			return dev_err_probe(&client->dev, ret,
-					     "Failed to read IC_DEVICE_ID\n");
-		/*
-		 * Moves data forward, removing the length byte, this is to
-		 * match the format of i2c_smbus_read_block_data().
-		 * Also adjust return value to reflect length byte removal.
-		 */
-		memmove(buf, buf + 1, MAX20830_IC_DEVICE_ID_LENGTH);
-		ret = ret - 1;
-	}
+	ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_IC_DEVICE_ID, buf);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to read IC_DEVICE_ID\n");
 
 	/* Verify we read the expected number of bytes */
 	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
-- 
2.45.2

[PATCH 5/5] hwmon: (pmbus/ir36021) Use pmbus_read_smbus_i2c_block_data for block commands

[Guenter Roeck]

The driver uses i2c_smbus_read_i2c_block_data() for PMBus block commands.
Use pmbus_read_smbus_i2c_block_data() instead to enable the driver to work
with I2C controllers which do not support i2c_smbus_read_i2c_block_data().

Drop the functionality check to get an explicit error message if the
required SMBus commands are not supported.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/ir36021.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/ir36021.c b/drivers/hwmon/pmbus/ir36021.c
index 0dce4c3f666f..58d8c4aac0d1 100644
--- a/drivers/hwmon/pmbus/ir36021.c
+++ b/drivers/hwmon/pmbus/ir36021.c
@@ -31,21 +31,13 @@ static int ir36021_probe(struct i2c_client *client)
 	u8 buf[I2C_SMBUS_BLOCK_MAX];
 	int ret;
 
-	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_READ_BYTE_DATA
-				     | I2C_FUNC_SMBUS_READ_WORD_DATA
-				     | I2C_FUNC_SMBUS_READ_BLOCK_DATA))
-		return -ENODEV;
-
-	ret = i2c_smbus_read_i2c_block_data(client, PMBUS_MFR_MODEL, 2, buf);
-	if (ret < 0) {
-		dev_err(&client->dev, "Failed to read PMBUS_MFR_MODEL\n");
-		return ret;
-	}
-	if (ret != 2 || buf[0] != 0x01 || buf[1] != 0x2d) {
-		dev_err(&client->dev, "MFR_MODEL unrecognised\n");
-		return -ENODEV;
-	}
+	ret = pmbus_read_smbus_i2c_block_data(client, PMBUS_MFR_MODEL, buf);
+	if (ret < 0)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to read PMBUS_MFR_MODEL\n");
+	if (ret != 1 || buf[0] != 0x2d)
+		return dev_err_probe(&client->dev, -ENODEV,
+				     "MFR_MODEL unrecognised\n");
 
 	return pmbus_do_probe(client, &ir36021_info);
 }
-- 
2.45.2

Re: [PATCH 5/5] hwmon: (pmbus/ir36021) Use pmbus_read_smbus_i2c_block_data for block commands

[sashiko-bot]

> The driver uses i2c_smbus_read_i2c_block_data() for PMBus block commands.
> Use pmbus_read_smbus_i2c_block_data() instead to enable the driver to work
> with I2C controllers which do not support i2c_smbus_read_i2c_block_data().
> 
> Drop the functionality check to get an explicit error message if the
> required SMBus commands are not supported.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803193535.2286578-1-linux@roeck-us.net?part=5

Re: [PATCH 4/5] hwmon: (pmbus/max20830) Use pmbus_read_smbus_i2c_block_data for block commands

[sashiko-bot]

> The driver tries to use both pmbus_read_smbus_i2c_block_data() and
> i2c_smbus_read_i2c_block_data() for PMBus block commands.
> 
> Use pmbus_read_smbus_i2c_block_data() instead to simplify the code.
> 
> Drop the functionality check to simplify the code and to trigger a return
> with -EOPNOTSUPP if SMBus block commands are not supported.
> 
> Cc: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> Cc: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803193535.2286578-1-linux@roeck-us.net?part=4

Re: [PATCH 1/5] hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The hwmon device is exposed to userspace before the driver probe is fully complete, causing a race condition on I2C page state.
--

--- Patch [1]: [PATCH 1/5] hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data() ---
commit 80dbae6dc3a3ada2ae6512549c713faf3d025f76
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()

PMBus drivers need support for reading SMBus block data. Unfortunately,
not all i2C controllers support this command.

Implement pmbus_read_smbus_i2c_block_data() which first tries to use
i2c_smbus_read_block_data(). If not supported, try to emulate it by calling
i2c_smbus_read_i2c_block_data(). Export the new function for use in PMBus
drivers.

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ba3e9e9fc8bcc..df2409c89a840 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition
during probe in pmbus_do_probe().

When devm_hwmon_device_register_with_groups() is called, the hwmon device
is exposed to userspace immediately:

drivers/hwmon/pmbus/pmbus_core.c:pmbus_do_probe() {
    ...
	data->hwmon_dev = devm_hwmon_device_register_with_groups(dev, name,
								 data, data->groups);
	if (IS_ERR(data->hwmon_dev)) {
		dev_err(dev, "Failed to register hwmon device\n");
		return PTR_ERR(data->hwmon_dev);
	}

	ret = pmbus_regulator_register(data);
	if (ret)
		return ret;

	ret = pmbus_irq_setup(client, data);
	if (ret)
		return ret;

	pmbus_init_debugfs(client, data);
    ...
}

Userspace (like udev) can immediately issue sysfs reads or writes, which hold
pmbus_lock and call pmbus_set_page().

Concurrently, the probe function continues execution and calls
pmbus_irq_setup() and pmbus_init_debugfs().

Both of these functions execute unlocked I2C operations that also call
pmbus_set_page(). Because pmbus_set_page() does not internally hold a lock,
the hardware I2C page state can become desynchronized from the software
data->currpage state.

Could this result in I2C operations being sent to the wrong page?

Since PMBus devices are often power supplies, if a sysfs write is directed to
the wrong page due to a race, could it change the voltage or limits on the
wrong power rail and potentially cause hardware damage?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803193535.2286578-1-linux@roeck-us.net?part=1

Re: [PATCH 2/5] hwmon: (pmbus/adm1275) Use pmbus_read_smbus_i2c_block_data for block commands

[sashiko-bot]

> The driver uses i2c_smbus_read_block_data() for PMBus block commands.
> Use pmbus_read_smbus_i2c_block_data() instead to enable the driver to work
> with I2C controllers which do not support i2c_smbus_read_block_data().
> 
> Drop the functionality check to get an explicit error message if the
> required SMBus commands are not supported.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803193535.2286578-1-linux@roeck-us.net?part=2

Re: [PATCH 3/5] hwmon: (pmbus/ltc2978) Use pmbus_read_smbus_i2c_block_data for block commands

[sashiko-bot]

> The driver uses a mix of pmbus_read_smbus_i2c_block_data() and
> i2c_smbus_read_i2c_block_data() for PMBus block commands.
> 
> Use pmbus_read_smbus_i2c_block_data() instead to enable the driver to work
> I2C controllers which do not support both block commands.
> 
> Drop the functionality check to simplify the code and to trigger a return
> with -EOPNOTSUPP if SMBus block commands are not supported.
> 
> As part of this patch, rework detection of LT7170 and LT7171. The return
> length of pmbus_read_smbus_i2c_block_data() may be less than the requested
> number of bytes, meaning the return length needs to be checked. Also,
> checking for "LT7170-1" after checking for "LT7170" and checking for
> "LT7171-1" after checking for "LT7171" is pointless since the first check
> will already produce a match, so drop the second part of the check.
> 
> Cc: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> Cc: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803193535.2286578-1-linux@roeck-us.net?part=3

Re: [PATCH 1/5] hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()

[Nuno Sá]

On Mon, Aug 03, 2026 at 12:35:31PM -0700, Guenter Roeck wrote:
> PMBus drivers need support for reading SMBus block data. Unfortunately,
> not all i2C controllers support this command.
> 
> Implement pmbus_read_smbus_i2c_block_data() which first tries to use
> i2c_smbus_read_block_data(). If not supported, try to emulate it by calling
> i2c_smbus_read_i2c_block_data(). Export the new function for use in PMBus
> drivers.
> 
> Cc: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
> Cc: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/pmbus/pmbus.h      |   1 +
>  drivers/hwmon/pmbus/pmbus_core.c | 102 ++++++++++++++++++++++++++++---
>  2 files changed, 95 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 5fe2c415eada..2cd3216b3cd9 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -561,6 +561,7 @@ void pmbus_set_update(struct i2c_client *client, u8 reg, bool update);
>  void pmbus_wait(struct i2c_client *client);
>  void pmbus_update_ts(struct i2c_client *client, int op);
>  int pmbus_set_page(struct i2c_client *client, int page, int phase);
> +int pmbus_read_smbus_i2c_block_data(struct i2c_client *client, u8 reg, char *data_buf);
>  int pmbus_read_word_data(struct i2c_client *client, int page, int phase,
>  			 u8 reg);

...

> +int pmbus_read_smbus_i2c_block_data(struct i2c_client *client, u8 reg, char *data_buf)
> +{
> +	u8 buf[I2C_SMBUS_BLOCK_MAX];
> +	int blen, len, ret;
> +
> +	if (i2c_check_functionality(client->adapter,
> +				    I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> +		pmbus_wait(client);
> +		ret = i2c_smbus_read_block_data(client, reg, data_buf);
> +		pmbus_update_ts(client, 0);
> +		return ret;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +		dev_err_once(&client->dev, "I2C adapter does not support I2C_FUNC_SMBUS_READ_I2C_BLOCK\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/*
> +	 * If the returned data is valid SMBus block data, the first byte
> +	 * must be the data length.
> +	 *
> +	 * i2c_smbus_read_i2c_block_data() may return an error if the chip
> +	 * sends NACK before the number of requested bytes is received.
> +	 * Handle this by reading the data length first, then reading the
> +	 * entire message up to I2C_SMBUS_BLOCK_MAX bytes. This ensures
> +	 * that requested number of bytes never exceeds the number of
> +	 * bytes sent by the chip.
> +	 */
> +	pmbus_wait(client);
> +	ret = i2c_smbus_read_i2c_block_data(client, reg, 1, buf);
> +	pmbus_update_ts(client, 0);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EIO;
> +

I think the above is ret != -1 is not really necessary (same for the
below check. i2c_smbus_read_i2c_block_data() always returns the length passed to it.
The only case where it doesn't is if we pass something >
I2C_SMBUS_BLOCK_MAX which would always fail anyways.

- Nuno Sá

> +	len = buf[0];
> +	if (len == 0)
> +		return 0;
> +	blen = len;
> +	if (len >= I2C_SMBUS_BLOCK_MAX)
> +		len = I2C_SMBUS_BLOCK_MAX - 1;
> +	pmbus_wait(client);
> +	ret = i2c_smbus_read_i2c_block_data(client, reg, len + 1, buf);
> +	pmbus_update_ts(client, 0);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != len + 1)
> +		return -EIO;
> +	if (buf[0] != blen)
> +		return -EIO;
> +	memcpy(data_buf, buf + 1, len);
> +	return len;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_read_smbus_i2c_block_data, "PMBUS");
> +
>  static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
>  				 char *data_buf)
>  {
> @@ -527,11 +617,7 @@ static int pmbus_read_block_data(struct i2c_client *client, int page, u8 reg,
>  	if (rv < 0)
>  		return rv;
>  
> -	pmbus_wait(client);
> -	rv = i2c_smbus_read_block_data(client, reg, data_buf);
> -	pmbus_update_ts(client, 0);
> -
> -	return rv;
> +	return pmbus_read_smbus_i2c_block_data(client, reg, data_buf);
>  }
>  
>  static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
> -- 
> 2.45.2
> 

Re: [PATCH 0/5] hwmon: (pmbus/core) Introduce and use pmbus_read_smbus_i2c_block_data()

[Nuno Sá]

On Mon, Aug 03, 2026 at 12:35:30PM -0700, Guenter Roeck wrote:
> PMBus drivers need support for reading SMBus block data. Unfortunately,
> not all i2C controllers support this command.
> 
> Implement pmbus_read_smbus_i2c_block_data() which first tries to use
> i2c_smbus_read_block_data(). If not supported, try to emulate it by calling
> i2c_smbus_read_i2c_block_data(). Export the new function for use in PMBus
> drivers, and use it in affected drivers.
> 
> ----------------------------------------------------------------
> Guenter Roeck (5):
>       hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()
>       hwmon: (pmbus/adm1275) Use pmbus_read_smbus_i2c_block_data for block commands
>       hwmon: (pmbus/ltc2978) Use pmbus_read_smbus_i2c_block_data for block commands
>       hwmon: (pmbus/max20830) Use pmbus_read_smbus_i2c_block_data for block commands
>       hwmon: (pmbus/ir36021) Use pmbus_read_smbus_i2c_block_data for block commands
> 
>  drivers/hwmon/pmbus/adm1275.c    |   9 +---
>  drivers/hwmon/pmbus/ir36021.c    |  22 +++------
>  drivers/hwmon/pmbus/ltc2978.c    |  21 ++++----
>  drivers/hwmon/pmbus/max20830.c   |  34 ++-----------
>  drivers/hwmon/pmbus/pmbus.h      |   1 +
>  drivers/hwmon/pmbus/pmbus_core.c | 102 ++++++++++++++++++++++++++++++++++++---
>  6 files changed, 117 insertions(+), 72 deletions(-)

Just one minor comment from me. With it:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

Re: [PATCH 1/5] hwmon: (pmbus/core) Introduce pmbus_read_smbus_i2c_block_data()

[Guenter Roeck]

On 8/4/26 01:45, Nuno Sá wrote:
...
>> +	/*
>> +	 * If the returned data is valid SMBus block data, the first byte
>> +	 * must be the data length.
>> +	 *
>> +	 * i2c_smbus_read_i2c_block_data() may return an error if the chip
>> +	 * sends NACK before the number of requested bytes is received.
>> +	 * Handle this by reading the data length first, then reading the
>> +	 * entire message up to I2C_SMBUS_BLOCK_MAX bytes. This ensures
>> +	 * that requested number of bytes never exceeds the number of
>> +	 * bytes sent by the chip.
>> +	 */
>> +	pmbus_wait(client);
>> +	ret = i2c_smbus_read_i2c_block_data(client, reg, 1, buf);
>> +	pmbus_update_ts(client, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (ret != 1)
>> +		return -EIO;
>> +
> 
> I think the above is ret != -1 is not really necessary (same for the
> below check. i2c_smbus_read_i2c_block_data() always returns the length passed to it.
> The only case where it doesn't is if we pass something >
> I2C_SMBUS_BLOCK_MAX which would always fail anyways.
> 

Good point. I'll drop the checks.

Thanks,
Guenter