Re: [PATCH v4 2/4] hwmon: (pmbus/tps25990): Rework TPS25990 direct conversion handling

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The custom conversion functions incorrectly retrieve I2C client data, leading to severe type confusion and out-of-bounds heap reads.
- [High] Replacing ratio-based math with direct conversion helpers inside `read_word_data` and `write_word_data` introduces a double-conversion bug that completely corrupts sysfs output and configuration limits.
- [High] Calling `pmbus_reg2data_direct_calc()` directly with negative `R` coefficients and small register values leads to a total loss of precision due to premature integer division.
- [High] The patch silently drops the correct translation ratios for the Over-Current Fault Limit (`PMBUS_IIN_OC_FAULT_LIMIT`) and bypasses dynamic resistor scaling.
- [Low] Spelling mistakes in the newly introduced `tps25990_parameters` enum comments.

Pre-existing issues:
- [High] When setting fault limits, an integer underflow on low user inputs leads to an unsigned wrap-around, causing `clamp_val()` to clamp to the maximum limit instead of the minimum limit, dangerously defeating hardware protection.
--

commit 11131ea034ce953233470df60e4f6a1c34cca609
Author: Stoyan Bogdanov <sbogdanov@baylibre.com>
Date:   Fri May 22 08:23:37 2026 +0000

    hwmon: (pmbus/tps25990): Rework TPS25990 direct conversion handling
    
    Rework the existing implementation of direct format conversion for
    TPS25990 non-standard parameters to improve code reusability and
    integration with the PMBus direct conversion helpers.

> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 05c6288ecafcc..1e252844217b8 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c

[ ... ]

> +enum tps25990_parameters {
> +	TPS25990_VIN_OVF = 0, /* VIN over volatage fault */
> +	TPS25990_IIN_OCF, /* IIN Over currect fault */

[Severity: Low]
This isn't a bug, but there are minor typos in the comments: volatage and
currect.

> +	TPS25990_DIRECT_VALUES_COUNT,
> +};

[ ... ]

> +static s64 tps25990_reg2data_direct(struct i2c_client *client, int param, s32 raw)
> +{
> +	struct pmbus_driver_info *info = i2c_get_clientdata(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);

[Severity: Critical]
Does this correctly retrieve the driver info?

Looking at pmbus_do_probe(), i2c_get_clientdata(client) is set to a
struct pmbus_data * object, not a struct pmbus_driver_info * object.

Using container_of() on the wrong type calculates a 0 offset, but improperly
casts the pmbus_data object into a struct tps25990_data pointer, causing the
subsequent read of info_local to fetch completely out-of-bounds heap memory.

Should this use pmbus_get_driver_info(client) instead?

> +	struct tps25990_local_direct_value *info_local = &data->info_local;
> +	s64 b, val;
> +	s32 m, R;
> +
> +	val = (s16)raw;
> +	m = info_local->m[param];
> +	b = info_local->b[param];
> +	R = info_local->R[param];
> +
> +	return pmbus_reg2data_direct_calc(val, b, m, R);

[Severity: High]
Can this result in a total loss of precision due to integer division?

For TPS25990_VIN_OVF, R is -4 and the raw register value is a small 4-bit
integer (0 to 15). The generic pmbus_reg2data_direct_calc() handles negative
R by dividing the input by 10 for each R:

pmbus_core.c:pmbus_reg2data_direct_calc() {
    ...
    while (R < 0) {
        val = div_s64(val + 5LL, 10L);
        R++;
    }
    ...
}

Dividing a small value like 15 by 10000 via integer division would truncate
it to 0, permanently destroying the underlying data. Does this need
pre-scaling before the calculation loop?

[ ... ]

> @@ -184,9 +230,7 @@ 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;
> +		ret = tps25990_reg2data_direct(client, TPS25990_VIN_OVF, ret);

[Severity: High]
Does this introduce a double-conversion bug?

The driver's .read_word_data hook is expected to convert between the
chip-specific raw register value and the standard PMBus raw format (Y_std).
The PMBus core then converts Y_std to physical units.

By calling pmbus_reg2data_direct_calc() directly here, does it return the
physical value (e.g. milliVolts) rather than Y_std, causing the PMBus core
to apply its standard conversion a second time to an already converted value?

[ ... ]

> @@ -246,17 +288,13 @@ 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 = tps25990_data2reg_direct(client, TPS25990_VIN_OVF, value);
>  		value = clamp_val(value, 0, 0xf);

[Severity: High]
This is a pre-existing issue, but does this lead to an integer underflow
that defeats hardware protection?

The value parameter in tps25990_write_word_data() is an unsigned u16. If a
sufficiently low limit (like 0) is converted, the mathematical result could
be negative. This would wrap around the unsigned u16 to a large value
(e.g., 65530).

Since clamp_val() compares using the type of the passed variable, it would see
the wrapped value as much greater than 15 and incorrectly clamp it to the
maximum hardware limit (0xf) instead of the minimum limit.

>  		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 = tps25990_data2reg_direct(client, TPS25990_IIN_OCF, value);

[Severity: High]
Are we losing the dynamic resistor scaling here?

The original code mapped TPS25990_VIREF to the standard Y_std format,
allowing the PMBus core to dynamically convert it using the PSC_CURRENT_IN
coefficients, which are adjusted in tps25990_probe() based on the
ti,rimon-micro-ohms property.

The new tps25990_data2reg_direct() call uses static coefficients from
tps25990_local_info which are never updated by the rimon logic. Will this
break the over-current limits for boards using a non-default rimon
configuration?

>  		value = clamp_val(value, 0, 0x3f);
>  		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
>  		break;

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