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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Brittle validation logic due to missing read length check for PMBUS_MFR_MODEL.
- [Low] Copy-paste errors in sysfs documentation for `in2_*` attributes.
- [Low] MAINTAINERS file entry breaks alphabetical ordering.
--

commit ec60b1738fafe62da18eefb7e9b1f9a12b6a9113
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/d1u74t) Add Murata D1U74T PSU driver

This commit adds a new PMBus driver for Murata D1U74T power supplies. It
registers the device with the hwmon subsystem and provides documentation for
the driver's sysfs attributes.

Closes: https://lore.kernel.org/oe-kbuild-all/202605122253.zInzmUeX-lkp@intel.com/

> diff --git a/Documentation/hwmon/d1u74t.rst b/Documentation/hwmon/d1u74t.rst
> new file mode 100644
> index 0000000000000..7362e56e76a71
> --- /dev/null
> +++ b/Documentation/hwmon/d1u74t.rst
[ ... ]
> +in1_rated_min		Minimum rated input voltage
> +in1_rated_max		Maximum rated input voltage
> +
> +in2_label		"vout1"
> +in2_input		Measured input voltage

This isn't a bug, but is there a copy-paste error in these descriptions?
Should these be documented as 'output voltage' instead of 'input voltage',
since the label is "vout1"?

> +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
[ ... ]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2040011a3865..28ca8e0353124 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6803,6 +6803,13 @@ S:	Maintained
>  F:	Documentation/hwmon/crps.rst
>  F:	drivers/hwmon/pmbus/crps.c
>  
> +MURATA D1U74T PSU DRIVER
> +M:	Abdurrahman Hussain <abdurrahman@nexthop.ai>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/d1u74t.rst
> +F:	drivers/hwmon/pmbus/d1u74t.c
> +
>  CRYPTO API

This isn't a bug, but does this entry break the alphabetical ordering? It
places an 'M' entry immediately before the 'C' section for CRYPTO API.

>  M:	Herbert Xu <herbert@gondor.apana.org.au>
[ ... ]
> 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
[ ... ]
> +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;
> +	}
> +
> +	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)) {

Is there a missing check for the read length in rc here?

If a short read occurs, could strncmp() evaluate the newly read bytes
combined with stale memory leftover from the previous PMBUS_MFR_ID read?

> +		buf[rc] = '\0';
> +		return dev_err_probe(dev, -ENODEV, "Model '%s' not supported\n",
> +				     buf);
> +	}
> +
> +	rc = pmbus_do_probe(client, &d1u74t_info);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-d1u74t-v2-0-431d00fbb1c4@nexthop.ai?part=2