Re: [PATCH v3 2/2] hwmon: (pmbus/vt7505) Add driver for Analog Devices MAX16545/MAX16550 and Volterra VT7505

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The arithmetic calculation in `vt7505_set_m()` can overflow when converting the 64-bit result to a signed 32-bit integer, resulting in a negative or corrupted PMBus coefficient.
--

--- Patch [2]: [PATCH v3 2/2] hwmon: (pmbus/vt7505) Add driver for Analog Devices MAX16545/MAX16550 and Volterra VT7505 ---
commit 6d968d4ee814c7e869f893c1177921f54e4e55cd
Author: Georgi Vlaev <[email protected]>

hwmon: (pmbus/vt7505) Add driver for Analog Devices MAX16545/MAX16550 and Volterra VT7505

This commit adds a PMBus driver for the MAX16545, MAX16550, and VT7505
controllers. It reports voltage, current, power, and temperature over
PMBus, calculating current and power coefficients based on the
adi,rload-ohms device tree property.

> diff --git a/drivers/hwmon/pmbus/vt7505.c b/drivers/hwmon/pmbus/vt7505.c
> new file mode 100644
> index 0000000000000..c60f849e1f9ea
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/vt7505.c

[ ... ]

> +static void vt7505_set_m(int *m, u32 rload)
> +{
> +	u64 val = (u64)*m * rload;
> +
> +	/* Make sure m fits the s32 type */
> +	*m = DIV_ROUND_CLOSEST_ULL(val, 1000);

[Severity: Medium]
Does this downcast to a signed 32-bit integer safely handle large values of
rload?

If the rload value is sufficiently large, could the result of
DIV_ROUND_CLOSEST_ULL() exceed INT_MAX and silently overflow into a
negative PMBus coefficient?

> +}
> +
> +static int vt7505_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct vt7505_chip_data *chip;
> +	struct pmbus_driver_info *info;
> +	u32 rload;
> +	int ret;

[ ... ]

> +	/*
> +	 * The m coefficient used in the direct-format current and power
> +	 * calculations depends on RLOAD, the external current-report resistor
> +	 * connected between the ILOAD pin and ground. Use the default value if
> +	 * none is specified.
> +	 */
> +	if (of_property_read_u32(dev->of_node, "adi,rload-ohms", &rload))
> +		rload = VT7505_RLOAD_DEFAULT;
> +
> +	if (!rload)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "adi,rload-ohms must be non-zero\n");

[Severity: Medium]
Should there be an upper bound validation for rload here?

Because rload is read directly from the device tree and passed to
vt7505_set_m(), an unexpectedly large unbounded value could trigger the
arithmetic overflow mentioned above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2