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

On 4/13/26 21:56, [email protected] wrote:
> 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.
> --
> 

The reported issues seem real. Please address.

Thanks,
Guenter

> commit 62ed9b8ac37c74adac303a0ebaea8df41fde6a85
> Author: Alexis Czezar Torreno <[email protected]>
> 
> 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 <[email protected]>");
>> +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")?
>