Re: [PATCH v4 4/4] hwmon: (pmbus/tps25990): Add TPS1689 support

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The output current (`PSC_CURRENT_OUT`) parameters for TPS1689 are configured, but the feature is rendered inactive due to a missing capability flag, and its mantissa is not dynamically scaled by the sense resistor.

Pre-existing issues:
- [Critical] The driver incorrectly uses `i2c_get_clientdata()` to retrieve the `pmbus_driver_info` pointer in `tps25990_reg2data_direct` and `tps25990_data2reg_direct`, resulting in a type confusion and out-of-bounds read.
- [High] The mantissa for the local `TPS25990_IIN_OCF` parameter is not dynamically scaled by the `rimon` resistor value, leading to incorrect over-current fault limits.
--

commit 7807fad0cb3cb2d485b85493ccab6a9ca7c6bdde
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.

> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 1e252844217b8..720e134f18925 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
[ ... ]
> @@ -286,7 +287,6 @@ 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_data2reg_direct(client, TPS25990_VIN_OVF, value);

[Severity: Critical]
This is a pre-existing issue, but does tps25990_data2reg_direct() retrieve
the correct pointer type here?

Looking at tps25990_data2reg_direct(), it uses i2c_get_clientdata() to
get struct pmbus_driver_info *:

static u16 tps25990_data2reg_direct(struct i2c_client *client, int param, s64 val)
{
	struct pmbus_driver_info *info = i2c_get_clientdata(client);
	struct tps25990_data *data = container_of(info, struct tps25990_data, info);

Since pmbus_do_probe() sets the client data to its internal
struct pmbus_data *, wouldn't this cast read past the end of
struct pmbus_data into arbitrary heap memory when accessing data->info_local?

Should this use pmbus_get_driver_info(client) to retrieve the correct
pointer instead?

>  		value = clamp_val(value, 0, 0xf);
>  		ret = pmbus_write_word_data(client, page, reg, value);
[ ... ]
> @@ -376,6 +376,14 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>  #endif
>  
>  static struct tps25990_local_direct_value tps25990_local_info[] = {
> +	[tps1689] = {
> +		.m[TPS25990_VIN_OVF] = 3984,
> +		.b[TPS25990_VIN_OVF] = -63750,
> +		.R[TPS25990_VIN_OVF] = -3,
> +		.m[TPS25990_IIN_OCF] = 7111,

[Severity: High]
This is a pre-existing issue, but is the mantissa for TPS25990_IIN_OCF
scaled correctly for custom sense resistors?

Because PMBUS_IIN_OC_FAULT_LIMIT corresponds to a physical current passing
through the sense resistor, shouldn't the mantissa be dynamically scaled by
the hardware rimon resistor value in tps25990_probe(), similar to
PSC_CURRENT_IN?

Without scaling, will writing to PMBUS_IIN_OC_FAULT_LIMIT configure the
over-current protection based on the unscaled default resistor value?

> +		.b[TPS25990_IIN_OCF] = -2133,
> +		.R[TPS25990_IIN_OCF] = -2,
> +	},
>  	[tps25990] = {
[ ... ]
> @@ -387,6 +395,59 @@ 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,
> +		.format[PSC_POWER] = direct,
> +		.m[PSC_POWER] = 2775,
> +		.b[PSC_POWER] = 0,
> +		.R[PSC_POWER] = -4,
> +		.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]
Are we missing the PMBUS_HAVE_IOUT capability flag here?

The format parameters for PSC_CURRENT_OUT are configured above, but
without the capability flag, isn't the feature inaccessible?

Additionally, does tps25990_probe() need to be updated to dynamically scale
the PSC_CURRENT_OUT mantissa with the rimon sense resistor, just like it
does for PSC_CURRENT_IN and PSC_POWER?

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