Re: [PATCH 2/4] hwmon: (ina2xx) Replace masks with enum in alert functions

On Wed, Jul 29, 2026 at 04:02:50PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> [snip]
> 
> [Severity: Medium]
> Does this actually distinguish current from shunt voltage alerts as stated
> in the commit message?

No, this patch is intended to do some replumbing to make the fix
cleaner. I could see how the patch phrasing could come off the wrong way
though. I'll reword this in v2.

> 
> By converting the enum back to the shared hardware mask here and in
> ina226_alert_read(), and not tracking which alert was actually configured,
> reading current limits or alarms via sysfs can return the value or status of
> a configured shunt voltage limit (or vice versa), yielding incorrectly scaled
> hardware measurements.
> 
> [snip]
> 
> [Severity: High]
> This is a pre-existing issue, but does ina226_alert_limit_write() reject
> valid configurations for reverse current flow?
> 
> The function explicitly returns -EINVAL for negative limits:
> 
>     if (val < 0)
>         return -EINVAL;
> 
> This prevents configuring hardware alerts for reverse current or negative
> shunt voltage, such as during battery discharge.
> 

Shunt/bus voltages are clamped to non-negative values in the driver, so
this seems like more of a design choice than a high-severity issue.

> Additionally, if the limit is exactly zero, the alert is not enabled here,
> which makes it impossible to trigger an alert at exactly zero.

Disabling on zero is an explicit driver choice, per
Documentation/hwmon/ina2xx.rst:

    Writing 0 to any `*crit` setting clears all `*crit` settings and
    alarms.

> 
> [snip]
> 
> [Severity: High]
> This is a pre-existing issue, but can the calculations in ina2xx_get_value()
> and ina226_alert_to_reg() overflow?
> 
> For large limit values, such as a 200V bus voltage limit, the shift operation
> in ina226_alert_to_reg() on 32-bit architectures:
> 
>     val = (val * 1000) << data->config->bus_voltage_shift;
> 
> yields 3.2 billion, which overflows a signed 32-bit long variable. This
> results in a negative value that gets clamped to 0.
> 
> Similarly, in ina2xx_get_value() called above:
> 
>     val = regval * data->power_lsb_uW;
> 
> the power calculation can overflow a 32-bit signed int for large regval and
> high power_lsb_uW, returning negative power readings.

This is 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=2