Re: [PATCH 2/3] hwmon: ina2xx: support ina232

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The INA232 configuration implicitly omits `.has_update_interval = true`, silently disabling sysfs update interval configuration.

Pre-existing issues:
- [High] Multiple alert limit calculations in `ina226_alert_to_reg` suffer from integer overflows and signedness mismatches on 32-bit systems, causing hardware limits to be incorrectly programmed.
- [High] Integer overflow in power calculations inside `ina2xx_get_value()` incorrectly returns negative power measurements at higher loads.
--

commit 553d30269962c93d687fffb0c6e1dcc15cbdf020
Author: Martino Facchin <m.facchin@arduino.cc>
Subject: hwmon: ina2xx: support ina232

This commit adds support for the INA232 current and power monitor. It defines
the INA232 configuration values and adds the device to the corresponding i2c
and device tree identification tables.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 613ffb622b7c4..122e7aa4fdfff 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c

[ ... ]

> @@ -196,6 +197,17 @@ static const struct ina2xx_config ina2xx_config[] = {
>  		.current_shift = 4,
>  		.has_update_interval = true,
>  	},
> +	[ina232] = {
> +		.config_default = INA226_CONFIG_DEFAULT,
> +		.calibration_value = 2048,
> +		.shunt_div = 400,
> +		.bus_voltage_shift = 0,
> +		.bus_voltage_lsb = 1600,
> +		.power_lsb_factor = 32,

[Severity: High]
This is a pre-existing issue, but can the power calculation overflow when
using larger power_lsb_factor values like the one added here for INA232?

In ina2xx_get_value(), the INA2XX_POWER calculation is:

    val = regval * data->power_lsb_uW;

The result is returned as a signed 32-bit int. For the INA232 with a common
2mOhm shunt, power_lsb_uW becomes 40,000. When the 16-bit regval exceeds
53,687, the product exceeds INT_MAX. This overflows the 32-bit signed math,
wrapping to a negative integer and reporting erroneous negative power
readings to userspace.

[Severity: High]
This is also a pre-existing issue, but do the alert limit calculations in
ina226_alert_to_reg() cause hardware limits to be incorrectly programmed on
32-bit systems?

For INA2XX_POWER, the upper bound is clamped:

    val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);

clamp_val() implicitly casts the upper bound to a 32-bit signed long on
32-bit platforms. This results in a negative number, meaning any valid
power limit is clamped to a negative value and ultimately programmed as 0.

Similarly, for INA2XX_SHUNT_VOLTAGE, the initial clamp uses SHRT_MAX *
shunt_div instead of division. The subsequent multiplication:

    val *= data->config->shunt_div;

overflows LONG_MAX on 32-bit platforms for high inputs, wrapping to a
negative value and also programming the hardware limit to 0.

Additionally, for INA2XX_BUS_VOLTAGE on parts with bus_voltage_shift > 0,
the calculation (val * 1000) << shift can exceed LONG_MAX for limits over
~134V, wrapping to negative and setting the limit to 0.

> +		.has_alerts = true,
> +		.has_ishunt = false,
> +		.has_power_average = false,
> +	},

[Severity: Medium]
Does this configuration intentionally omit the update interval?

Because has_update_interval is not explicitly set, it defaults to false.
This hides the hwmon_chip_update_interval sysfs attribute, preventing
adjustments to the averaging time despite the INA232 sharing the INA226
register layout.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-monza-ina232-v1-0-925b0d12771b@oss.qualcomm.com?part=2