Re: [PATCH 3/4] hwmon: (ina2xx) Track active alarm in ina2xx_data

[sashiko-bot@kernel.org]

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

New issues:
- [High] Writing 0 to an inactive alert limit unintentionally clears the hardware mask, disabling the unrelated, currently active alert limit.
- [Medium] The `ina226_alert_read` function fails to use the new `active_alert` disambiguation state, causing unconfigured alarm attributes to incorrectly return 1 if another alarm triggers.

Pre-existing issues:
- [High] On 32-bit architectures, setting any positive power limit results in a limit of 0 due to an integer underflow when interacting with `clamp_val()`.
- [High] Setting large shunt voltage limits causes an integer overflow on 32-bit architectures, resulting in a limit of 0 instead of the maximum clamped limit.
--

--- Patch [3]: [PATCH 3/4] hwmon: (ina2xx) Track active alarm in ina2xx_data ---
commit 480990f993b0efa725eee2c5b0c2a864cd60bcfb
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Track active alarm in ina2xx_data

INA2XX current limits are converted into shunt voltage limits internally
using the shunt resistor value. Once a current limit's corresponding
voltage limit is written to the hardware, shunt voltage and current
alarms are indistinguishable from each other.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -157,6 +157,7 @@ struct ina2xx_data {
>  	const struct ina2xx_config *config;
>  	enum ina2xx_ids chip;
>  
> +	enum ina2xx_alert_type active_alert;

[Severity: Medium]
Will this new state also be checked in ina226_alert_read()?

If an unconfigured alarm triggers, could it incorrectly return 1 since both
alarms map to the same hardware mask?

drivers/hwmon/ina2xx.c:ina226_alert_read() {
    ...
    mask = ina2xx_alert_type_to_mask(alert);
    *val = (regval & mask) && (regval & INA226_ALERT_FUNCTION_FLAG);
    ...
}

[ ... ]

> @@ -492,9 +522,13 @@ static int ina226_alert_limit_write(struct ina2xx_data *data, enum ina2xx_alert_
>  
>  	if (val) {
>  		mask = ina2xx_alert_type_to_mask(alert);
> -		return regmap_update_bits(regmap, INA226_MASK_ENABLE,
> -					  INA226_ALERT_CONFIG_MASK, mask);
> +		ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
> +					 INA226_ALERT_CONFIG_MASK, mask);
> +		if (ret < 0)
> +			return ret;
>  	}
> +
> +	data->active_alert = val ? alert : INA2XX_ALERT_NONE;

[Severity: High]
If a user writes 0 to an inactive alert limit, will it unintentionally clear
the hardware mask at the beginning of ina226_alert_limit_write() and wipe out
the currently active alert?

drivers/hwmon/ina2xx.c:ina226_alert_limit_write() {
    ...
    ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
                 INA226_ALERT_CONFIG_MASK, 0);
    ...
}

[Severity: High]
This is a pre-existing issue, but on 32-bit architectures, does this clamp
operation compute a negative boundary for INA2XX_POWER?

drivers/hwmon/ina2xx.c:ina226_alert_to_reg() {
    ...
    case INA2XX_POWER:
        val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
    ...
}

Because UINT_MAX is cast to signed long by clamp_val(), it could result in a
negative upper bound, forcing val to a negative number which is then clamped
to 0.

[Severity: High]
This is also a pre-existing issue, but does setting a large shunt voltage
limit cause an integer overflow on 32-bit architectures?

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;
    ...
}

If a user sets a large valid limit on a 32-bit system, the multiplication
might overflow the signed 32-bit long into a negative number, which is then
incorrectly clamped to 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-upstream-ina2xx-in0-curr1-alarms-v1-0-349f7b2f1df8@redhat.com?part=3