Re: [PATCH v2 4/4] hwmon: (ina2xx) Decouple in0 and curr1 alarms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Writing 0 to an inactive alert limit unintentionally clears the currently active alert, breaking the newly introduced alarm decoupling.
--

--- Patch [4]: [PATCH v2 4/4] hwmon: (ina2xx) Decouple in0 and curr1 alarms ---
commit 0eea1308d1961d2cb0471626855b002f45901d56
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Decouple in0 and curr1 alarms

This commit attempts to decouple shunt voltage and current limit alarms by
tracking the active alert type and returning zero during reads if the active
alarm is for a different type.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
[ ... ]
> @@ -484,6 +521,7 @@ static int ina226_alert_limit_write(struct ina2xx_data *data, enum ina2xx_alert_
>  				 INA226_ALERT_CONFIG_MASK, 0);
>  	if (ret < 0)
>  		return ret;
> +	data->active_alert = INA2XX_ALERT_NONE;
>  
>  	ret = regmap_write(regmap, INA226_ALERT_LIMIT,
>  			   ina226_alert_to_reg(data, reg, val));
> @@ -492,9 +530,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 = alert;
>  	}
> +
>  	return 0;
>  }

[Severity: High]
Will this unintentionally disable active alerts if a 0 is written to an
inactive alert?

Because the read side was updated in this patch to return 0 for inactive
alerts, state restoration tools might now read 0 for an inactive alert and
subsequently write 0 back to it during initialization.

When ina226_alert_limit_write() is called with val == 0 for that inactive
alert, the code just before this hunk unconditionally clears the hardware
mask and the newly added line sets data->active_alert = INA2XX_ALERT_NONE.

Since val is 0, the if (val) block is bypassed, leaving all alerts
disabled. This seems to allow writes to an inactive alert to silently
disable the currently active alert.

Does this function need an early return or guard to ignore writes of 0
if the alert being written to is already inactive?

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