Re: [PATCH 4/4] hwmon: (ina2xx) Return 0 on inactive alarm/limit reads

On Wed, Jul 29, 2026 at 04:28:36PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> [snip]
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but since we are fixing the
> read path for inactive alerts here, what happens on the write path?
> 
> If a user or monitoring daemon reads an inactive alert attribute (which
> now correctly returns 0) and subsequently writes 0 back to it, will
> ina226_alert_limit_write() unconditionally wipe the active alert?
> 
> Looking at ina226_alert_limit_write():
> 
> 	ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
> 				 INA226_ALERT_CONFIG_MASK, 0);
> 	...
> 	data->active_alert = val ? alert : INA2XX_ALERT_NONE;
> 
> Does this inadvertently destroy the configuration of the currently active
> alert of a different type without checking if the disable operation applies
> to it?

As with [1] and [2], this clearing is documented in
Documentation/hwmon/ina2xx.rst.

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

> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential overflow when
> calculating power values.
> 
> In ina2xx_get_value() for INA2XX_POWER:
> 
> 	case INA2XX_POWER:
> 		val = regval * data->power_lsb_uW;
> 		break;
> 
> [snip]
> 
> [Severity: High]
> This is a pre-existing issue, but could we run into an overflow on 32-bit
> architectures when setting shunt voltage limits?
> 
> In ina226_alert_to_reg():
> 
> 	case INA2XX_SHUNT_VOLTAGE:
> 		val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
> 		val *= data->config->shunt_div;
> 
> [snip]
> 
> [Severity: High]
> This is a pre-existing issue, but setting power limits on 32-bit systems
> seems to always evaluate to 0.
> 
> In ina226_alert_to_reg():
> 
> 	case INA2XX_POWER:
> 		val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);

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

> [snip]
> 
> [Severity: Medium]
> This is a pre-existing issue, but there appears to be a locking violation
> when reading the shunt resistor.
> 
> The hwmon subsystem guidelines require drivers to use hwmon_lock() for
> attributes registered outside the core info parameter. 
> 
> Looking at shunt_resistor_show():
> 
> static ssize_t shunt_resistor_show(struct device *dev,
> 				   struct device_attribute *da, char *buf)
> {
> 	struct ina2xx_data *data = dev_get_drvdata(dev);
> 
> 	return sysfs_emit(buf, "%li\n", data->rshunt);
> }
> 
> Since shunt_resistor_store() modifies data->rshunt, does this show
> function need to acquire the hwmon_lock to prevent unprotected concurrent
> accesses?

This seems accurate. It's not very related to the series but an easy
fix, so I'll put it in v2 since it was reported here.

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