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

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

New issues:
- [High] When writing the `PMBUS_IIN_OC_FAULT_LIMIT` for `tps1689`, the 16-bit `value` is passed directly to `pmbus_write_byte_data()` without clamping, causing silent integer truncation and incorrect hardware limits.
- [Medium] The `tps1689` configuration defines format and coefficients for `PSC_CURRENT_OUT`, but misses the required capability flag and scaling logic, leaving it completely unused and broken.

Pre-existing issues:
- [High] When a user requests a voltage or current limit lower than the base hardware offset, the unsigned 16-bit limit underflows, silently configuring the hardware to its absolute maximum limit.
--

--- Patch [3]: [PATCH v5 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support ---
commit 3362ebf6e63c82cf07dc00cc00e78fd1761514ef
Author: Stoyan Bogdanov <[email protected]>

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 490dcd2a5fa51..7e2991a85da13 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c

[ ... ]

> @@ -253,20 +262,23 @@ static int tps25990_write_word_data(struct i2c_client *client,
>  		value = clamp_val(value, 0, 0xff);
>  		ret = pmbus_write_word_data(client, page, reg, value);
>  		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;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> +						  TPS25990_VIN_OVF_NUM);
> +			value = clamp_val(value, 0, 0xf);
> +		}

[Severity: High]
This is a pre-existing issue, but could this calculation underflow if the user
requests a limit lower than the base hardware offset?

If a limit is requested where the value is less than TPS25990_VIN_OVF_OFF (155),
wouldn't the u16 value silently underflow to a very large positive number when
subtracted?

Since this huge value is then scaled and passed to clamp_val() against 0xf,
would this result in setting the absolute maximum possible limit in the hardware
instead of the requested low limit, inadvertently bypassing the protections?

>  		ret = pmbus_write_word_data(client, page, reg, value);
>  		break;
>  
>  	case PMBUS_IIN_OC_FAULT_LIMIT:
> -		value -= TPS25990_IIN_OCF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> -					  TPS25990_IIN_OCF_NUM);
> -		value = clamp_val(value, 0, 0x3f);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_IIN_OCF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> +						  TPS25990_IIN_OCF_NUM);
> +			value = clamp_val(value, 0, 0x3f);
> +		}

[Severity: High]
This is also a pre-existing issue, but does this face the same underflow risk
if the requested limit is less than TPS25990_IIN_OCF_OFF?

>  		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);

[Severity: High]
When data->chip_id == tps1689, this code skips the scaling block and passes
the u16 value directly into pmbus_write_byte_data().

Does this silently truncate the 16-bit value into an 8-bit register write?

If a large limit is requested, could this truncation cause it to wrap around
to a very small limit, potentially causing the eFuse to prematurely cut power?

>  		break;

[ ... ]

> @@ -347,6 +359,59 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>  #endif
>  
>  static struct pmbus_driver_info tps25990_base_info[] = {
> +	[tps1689] = {
> +		.pages = 1,

[ ... ]

> +		.format[PSC_CURRENT_OUT] = direct,
> +		.m[PSC_CURRENT_OUT] = 24347,
> +		.b[PSC_CURRENT_OUT] = 0,
> +		.R[PSC_CURRENT_OUT] = -3,

[ ... ]

> +		.func[0] = (PMBUS_HAVE_VIN |
> +			    PMBUS_HAVE_VOUT |
> +			    PMBUS_HAVE_VMON |
> +			    PMBUS_HAVE_IIN |
> +			    PMBUS_HAVE_PIN |
> +			    PMBUS_HAVE_TEMP |
> +			    PMBUS_HAVE_STATUS_VOUT |
> +			    PMBUS_HAVE_STATUS_IOUT |
> +			    PMBUS_HAVE_STATUS_INPUT |
> +			    PMBUS_HAVE_STATUS_TEMP |
> +			    PMBUS_HAVE_SAMPLES),

[Severity: Medium]
The configuration defines format and coefficients for PSC_CURRENT_OUT, however
the func[0] capability flags do not include PMBUS_HAVE_IOUT.

Does this mean the PMBus core will completely ignore the output current sensor?

Additionally, looking at tps25990_probe(), it appears dynamic scaling for the
rimon resistor is applied for PSC_CURRENT_IN and PSC_POWER, but not
PSC_CURRENT_OUT:

drivers/hwmon/pmbus/tps25990.c:tps25990_probe() {
    ...
	/* Adapt the current and power scale for each instance */
	tps25990_set_m(&data->info.m[PSC_CURRENT_IN], rimon);
	tps25990_set_m(&data->info.m[PSC_POWER], rimon);
    ...
}

Would this reading be incorrect even if the sensor capability were enabled?

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