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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Medium] The driver incorrectly uses `i2c_smbus_read_i2c_block_data` instead of `i2c_smbus_read_block_data`, violating the protocol and logging an untrusted binary buffer.
- [Low] The MODULE_IMPORT_NS macro uses an unquoted identifier, resulting in a build failure.
- [Low] The sysfs attribute `in2_alarm` is created but not documented.
--

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

hwmon: (pmbus/max20830) add driver for max20830

This commit adds support for the Analog Devices MAX20830 step-down DC-DC
switching regulator. It provides a PMBus client driver to monitor input and
output voltages, output current, and temperature.

> diff --git a/Documentation/hwmon/max20830.rst b/Documentation/hwmon/max20830.rst
> new file mode 100644
> index 0000000000000..b9dffb7605978
> --- /dev/null
> +++ b/Documentation/hwmon/max20830.rst
[ ... ]
> +================= ========================================
> +in1_label         "vin"
> +in1_input         Measured input voltage
> +in1_alarm         Input voltage alarm
> +in2_label         "vout1"
> +in2_input         Measured output voltage

This isn't a bug, but since the driver configuration enables
PMBUS_HAVE_STATUS_VOUT, it will expose the in2_alarm attribute. Should
in2_alarm be documented here alongside the other alarms?

> +curr1_label       "iout1"
> +curr1_input       Measured output current
> +curr1_alarm       Output current alarm
> +temp1_input       Measured temperature
> +temp1_alarm       Chip temperature alarm
> +================= ========================================

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> new file mode 100644
> index 0000000000000..bcf60fd948e34
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/max20830.c
[ ... ]
> +static int max20830_probe(struct i2c_client *client)
> +{
> +	u8 buf[I2C_SMBUS_BLOCK_MAX + 1];
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> +		return -ENODEV;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
> +					    I2C_SMBUS_BLOCK_MAX, buf);

Should this use i2c_smbus_read_block_data() instead?

PMBus IC_DEVICE_ID is an SMBus Block Read command, which natively returns a
length byte followed by the data. Using the I2C block read API with a
hardcoded length of 32 forces an over-read and captures the raw SMBus length
byte in buf[0].

> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to read IC_DEVICE_ID\n");
> +
> +	buf[ret] = '\0';
> +	if (ret < 9 || strncmp(buf + 1, "MAX20830", 8))
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "Unsupported device: '%s'\n", buf);

If the device is unsupported, will logging the buffer via %s print unprintable
characters?

Since buf[0] contains the raw length byte from the block read, this could
inject control characters and trailing binary padding into the kernel log.

[ ... ]
> +MODULE_AUTHOR("Alexis Czezar Torreno <alexisczezar.torreno@analog.com>");
> +MODULE_DESCRIPTION("PMBus driver for Analog Devices MAX20830");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(PMBUS);

Will this cause a build failure?

The MODULE_IMPORT_NS macro expects a string literal. Should PMBUS be enclosed
in quotes, like MODULE_IMPORT_NS("PMBUS")?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260414-dev_max20830-v1-0-210d3f82c571@analog.com?part=2