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

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

On 5/14/26 20:34, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] Missing includes for <linux/module.h> and <linux/string.h>.
> - [Low] Logging raw I2C device strings without escaping.
> --
> 
> commit f645f72f2f4d8cc53bec169fb4350e6231e649a4
> Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
> 
> hwmon: (pmbus/d1u74t) Add Murata D1U74T PSU driver
> 
> This commit adds a PMBus client driver for the Murata D1U74T power supplies.
> 
> Closes: https://lore.kernel.org/oe-kbuild-all/202605122253.zInzmUeX-lkp@intel.com/
> 
>> diff --git a/drivers/hwmon/pmbus/d1u74t.c b/drivers/hwmon/pmbus/d1u74t.c
>> new file mode 100644
>> index 0000000000000..286ba492e3362
>> --- /dev/null
>> +++ b/drivers/hwmon/pmbus/d1u74t.c
>> @@ -0,0 +1,86 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright 2026 Nexthop Systems.
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/of.h>
>> +#include <linux/pmbus.h>
>> +
>> +#include "pmbus.h"
> 
> This isn't a bug, but should this file include <linux/module.h> and
> <linux/string.h>? It uses module-specific macros like MODULE_DEVICE_TABLE
> and MODULE_AUTHOR, as well as the strncmp function. While compilation might
> succeed due to transitive includes, it could lead to build failures or
> implicit declaration warnings on some architectures.
> 

I added that; no need to resend.

> [ ... ]
> 
>> +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';
>> +		return dev_err_probe(dev, -ENODEV,
>> +				     "Unsupported Manufacturer ID '%s'\n",
>> +				     buf);
>> +	}
>> +
>> +	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 (rc < 8 || strncmp(buf, "D1U74T-W", 8)) {
>> +		buf[rc] = '\0';
>> +		return dev_err_probe(dev, -ENODEV, "Model '%s' not supported\n",
>> +				     buf);
>> +	}
> 
> Does this code log potentially unescaped strings directly from the device?
> 
> Since the I2C bus can return garbage data or unprintable characters from a
> malfunctioning device, using the %s format specifier could result in
> corrupted log output. Would it be safer to use the %*pE specifier to safely
> escape these untrusted strings?
> 
Possibly, but I'll let it go for now. Other drivers do the same. It can be
fixed in a follow-up patch if needed.

Thanks,
Guenter