Re: [PATCH v3 2/2] hwmon: (pmbus/max20830) add driver for max20830

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Unconditional requirement of I2C block read breaks support for standard SMBus controllers.
- [High] Incorrect parsing of SMBus block read ByteCount leads to probe failure or short-read acceptance.
- [Low] Pointer sign mismatch warning in strncmp().
--

commit e81b5e8d851a24ad6bc004153df89f39f490846a
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: (pmbus/max20830) add driver for max20830
    
Adds support for the MAX20830 step-down DC-DC switching regulator with
PMBus interface. It allows monitoring of input/output voltage, output
current and temperature through the PMBus serial interface.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> new file mode 100644
> index 0000000000000..b1c6985067d7c
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/max20830.c

[ ... ]

> +static int max20830_probe(struct i2c_client *client)
> +{
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1] = {};
> +	u8 len;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> +		return -ENODEV;

Does unconditionally requiring I2C_FUNC_SMBUS_READ_I2C_BLOCK break
compatibility with standard PC/server SMBus controllers (like Intel i801 or
AMD piix4)?

These controllers support I2C_FUNC_SMBUS_READ_BLOCK_DATA but may not support
arbitrary I2C block reads. Since PMBus is an SMBus-based protocol, should
the code prefer standard i2c_smbus_read_block_data() and only fall back to
I2C block reads if native SMBus block read functionality is missing?

> +
> +	/*
> +	 * Use i2c_smbus_read_i2c_block_data() instead of
> +	 * i2c_smbus_read_block_data() to support I2C controllers
> +	 * which do not support SMBus block reads.
> +	 */
> +	ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
> +					    I2C_SMBUS_BLOCK_MAX, buf);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to read IC_DEVICE_ID\n");
> +
> +	/* First byte is the block length (including itself). */
> +	len = buf[0];
> +	if (len != 9 || ret < len)

Is this byte count check and length calculation correct? The SMBus
specification states that the ByteCount specifies the number of data bytes
and does not include itself.

If a spec-compliant device returns a ByteCount of 8 for the 8-character
string MAX20830, won't the len != 9 check incorrectly abort with -ENODEV?

Additionally, does ret < len correctly validate the read size? If ret is 9
(1 length byte + 8 data bytes), the check ret < len (9 < 9) evaluates to
false, passing the check. However, a ByteCount of 9 means 9 data bytes were
intended (10 total bytes).

Should this validation check be ret < len + 1 instead to avoid silently
overwriting a missing 9th data byte?

> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "IC_DEVICE_ID length mismatch: reported %u, read %d\n",
> +				     len, ret);
> +
> +	/* Data is at buf[1..8], so null terminator goes at buf[9]. */
> +	buf[len] = '\0';
> +	if (strncmp(buf + 1, "MAX20830", 8))

This isn't a bug, but buf is defined as a u8 array and strncmp() expects a
const char pointer. Does this trigger a -Wpointer-sign compiler warning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260417-dev_max20830-v3-0-0cb8d56067aa@analog.com?part=2