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

On Wed, Jul 29, 2026 at 04:18:23PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> [snip]
> 
> [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);
>     ...
> }

The state check is added in patch 4/4. I think I split the commits too
much though; I'll squash these in v2.

> [snip]
> 
> [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);
>     ...
> }

Same as in [1], this is an explicit design choice documented in
Documentation/hwmon/ina2xx.rst.

[1]: https://lore.kernel.org/all/aoYYxM63yFRgrxFD@rh-jkangas-kernel/

> 
> [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.

These are addressed in e6c80061ca23 ("hwmon: (ina2xx) Fix various
overflow issues").

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