Re: [PATCH 2/2] hwmon: (pmbus/d1u74t) Add Murata D1U74T PSU driver

[Guenter Roeck <linux@roeck-us.net>]

Hi,

On 5/12/26 20:15, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] The driver fails to verify the number of bytes read (`rc`) from `PMBUS_MFR_MODEL` before calling `strncmp(buf, "D1U74T-W", 8)`.
> - [Low] Copy-paste errors in `Documentation/hwmon/d1u74t.rst` for `in2_*` attributes.
> - [Low] Inconsistent error handling styles for failure modes returning `-ENODEV` in the probe function.
> --
> 
> commit a76e770177dc6ee5472b638d5e12f032fd0a5cd5
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> 
> hwmon: (pmbus/d1u74t) Add Murata D1U74T PSU driver
> 
> This commit introduces a new PMBus driver for Murata D1U74T power supplies.
> It relies on the core PMBus framework and checks the manufacturer ID and
> model string during probe.
> 
>> diff --git a/Documentation/hwmon/d1u74t.rst b/Documentation/hwmon/d1u74t.rst
>> new file mode 100644
>> index 0000000000000..f7786ebba0b47
>> --- /dev/null
>> +++ b/Documentation/hwmon/d1u74t.rst
>> @@ -0,0 +1,97 @@
> [ ... ]
>> +in2_label		"vout1"
>> +in2_input		Measured input voltage
>> +in2_crit		Critical input over voltage
>> +in2_crit_alarm		Critical input over voltage alarm
>> +in2_lcrit		Critical input under voltage fault
>> +in2_lcrit_alarm		Critical input under voltage fault alarm
>> +in2_max			Maximum input over voltage
>> +in2_max_alarm		Maximum input over voltage alarm
>> +in2_min			Minimum input under voltage warning
>> +in2_min_alarm		Minimum input under voltage warning alarm
>> +in2_rated_min		Minimum rated input voltage
>> +in2_rated_max		Maximum rated input voltage
> 
> This isn't a bug, but should these descriptions refer to output voltage
> instead of input voltage, since the label is vout1?
> 
> [ ... ]
>> diff --git a/drivers/hwmon/pmbus/d1u74t.c b/drivers/hwmon/pmbus/d1u74t.c
>> new file mode 100644
>> index 0000000000000..3127e0e5a23de
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/d1u74t.c
>> @@ -0,0 +1,85 @@
> [ ... ]
>> +static int d1u74t_probe(struct i2c_client *client)
>> +{
>> +	char buf[I2C_SMBUS_BLOCK_MAX + 2] = { 0 };
>> +	struct device *dev = &client->dev;
>> +	int rc;
>> +
>> +	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_ID, buf);
>> +	if (rc < 0)
>> +		return dev_err_probe(dev, rc, "Failed to read PMBUS_MFR_ID\n");
>> +
>> +	if (rc != 9 || strncmp(buf, "Murata-PS", 9)) {
>> +		buf[rc] = '\0';
>> +		dev_err(dev, "Unsupported Manufacturer ID '%s'\n", buf);
>> +		return -ENODEV;
>> +	}
> 
> This isn't a bug, but could dev_err_probe() be used here for consistency
> with the other error paths in this function?
> 
>> +
>> +	rc = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, buf);
>> +	if (rc < 0)
>> +		return dev_err_probe(dev, rc,
>> +				     "Failed to read PMBUS_MFR_MODEL\n");
>> +
>> +	if (strncmp(buf, "D1U74T-W", 8)) {
>> +		buf[rc] = '\0';
>> +		return dev_err_probe(dev, -ENODEV, "Model '%s' not supported\n",
>> +				     buf);
>> +	}
> 
> Does this code evaluate stale data in the buffer if the model read returns
> fewer than 8 bytes?
> 
> The buf array isn't cleared between the manufacturer and model reads. If
> i2c_smbus_read_block_data() successfully returns a length smaller than 8,
> strncmp() could end up comparing against leftover characters from the
> previous Murata-PS string since rc isn't verified before the comparison.
> 
I think those are all valid issues. Please fix.

Thanks,
Guenter