Re: [PATCH v3 3/4] hwmon: (pmbus_core) Support relative encoding of vout related commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: (pmbus_core) truncation and s64 overflow in vout_command scaling
--

commit 05161e0e5e17d81565feffc1dff16ea378c5e2e9
Author: Lars Petter Mostad <lars.petter.mostad@appear.net>

hwmon: (pmbus_core) Support relative encoding of vout related commands

This commit adds support for PMBus 1.3 relative encoding of output voltage
limit and margin commands, scaling values relative to VOUT_COMMAND when the
relative bit in VOUT_MODE is set.

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 5f69c1420b4e1..ab91438b0c261 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -101,6 +101,9 @@ struct pmbus_data {
>  
>  	int exponent[PMBUS_PAGES];
>  				/* linear mode: exponent for output voltages */
> +	bool vout_mode_relative[PMBUS_PAGES];
> +				/* VOUT limits relative to VOUT_COMMAND */
> +	int vout_command[PMBUS_PAGES];	/* VOUT_COMMAND in mV, relative mode */
>  
>  	const struct pmbus_driver_info *info;
>  

[ ... ]

> @@ -1005,6 +1028,12 @@ static s64 pmbus_reg2data(struct pmbus_data *data, struct pmbus_sensor *sensor)
>  		val = pmbus_reg2data_linear(data, sensor);
>  		break;
>  	}
> +
> +	/* In relative mode, val is a permillage of VOUT_COMMAND */
> +	if (pmbus_sensor_is_vout_relative(data, sensor))
> +		val = DIV_S64_ROUND_CLOSEST(val * data->vout_command[sensor->page],
> +					    1000);
> +
>  	return val;
>  }
>  
> @@ -1209,6 +1238,12 @@ static u16 pmbus_data2reg(struct pmbus_data *data,
>  	if (!sensor->convert)
>  		return val;
>  
> +	if (pmbus_sensor_is_vout_relative(data, sensor)) {
> +		val = clamp_val(val, -INT_MAX, INT_MAX);
> +		val = DIV_S64_ROUND_CLOSEST(val * 1000,
> +					    data->vout_command[sensor->page]);
> +	}
> +
>  	switch (data->info->format[sensor->class]) {
>  	case direct:
>  		regval = pmbus_data2reg_direct(data, sensor, val);
> @@ -2736,6 +2771,21 @@ static int pmbus_init_coefficients(struct i2c_client *client,
>  	return 0;
>  }
>  
> +/* Decode a raw VOUT_COMMAND value to mV */
> +static s64 pmbus_vout_command_mv(struct pmbus_data *data, int page,
> +				 int vout_command_raw)
> +{
> +	struct pmbus_sensor s = {
> +		.page = page,
> +		.reg = PMBUS_VOUT_COMMAND,
> +		.class = PSC_VOLTAGE_OUT,
> +		.convert = true,
> +		.data = vout_command_raw,
> +	};
> +
> +	return pmbus_reg2data(data, &s);
> +}
> +

[ ... ]

> @@ -2979,6 +3033,22 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>  			return ret;
>  	}
>  
> +	for (page = 0; page < info->pages; page++) {
> +		if (data->vout_mode_relative[page]) {
> +			ret = _pmbus_read_word_data(client, page, 0xff,
> +						    PMBUS_VOUT_COMMAND);
> +			if (ret < 0)
> +				return ret;
> +			data->vout_command[page] =
> +				pmbus_vout_command_mv(data, page, ret);
> +			if (data->vout_command[page] <= 0) {
> +				dev_err(dev, "Invalid VOUT_COMMAND 0x%04x for relative VOUT mode\n",
> +					ret);
> +				return -ENODEV;
> +			}
> +		}
> +	}
> +

[ ... ]

> @@ -3351,7 +3441,8 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
>  		.data = -1,
>  	};
>  	int val = DIV_ROUND_CLOSEST(min_uv, 1000); /* convert to mV */
> -	int low, high;
> +	int low, high, ret;
> +	s64 vout_mv = 0;
>  
>  	*selector = 0;
>  
> @@ -3373,7 +3464,21 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
>  
>  	val = pmbus_data2reg(data, &s, val);
>  
> -	return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
> +	/* Relative limits are scaled by VOUT_COMMAND, which must stay positive */
> +	if (data->vout_mode_relative[s.page]) {
> +		vout_mv = pmbus_vout_command_mv(data, s.page, val);
> +		if (vout_mv <= 0)
> +			return -EINVAL;
> +	}
> +
> +	ret = _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (data->vout_mode_relative[s.page])
> +		data->vout_command[s.page] = vout_mv;
> +
> +	return 0;
>  }

[Severity: Medium]
Can storing the s64 result of pmbus_vout_command_mv() into the 32-bit int
data->vout_command[] truncate the voltage and cause division by zero or
signed 64-bit multiplication overflow?

In pmbus_vout_command_mv() -> pmbus_reg2data(), linear mode with a positive
exponent (up to 15) or direct mode with a negative R can produce an s64
value exceeding INT_MAX.

In pmbus_init_common(), assigning that s64 return value to int
data->vout_command[page] truncates the upper 32 bits before checking
if (data->vout_command[page] <= 0), allowing values whose lower 32 bits are
positive to pass with a wrapped value.

In pmbus_regulator_set_voltage(), the check:

	vout_mv = pmbus_vout_command_mv(data, s.page, val);
	if (vout_mv <= 0)
		return -EINVAL;

validates the 64-bit vout_mv before truncating it into int
data->vout_command[s.page]. If the lower 32 bits of vout_mv are zero or
negative, data->vout_command[s.page] becomes zero or negative, which can
trigger a division by zero in pmbus_data2reg() on subsequent writes to
relative VOUT limit registers:

	val = DIV_S64_ROUND_CLOSEST(val * 1000,
				    data->vout_command[sensor->page]);

Additionally, in pmbus_reg2data():

	if (pmbus_sensor_is_vout_relative(data, sensor))
		val = DIV_S64_ROUND_CLOSEST(val * data->vout_command[sensor->page],
					    1000);

val comes unclamped from pmbus_reg2data_linear() or pmbus_reg2data_direct()
(for instance, in linear mode with exponent >= 7), so could multiplying val
by data->vout_command[sensor->page] overflow s64?

Would it make sense to reject vout_mv values above a safe bound (or clamp /
use mul_s64_u64_div_s64) so that data->vout_command[page] cannot truncate
and val * data->vout_command[sensor->page] cannot overflow S64_MAX?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-hwmon-pmbus-fs1412-v3-0-51df629e13d7@nexthop.ai?part=3