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

Jared,

On 8/20/26 06:23, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?
> 
Is that a valid concern ?

Thanks,
Guenter