Re: [PATCH v2 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 2 potential issue(s) to consider:
- [High] hwmon: (pmbus_core) divide-by-zero after pmbus_regulator_set_voltage()
- [Medium] hwmon: (pmbus_core) broken relative VOUT margin handling in regulator ops
--

commit 8ccb177073557cf9644c419b791da4a5cf494d3e
Author: Lars Petter Mostad <lars.petter.mostad@appear.net>

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

This patch adds support for PMBus 1.3 relative encoding of output voltage
related commands, scaling limits relative to VOUT_COMMAND in
pmbus_reg2data() and pmbus_data2reg() and updating the cached VOUT_COMMAND
when setting regulator voltage.

> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 5f69c1420b4e1..47dff264d28bf 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c

[ ... ]

> @@ -803,6 +806,26 @@ static void pmbus_update_sensor_data(struct i2c_client *client, struct pmbus_sen
>  						     sensor->phase, sensor->reg);
>  }
>  
> +static bool pmbus_sensor_is_vout_relative(struct pmbus_data *data,
> +					  struct pmbus_sensor *sensor)
> +{
> +	if (sensor->class != PSC_VOLTAGE_OUT ||
> +	    !data->vout_mode_relative[sensor->page])
> +		return false;
> +
> +	switch (sensor->reg) {
> +	case PMBUS_VOUT_MARGIN_HIGH:
> +	case PMBUS_VOUT_MARGIN_LOW:
> +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> +	case PMBUS_VOUT_OV_WARN_LIMIT:
> +	case PMBUS_VOUT_UV_WARN_LIMIT:
> +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

[Severity: Medium]
Can the PMBUS_VOUT_MARGIN_HIGH and PMBUS_VOUT_MARGIN_LOW cases in
pmbus_sensor_is_vout_relative() ever match, and how does
pmbus_regulator_list_voltage() behave in relative mode?

No hwmon sensor attributes are registered for PMBUS_VOUT_MARGIN_LOW or
PMBUS_VOUT_MARGIN_HIGH. The only callers in drivers/hwmon/pmbus/pmbus_core.c
that read these registers and pass them to pmbus_reg2data() are
pmbus_regulator_get_low_margin() and pmbus_regulator_get_high_margin():

static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page)
{
	struct pmbus_data *data = i2c_get_clientdata(client);
	struct pmbus_sensor s = {
		.page = page,
		.class = PSC_VOLTAGE_OUT,
		.convert = true,
		.data = -1,
	};

	if (data->vout_low[page] < 0) {
		if (pmbus_check_word_register(client, page, PMBUS_MFR_VOUT_MIN))
			s.data = _pmbus_read_word_data(client, page, 0xff,
						       PMBUS_MFR_VOUT_MIN);
		if (s.data < 0) {
			s.data = _pmbus_read_word_data(client, page, 0xff,
						       PMBUS_VOUT_MARGIN_LOW);
			if (s.data < 0)
				return s.data;
		}
		data->vout_low[page] = pmbus_reg2data(data, &s);
	}

	return data->vout_low[page];
}

Neither pmbus_regulator_get_low_margin() nor
pmbus_regulator_get_high_margin() initializes s.reg (leaving it as 0), so
when they fall back to PMBUS_VOUT_MARGIN_LOW or PMBUS_VOUT_MARGIN_HIGH,
pmbus_reg2data() -> pmbus_sensor_is_vout_relative() returns false and caches
the unscaled permillage value as mV in data->vout_low[page] and
data->vout_high[page].

In addition, while pmbus_regulator_set_voltage() skips checking these margins
when data->vout_mode_relative[s.page] is true, pmbus_regulator_list_voltage()
still calls pmbus_regulator_get_low_margin() and
pmbus_regulator_get_high_margin() unconditionally:

static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
					unsigned int selector)
{
...
	low = pmbus_regulator_get_low_margin(client, rdev_get_id(rdev));
	if (low < 0)
		return low;

	high = pmbus_regulator_get_high_margin(client, rdev_get_id(rdev));
	if (high < 0)
		return high;

	if (val >= low && val <= high)
		return val * 1000; /* unit is uV */

	return 0;
}

Even if s.reg were populated when reading the margin registers,
data->vout_low[page] and data->vout_high[page] are cached once and not
updated when pmbus_regulator_set_voltage() changes VOUT_COMMAND. Should
pmbus_regulator_get_low_margin(), pmbus_regulator_get_high_margin(), and
pmbus_regulator_list_voltage() also account for relative mode?

[ ... ]

> @@ -3351,29 +3419,38 @@ 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;
>  
>  	*selector = 0;
>  
>  	guard(pmbus_lock)(client);
>  
> -	low = pmbus_regulator_get_low_margin(client, s.page);
> -	if (low < 0)
> -		return low;
> +	if (!data->vout_mode_relative[s.page]) {
> +		low = pmbus_regulator_get_low_margin(client, s.page);
> +		if (low < 0)
> +			return low;
>  
> -	high = pmbus_regulator_get_high_margin(client, s.page);
> -	if (high < 0)
> -		return high;
> +		high = pmbus_regulator_get_high_margin(client, s.page);
> +		if (high < 0)
> +			return high;
>  
> -	/* Make sure we are within margins */
> -	if (low > val)
> -		val = low;
> -	if (high < val)
> -		val = high;
> +		/* Make sure we are within margins */
> +		if (low > val)
> +			val = low;
> +		if (high < val)
> +			val = high;
> +	}
>  
>  	val = pmbus_data2reg(data, &s, val);
>  
> -	return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
> +	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])
> +		pmbus_init_vout_relative(data, s.page, val);
> +
> +	return 0;
>  }

[Severity: High]
Could updating data->vout_command[s.page] in pmbus_regulator_set_voltage()
lead to a division by zero in pmbus_data2reg()?

At probe time, pmbus_init_common() rejects non-positive values in
data->vout_command[page] after calling pmbus_init_vout_relative(). However,
in pmbus_regulator_set_voltage(), when data->vout_mode_relative[s.page] is
true, margin clamping is skipped and pmbus_init_vout_relative(data, s.page,
val) updates data->vout_command[s.page] without checking whether the decoded
voltage is <= 0.

If a consumer requests a small voltage (for example min_uv < 500 uV, or a
value that rounds to 0 in pmbus_data2reg()), data->vout_command[s.page]
becomes 0. A subsequent write to a relative VOUT limit attribute in sysfs
will then call pmbus_set_sensor() -> pmbus_data2reg() and divide by zero:

	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]);
	}

Should pmbus_regulator_set_voltage() or pmbus_data2reg() guard against a
non-positive vout_command?

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