Re: [PATCH v6 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Incorrect scaling, missing boundary clamping, and unsigned wrap-around for TPS1689 PMBUS_VIN_OV_FAULT_LIMIT cause severely corrupted limits and bitfield overflows.
- [High] The shared 8-bit warning limit write paths treat PMBus direct values as unsigned, breaking all warning limits for TPS1689.
- [Medium] Missing scale adaptation for PSC_CURRENT_OUT in tps25990_probe leads to output current readings being off by a factor of 1000.
- [Low] The intermediate expression in the read path for PMBUS_VIN_OV_FAULT_LIMIT unconditionally underflows in unsigned arithmetic, violating subsystem guidelines.

Pre-existing issues:
- [High] Writing a small voltage/current limit below the hardware offset for TPS25990 causes an unsigned integer underflow, wrapping around to configure the hardware with its maximum (weakest) protection limit.
--

--- Patch [3]: [PATCH v6 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support ---
commit a29f74ffb1bc30041022deb0ab63b9dd79c1a779
Author: Stoyan Bogdanov <sbogdanov@baylibre.com>

hwmon: (pmbus/tps25990): Add TPS1689 support

Extend the existing TPS25990 driver to support the TPS1689 eFuse,
as both devices share the same command interface and functionality.

Update the documentation to include TPS1689 support.

> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 7634ac743025d..a91ea8f33b296 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
[ ... ]
> @@ -193,9 +202,18 @@ static int tps25990_read_word_data(struct i2c_client *client,
>  		ret = pmbus_read_word_data(client, page, phase, reg);
>  		if (ret < 0)
>  			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> -					TPS25990_VIN_OVF_DIV);
> -		ret += TPS25990_VIN_OVF_OFF;
> +		if (data->chip_id == tps25990) {
> +			ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> +						TPS25990_VIN_OVF_DIV);
> +			ret += TPS25990_VIN_OVF_OFF;
> +		} else if (data->chip_id == tps1689) {
> +			ret = DIV_ROUND_CLOSEST(
> +				((FIELD_GET(TPS1689_VIN_OV_RANGE_SEL_MASK, ret) + 1) *
> +				TPS1689_VIN_RANGE_SPAN_MV) +
> +				(FIELD_GET(TPS1689_VIN_VOV_MASK, ret) *
> +				TPS1689_VIN_VOV_STEP_MV - TPS1689_VIN_RANGE_SPAN_MV),

[Severity: Low]
Does this intermediate expression underflow in unsigned arithmetic?

The maximum VOV is 63, so 63 * 250 = 15750, making the subtraction
unconditionally negative. Due to the unsigned evaluation of FIELD_GET(), this
underflows to a large positive integer before wrapping back correctly when
added. Can this be simplified to RNG * 16000 + VOV * 250 to avoid the
underflow?

> +				TPS1689_VIN_SCALING);
> +		}
>  		break;
[ ... ]
> @@ -238,6 +256,8 @@ static int tps25990_read_word_data(struct i2c_client *client,
>  static int tps25990_write_word_data(struct i2c_client *client,
>  				    int page, int reg, u16 value)
>  {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
>  	int ret;
>  
>  	switch (reg) {

[Severity: High]
Does this newly added device support require updates to the warning limit
logic?

For TPS1689, standard voltages map to negative PMBus direct format values
(e.g., -4). In tps25990_write_word_data(), the shared 8-bit warning limits
perform a logical right shift:

	case PMBUS_VIN_UV_WARN_LIMIT:
        ...
	case PMBUS_PIN_OP_WARN_LIMIT:
		value >>= TPS25990_8B_SHIFT;
		value = clamp_val(value, 0, 0xff);

Since value is an unsigned u16, will the right shift convert negative PMBus
values to large positive numbers, causing standard voltage warning limits to
be misconfigured as the maximum possible value?

> @@ -255,10 +275,23 @@ static int tps25990_write_word_data(struct i2c_client *client,
>  		break;
>  
>  	case PMBUS_VIN_OV_FAULT_LIMIT:
> -		value -= TPS25990_VIN_OVF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> -					  TPS25990_VIN_OVF_NUM);
> -		value = clamp_val(value, 0, 0xf);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_VIN_OVF_OFF;

[Severity: High]
This isn't a bug introduced by this patch, but does writing a small limit
below the hardware offset cause an unsigned integer underflow?

If a user writes a limit that converts to a PMBus value smaller than
TPS25990_VIN_OVF_OFF (155) or TPS25990_IIN_OCF_OFF (157), value will
underflow to a huge number. Since clamp_val() then clamps it to the absolute
maximum limit index, will this completely defeat the hardware protection
during an over-voltage or over-current event?

> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> +						  TPS25990_VIN_OVF_NUM);
> +			value = clamp_val(value, 0, 0xf);
> +		} else if (data->chip_id == tps1689) {
> +			u32 scaled_value = value * TPS1689_VIN_SCALING + TPS1689_VIN_RANGE_SPAN_MV;
> +			u32 tmp_scaled_value = scaled_value;
> +			u8 ov_rng_sel = 0;
> +			u32 ov_set = 0;
> +
> +			ov_rng_sel = tmp_scaled_value / TPS1689_VIN_RANGE_SPAN_MV;
> +			ov_set = tmp_scaled_value - (TPS1689_VIN_RANGE_SPAN_MV * ov_rng_sel);
> +			value = FIELD_PREP(TPS1689_VIN_OV_RANGE_SEL_MASK, ov_rng_sel - 1) |

[Severity: High]
Does this calculation incorrectly scale the limit and overflow the bitfield?

If a PMBus direct format value of -4 (65532 as unsigned u16) is passed in,
scaling it by 251 generates an out-of-bounds ov_rng_sel. Since there is no
clamping before FIELD_PREP(), will this silently truncate the value and
configure the hardware with dangerously low over-voltage limits?

> +						FIELD_PREP(TPS1689_VIN_VOV_MASK,
> +						(ov_set / TPS1689_VIN_VOV_STEP_MV));
> +		}
>  		ret = pmbus_write_word_data(client, page, reg, value);
>  		break;
[ ... ]
> @@ -347,6 +380,60 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>  #endif
>  
>  static const struct pmbus_driver_info tps25990_base_info[] = {
> +	[tps1689] = {
> +		.pages = 1,
[ ... ]
> +		.format[PSC_CURRENT_IN] = direct,
> +		.m[PSC_CURRENT_IN] = 9548,
> +		.b[PSC_CURRENT_IN] = 0,
> +		.R[PSC_CURRENT_IN] = -6,
> +		.format[PSC_CURRENT_OUT] = direct,
> +		.m[PSC_CURRENT_OUT] = 24347,
> +		.b[PSC_CURRENT_OUT] = 0,
> +		.R[PSC_CURRENT_OUT] = -3,

[Severity: Medium]
Does tps25990_probe() need to be updated to adapt the scale for
PSC_CURRENT_OUT?

While PSC_CURRENT_IN is dynamically scaled based on the rimon resistor value
in tps25990_probe():

	tps25990_set_m(&data->info.m[PSC_CURRENT_IN], rimon);
	tps25990_set_m(&data->info.m[PSC_POWER], rimon);

PSC_CURRENT_OUT is omitted. Since the default rimon scaling multiplies m by
1000, will skipping this adaptation cause the reported output current telemetry
to be off by a factor of 1000?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805090400.2059006-1-sbogdanov@baylibre.com?part=3