Re: [PATCH v2 3/9] hwmon: adm1275: Detect coefficient overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces spurious probe failures by restricting the intermediate `shunt * m` multiplication to a 32-bit limit instead of using a 64-bit calculation, needlessly rejecting perfectly valid shunt resistor configurations.
--

commit 02af229d429dc5c77e875edeb815b9caba7e6da2
Author: Matti Vaittinen <mazziesaccount@gmail.com>

hwmon: adm1275: Detect coefficient overflow

This commit detects potential coefficient overflow when a large shunt
resistor is used and aborts the probe if an overflow is detected.

> diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
> index 43baa5ded35e5..ccc3ad21e38e7 100644
> --- a/drivers/hwmon/pmbus/adm1275.c
> +++ b/drivers/hwmon/pmbus/adm1275.c
> @@ -839,15 +839,25 @@ static int adm1275_probe(struct i2c_client *client)
>  		info->R[PSC_VOLTAGE_OUT] = coefficients[voindex].R;
>  	}
>  	if (cindex >= 0) {
> +		u32 m;
> +
>  		/* Scale current with sense resistor value */
> -		info->m[PSC_CURRENT_OUT] =
> -			coefficients[cindex].m * shunt / 1000;
> +		if (unlikely(check_mul_overflow(coefficients[cindex].m, shunt, &m))) {

[Severity: High]
Does this code introduce a spurious probe failure for valid setups?

By checking for overflow on the intermediate product using a 32-bit
unsigned integer, the probe will fail for configurations where the
product exceeds U32_MAX, but the final scaled value after dividing by
1000 would safely fit into the 32-bit signed integer target field.

Could we use a 64-bit intermediate calculation instead, such as
div_s64((s64)coefficients[cindex].m * shunt, 1000), to prevent this
regression and avoid needlessly rejecting valid shunt resistor setups?

> +			dev_err(&client->dev, "Current coefficient overflow\n");
> +			return -EOVERFLOW;
> +		}
> +		info->m[PSC_CURRENT_OUT] = m / 1000;
>  		info->b[PSC_CURRENT_OUT] = coefficients[cindex].b;
>  		info->R[PSC_CURRENT_OUT] = coefficients[cindex].R;
>  	}
>  	if (pindex >= 0) {
> -		info->m[PSC_POWER] =
> -			coefficients[pindex].m * shunt / 1000;
> +		u32 m;
> +
> +		if (unlikely(check_mul_overflow(coefficients[pindex].m, shunt, &m))) {

[Severity: High]
Can this similarly cause the probe to abort prematurely?

For example, configuring an ADM1293 on the 100mV range with a 250000 uOhm
shunt resistor yields a power coefficient product of 26042 * 250000, which
is 6,510,500,000. This mathematically overflows the 32-bit check here, but
the final value of 6,510,500 easily fits in the target field.

Would it be better to calculate this using 64-bit math so the device can
successfully initialize all of its sensors?

> +			dev_err(&client->dev, "Power coefficient overflow\n");
> +			return -EOVERFLOW;
> +		}
> +		info->m[PSC_POWER] = m / 1000;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782458224.git.mazziesaccount@gmail.com?part=3