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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch fails to actually distinguish current and shunt voltage alerts during read operations, contrary to the claims in the commit message.

Pre-existing issues:
- [High] Integer overflow in `ina2xx_get_value()` and `ina226_alert_to_reg()` leading to corrupted power, current, and voltage limits.
- [High] The driver explicitly rejects negative alert limits, preventing the use of hardware alerts for reverse current flow.
--

--- Patch [2]: [PATCH 2/4] hwmon: (ina2xx) Replace masks with enum in alert functions ---
commit e029f17b406174e97d904910649854d150669988
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Replace masks with enum in alert functions
    
Instead of passing an explicit mask to alert/limit functions like
ina226_alert_read(), introduce an enum ina2xx_alert_type that can be
converted to a mask internally. This allows distinguishing current from
shunt voltage, which are both based on the same function in the
hardware.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 911bf32e8f215..beb851d87f787 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
[ ... ]
> +static int ina226_alert_limit_read(struct ina2xx_data *data, enum ina2xx_alert_type alert,
> +				   int reg, long *val)
>  {
>  	struct regmap *regmap = data->regmap;
>  	int regval;
> +	u32 mask;
>  	int ret;
>  
>  	ret = regmap_read(regmap, INA226_MASK_ENABLE, &regval);
>  	if (ret)
>  		return ret;
>  
> +	mask = ina2xx_alert_type_to_mask(alert);
>  	if (regval & mask) {

[Severity: Medium]
Does this actually distinguish current from shunt voltage alerts as stated
in the commit message?

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.

>  		ret = regmap_read(regmap, INA226_ALERT_LIMIT, &regval);
>  		if (ret)
[ ... ]
> -static int ina226_alert_limit_write(struct ina2xx_data *data, u32 mask, int reg, long val)
> +static int ina226_alert_limit_write(struct ina2xx_data *data, enum ina2xx_alert_type alert,
> +				    int reg, long val)
>  {
>  	struct regmap *regmap = data->regmap;
> +	u32 mask;
>  	int ret;
>  
>  	if (val < 0)
> @@ -450,9 +490,11 @@ static int ina226_alert_limit_write(struct ina2xx_data *data, u32 mask, int reg,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (val)
> +	if (val) {
> +		mask = ina2xx_alert_type_to_mask(alert);
>  		return regmap_update_bits(regmap, INA226_MASK_ENABLE,
>  					  INA226_ALERT_CONFIG_MASK, mask);
> +	}

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

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

>  	return 0;
>  }
>  
[ ... ]
> @@ -510,15 +554,15 @@ static int ina2xx_in_read(struct device *dev, u32 attr, int channel, long *val)
>  		*val = ina2xx_get_value(data, voltage_reg, regval);
>  		break;

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

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