Re: [PATCH v2 2/4] hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read()

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

Pre-existing issues:
- [High] Arithmetic overflows and incorrect clamping logic cause alert limits to be wrongly configured to 0 instead of maximum for shunt voltage and power alerts on 32-bit systems.
--

--- Patch [2]: [PATCH v2 2/4] hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read() ---
commit 034555c6bd153d8c094cfb7d04cc7d98d34d3774
Author: Jared Kangas <[email protected]>

hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read()

This commit updates ina226_alert_read() to take a struct ina2xx_data pointer
instead of a struct regmap pointer. This aligns its parameters with
ina226_alert_limit_read() and ina226_alert_limit_write().

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index f6c63ce1ef193..c56e42432673f 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -476,12 +476,12 @@ static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
>  	return 0;
>  }
>  
> -static int ina226_alert_read(struct regmap *regmap, u32 mask, long *val)
> +static int ina226_alert_read(struct ina2xx_data *data, u32 mask, long *val)
>  {
>  	unsigned int regval;
>  	int ret;

[ ... ]

[Severity: High]
This is a pre-existing issue, and was not introduced by the patch under review,
but does ina226_alert_to_reg() have arithmetic overflows and incorrect clamping
logic on 32-bit systems?

When a user or configuration script sets a valid power limit or a large shunt
voltage limit on a 32-bit system, it appears the values might collapse to zero:

drivers/hwmon/ina2xx.c:ina226_alert_to_reg() {
    ...
	case INA2XX_SHUNT_VOLTAGE:
		val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
		val *= data->config->shunt_div;
		val <<= data->config->shunt_voltage_shift;
		return clamp_val(val, 0, SHRT_MAX);
    ...
	case INA2XX_POWER:
		val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
		val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
		return clamp_val(val, 0, USHRT_MAX);
    ...
}

For INA2XX_POWER, could UINT_MAX - data->power_lsb_uW implicitly cast to a
negative long upper bound in the clamp_val macro, forcing all limits to
negative?

And for INA2XX_SHUNT_VOLTAGE, is the clamp mistakenly using multiplication
(SHRT_MAX * data->config->shunt_div) instead of division, allowing large values
that later overflow a signed 32-bit long multiplication?

In both cases, it looks like the final step clamps the negative intermediate
value to 0, which could cause immediate false hardware alerts if the alert pin
is wired to hardware reset logic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820-upstream-ina2xx-in0-curr1-alarms-v2-0-fdce35abc41e@redhat.com?part=2