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

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

[Jared Kangas]

The in0 and curr1 alarms in the ina2xx driver are both based on shunt
voltage in the hardware, which causes unexpected behavior when reading
the two inputs' alarms:

1. Both alarms may read back 1, even though only one is set.

2. The active alarm may read back 0 even if the limit was reached due to
   the use of alert latching. If the inactive alarm is read, it clears
   the active alarm, and the alarm may not be set before the next read:

        # echo 1800 >curr1_lcrit
        # head {in0,curr1}_lcrit_alarm
        ==> in0_lcrit_alarm <==
        1

        ==> curr1_lcrit_alarm <==
        0

To address this, track the active alarm's type in the driver, and when
reading alarms, return early without polling the hardware if the alarm
being read is inactive.

Patch 1 is a locking fix reported by Sashiko in [1], followed by some
prep work in 2-3 to simplify the fix in patch 4.

[1]: https://lore.kernel.org/all/20260729162836.89BDF1F00A3A@smtp.kernel.org/

Signed-off-by: Jared Kangas <jkangas@redhat.com>
---
Changes in v2:
- Squash v1's patches 3/4 and 4/4 into v2's 4/4
- Split active_alert assignment in alert_limit_write in case of
  failures between zeroing old mask and writing new mask.
- Add hwmon_lock in shunt_resistor_show
- Add #include <linux/bitops.h>
- Touch up code comments and commit descriptions
- Link to v1: https://lore.kernel.org/r/20260729-upstream-ina2xx-in0-curr1-alarms-v1-0-349f7b2f1df8@redhat.com

---
Jared Kangas (4):
      hwmon: (ina2xx) Acquire hwmon_lock in shunt_resistor_show()
      hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read()
      hwmon: (ina2xx) Replace masks with enum in alert functions
      hwmon: (ina2xx) Decouple in0 and curr1 alarms

 drivers/hwmon/ina2xx.c | 163 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 136 insertions(+), 27 deletions(-)
---
base-commit: 1590cf0329716306e948a8fc29f1d3ee87d3989f
change-id: 20260724-upstream-ina2xx-in0-curr1-alarms-f85e7d692fdf

Best regards,
-- 
Jared Kangas <jkangas@redhat.com>

[PATCH v2 1/4] hwmon: (ina2xx) Acquire hwmon_lock in shunt_resistor_show()

[Jared Kangas]

shunt_resistor_store() currently acquires hwmon_lock to set
data->rshunt, but the corresponding access in shunt_resistor_show() is
unprotected. Acquire the lock in shunt_resistor_show() as well to ensure
proper synchronization.

Fixes: 3ad867001c91 ("hwmon: (ina2xx) fix sysfs shunt resistor read access")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260729162836.89BDF1F00A3A@smtp.kernel.org/
Signed-off-by: Jared Kangas <jkangas@redhat.com>
---
 drivers/hwmon/ina2xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index c4742e84b999d..f6c63ce1ef193 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -859,8 +859,12 @@ static ssize_t shunt_resistor_show(struct device *dev,
 				   struct device_attribute *da, char *buf)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
+	long rshunt;
 
-	return sysfs_emit(buf, "%li\n", data->rshunt);
+	scoped_guard(hwmon_lock, dev) {
+		rshunt = data->rshunt;
+	}
+	return sysfs_emit(buf, "%li\n", rshunt);
 }
 
 static ssize_t shunt_resistor_store(struct device *dev,

-- 
2.55.0

[PATCH v2 2/4] hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read()

[Jared Kangas]

Mirror ina226_alert_limit_read/write and use struct ina2xx_data instead
of struct regmap in ina226_alert_read's parameters.

Signed-off-by: Jared Kangas <jkangas@redhat.com>
---
 drivers/hwmon/ina2xx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index f6c63ce1ef193..c56e42432673f 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -476,12 +476,12 @@ static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
 	return 0;
 }
 
-static int ina226_alert_read(struct regmap *regmap, u32 mask, long *val)
+static int ina226_alert_read(struct ina2xx_data *data, u32 mask, long *val)
 {
 	unsigned int regval;
 	int ret;
 
-	ret = regmap_read_bypassed(regmap, INA226_MASK_ENABLE, &regval);
+	ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &regval);
 	if (ret)
 		return ret;
 
@@ -516,9 +516,9 @@ static int ina2xx_in_read(struct device *dev, u32 attr, int channel, long *val)
 		return ina226_alert_limit_read(data, over_voltage_mask,
 					       voltage_reg, val);
 	case hwmon_in_lcrit_alarm:
-		return ina226_alert_read(regmap, under_voltage_mask, val);
+		return ina226_alert_read(data, under_voltage_mask, val);
 	case hwmon_in_crit_alarm:
-		return ina226_alert_read(regmap, over_voltage_mask, val);
+		return ina226_alert_read(data, over_voltage_mask, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -573,7 +573,7 @@ static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
 		return ina226_alert_limit_read(data, INA226_POWER_OVER_LIMIT_MASK,
 					       INA2XX_POWER, val);
 	case hwmon_power_crit_alarm:
-		return ina226_alert_read(data->regmap, INA226_POWER_OVER_LIMIT_MASK, val);
+		return ina226_alert_read(data, INA226_POWER_OVER_LIMIT_MASK, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -615,9 +615,9 @@ static int ina2xx_curr_read(struct device *dev, u32 attr, long *val)
 		return ina226_alert_limit_read(data, INA226_SHUNT_OVER_VOLTAGE_MASK,
 					       INA2XX_CURRENT, val);
 	case hwmon_curr_lcrit_alarm:
-		return ina226_alert_read(regmap, INA226_SHUNT_UNDER_VOLTAGE_MASK, val);
+		return ina226_alert_read(data, INA226_SHUNT_UNDER_VOLTAGE_MASK, val);
 	case hwmon_curr_crit_alarm:
-		return ina226_alert_read(regmap, INA226_SHUNT_OVER_VOLTAGE_MASK, val);
+		return ina226_alert_read(data, INA226_SHUNT_OVER_VOLTAGE_MASK, val);
 	default:
 		return -EOPNOTSUPP;
 	}

-- 
2.55.0

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

[Jared Kangas]

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 semantically separates current from
shunt voltage in helpers that use function masks, which previously saw
the same mask for the two functions.

Signed-off-by: Jared Kangas <jkangas@redhat.com>
---
 drivers/hwmon/ina2xx.c | 90 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 67 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index c56e42432673f..74c4e3795ac4d 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -127,6 +127,17 @@ enum ina2xx_ids {
 	sy24655
 };
 
+enum ina2xx_alert_type {
+	INA2XX_ALERT_NONE,
+	INA2XX_ALERT_CURRENT_LOW,
+	INA2XX_ALERT_CURRENT_HIGH,
+	INA2XX_ALERT_POWER_HIGH,
+	INA2XX_ALERT_BUS_VOLTAGE_LOW,
+	INA2XX_ALERT_BUS_VOLTAGE_HIGH,
+	INA2XX_ALERT_SHUNT_VOLTAGE_LOW,
+	INA2XX_ALERT_SHUNT_VOLTAGE_HIGH,
+};
+
 struct ina2xx_config {
 	u16 config_default;
 	bool has_alerts;	/* chip supports alerts and limits */
@@ -406,16 +417,43 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
 	}
 }
 
-static int ina226_alert_limit_read(struct ina2xx_data *data, u32 mask, int reg, long *val)
+static u32 ina2xx_alert_type_to_mask(enum ina2xx_alert_type alert)
+{
+	switch (alert) {
+	case INA2XX_ALERT_CURRENT_LOW:
+	case INA2XX_ALERT_SHUNT_VOLTAGE_LOW:
+		return INA226_SHUNT_UNDER_VOLTAGE_MASK;
+	case INA2XX_ALERT_CURRENT_HIGH:
+	case INA2XX_ALERT_SHUNT_VOLTAGE_HIGH:
+		return INA226_SHUNT_OVER_VOLTAGE_MASK;
+	case INA2XX_ALERT_BUS_VOLTAGE_LOW:
+		return INA226_BUS_UNDER_VOLTAGE_MASK;
+	case INA2XX_ALERT_BUS_VOLTAGE_HIGH:
+		return INA226_BUS_OVER_VOLTAGE_MASK;
+	case INA2XX_ALERT_POWER_HIGH:
+		return INA226_POWER_OVER_LIMIT_MASK;
+	case INA2XX_ALERT_NONE:
+		return 0;
+	default:
+		/* programmer error */
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+}
+
+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) {
 		ret = regmap_read(regmap, INA226_ALERT_LIMIT, &regval);
 		if (ret)
@@ -427,9 +465,11 @@ static int ina226_alert_limit_read(struct ina2xx_data *data, u32 mask, int reg,
 	return 0;
 }
 
-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);
+	}
 	return 0;
 }
 
@@ -476,15 +518,17 @@ static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
 	return 0;
 }
 
-static int ina226_alert_read(struct ina2xx_data *data, u32 mask, long *val)
+static int ina226_alert_read(struct ina2xx_data *data, enum ina2xx_alert_type alert, long *val)
 {
 	unsigned int regval;
+	u32 mask;
 	int ret;
 
 	ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &regval);
 	if (ret)
 		return ret;
 
+	mask = ina2xx_alert_type_to_mask(alert);
 	*val = (regval & mask) && (regval & INA226_ALERT_FUNCTION_FLAG);
 
 	return 0;
@@ -493,10 +537,10 @@ static int ina226_alert_read(struct ina2xx_data *data, u32 mask, long *val)
 static int ina2xx_in_read(struct device *dev, u32 attr, int channel, long *val)
 {
 	int voltage_reg = channel ? INA2XX_BUS_VOLTAGE : INA2XX_SHUNT_VOLTAGE;
-	u32 under_voltage_mask = channel ? INA226_BUS_UNDER_VOLTAGE_MASK
-					 : INA226_SHUNT_UNDER_VOLTAGE_MASK;
-	u32 over_voltage_mask = channel ? INA226_BUS_OVER_VOLTAGE_MASK
-					: INA226_SHUNT_OVER_VOLTAGE_MASK;
+	enum ina2xx_alert_type under_voltage_alert = channel ? INA2XX_ALERT_BUS_VOLTAGE_LOW
+							     : INA2XX_ALERT_SHUNT_VOLTAGE_LOW;
+	enum ina2xx_alert_type over_voltage_alert = channel ? INA2XX_ALERT_BUS_VOLTAGE_HIGH
+							    : INA2XX_ALERT_SHUNT_VOLTAGE_HIGH;
 	struct ina2xx_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
 	unsigned int regval;
@@ -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;
 	case hwmon_in_lcrit:
-		return ina226_alert_limit_read(data, under_voltage_mask,
+		return ina226_alert_limit_read(data, under_voltage_alert,
 					       voltage_reg, val);
 	case hwmon_in_crit:
-		return ina226_alert_limit_read(data, over_voltage_mask,
+		return ina226_alert_limit_read(data, over_voltage_alert,
 					       voltage_reg, val);
 	case hwmon_in_lcrit_alarm:
-		return ina226_alert_read(data, under_voltage_mask, val);
+		return ina226_alert_read(data, under_voltage_alert, val);
 	case hwmon_in_crit_alarm:
-		return ina226_alert_read(data, over_voltage_mask, val);
+		return ina226_alert_read(data, over_voltage_alert, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -570,10 +614,10 @@ static int ina2xx_power_read(struct device *dev, u32 attr, long *val)
 	case hwmon_power_average:
 		return sy24655_average_power_read(data, SY24655_EIN, val);
 	case hwmon_power_crit:
-		return ina226_alert_limit_read(data, INA226_POWER_OVER_LIMIT_MASK,
+		return ina226_alert_limit_read(data, INA2XX_ALERT_POWER_HIGH,
 					       INA2XX_POWER, val);
 	case hwmon_power_crit_alarm:
-		return ina226_alert_read(data, INA226_POWER_OVER_LIMIT_MASK, val);
+		return ina226_alert_read(data, INA2XX_ALERT_POWER_HIGH, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -609,15 +653,15 @@ static int ina2xx_curr_read(struct device *dev, u32 attr, long *val)
 		*val = ina2xx_get_value(data, INA2XX_CURRENT, regval);
 		return 0;
 	case hwmon_curr_lcrit:
-		return ina226_alert_limit_read(data, INA226_SHUNT_UNDER_VOLTAGE_MASK,
+		return ina226_alert_limit_read(data, INA2XX_ALERT_CURRENT_LOW,
 					       INA2XX_CURRENT, val);
 	case hwmon_curr_crit:
-		return ina226_alert_limit_read(data, INA226_SHUNT_OVER_VOLTAGE_MASK,
+		return ina226_alert_limit_read(data, INA2XX_ALERT_CURRENT_HIGH,
 					       INA2XX_CURRENT, val);
 	case hwmon_curr_lcrit_alarm:
-		return ina226_alert_read(data, INA226_SHUNT_UNDER_VOLTAGE_MASK, val);
+		return ina226_alert_read(data, INA2XX_ALERT_CURRENT_LOW, val);
 	case hwmon_curr_crit_alarm:
-		return ina226_alert_read(data, INA226_SHUNT_OVER_VOLTAGE_MASK, val);
+		return ina226_alert_read(data, INA2XX_ALERT_CURRENT_HIGH, val);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -661,12 +705,12 @@ static int ina2xx_in_write(struct device *dev, u32 attr, int channel, long val)
 	switch (attr) {
 	case hwmon_in_lcrit:
 		return ina226_alert_limit_write(data,
-			channel ? INA226_BUS_UNDER_VOLTAGE_MASK : INA226_SHUNT_UNDER_VOLTAGE_MASK,
+			channel ? INA2XX_ALERT_BUS_VOLTAGE_LOW : INA2XX_ALERT_SHUNT_VOLTAGE_LOW,
 			channel ? INA2XX_BUS_VOLTAGE : INA2XX_SHUNT_VOLTAGE,
 			val);
 	case hwmon_in_crit:
 		return ina226_alert_limit_write(data,
-			channel ? INA226_BUS_OVER_VOLTAGE_MASK : INA226_SHUNT_OVER_VOLTAGE_MASK,
+			channel ? INA2XX_ALERT_BUS_VOLTAGE_HIGH : INA2XX_ALERT_SHUNT_VOLTAGE_HIGH,
 			channel ? INA2XX_BUS_VOLTAGE : INA2XX_SHUNT_VOLTAGE,
 			val);
 	default:
@@ -681,7 +725,7 @@ static int ina2xx_power_write(struct device *dev, u32 attr, long val)
 
 	switch (attr) {
 	case hwmon_power_crit:
-		return ina226_alert_limit_write(data, INA226_POWER_OVER_LIMIT_MASK,
+		return ina226_alert_limit_write(data, INA2XX_ALERT_POWER_HIGH,
 						INA2XX_POWER, val);
 	default:
 		return -EOPNOTSUPP;
@@ -695,10 +739,10 @@ static int ina2xx_curr_write(struct device *dev, u32 attr, long val)
 
 	switch (attr) {
 	case hwmon_curr_lcrit:
-		return ina226_alert_limit_write(data, INA226_SHUNT_UNDER_VOLTAGE_MASK,
+		return ina226_alert_limit_write(data, INA2XX_ALERT_CURRENT_LOW,
 						INA2XX_CURRENT, val);
 	case hwmon_curr_crit:
-		return ina226_alert_limit_write(data, INA226_SHUNT_OVER_VOLTAGE_MASK,
+		return ina226_alert_limit_write(data, INA2XX_ALERT_CURRENT_HIGH,
 						INA2XX_CURRENT, val);
 	default:
 		return -EOPNOTSUPP;

-- 
2.55.0

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

[Jared Kangas]

INA2XX current limits are converted into shunt voltage limits internally
using the shunt resistor value. Once a current limit's corresponding
voltage limit is written to the hardware, shunt voltage and current
alarms are indistinguishable from each other.

This causes two issues:

1. in0/curr1 alarms may be unintentionally cleared by reading from the
   opposite input's alarm.

2. When a limit for either in0 (shunt voltage) or curr1 (current) is
   set, both of their alarms are triggered, and both of their limits
   read nonzero.

An example of this behavior on an INA231:

        # cd /sys/class/hwmon/hwmon0
        # head {curr1,in0}_input
        ==> curr1_input <==
        1713

        ==> in0_input <==
        2
        # echo 1800 >curr1_lcrit
        # head {curr1,in0}_lcrit_alarm
        ==> curr1_lcrit_alarm <==
        1

        ==> in0_lcrit_alarm <==
        0
        # head {in0,curr1}_lcrit_alarm
        ==> in0_lcrit_alarm <==
        1

        ==> curr1_lcrit_alarm <==
        0
        # head {in0,curr1}_lcrit_alarm
        ==> in0_lcrit_alarm <==
        1

        ==> curr1_lcrit_alarm <==
        1

This is because curr1 uses the same underlying masks
(INA226_SHUNT_*_VOLTAGE_MASK) as in0 on the hardware. As a result,
ina2xx_{curr,in}_read() both read the shunt voltage alarms/limits
without considering whether the voltage or current is currently set.

To fix this, track the active alarm type in ina2xx_data and guard
alarm/limit reads with a check that returns zero if the active alarm is
for a different type. The new field is initialized based on the
MASK_ENABLE register's set function, assuming voltage instead of current
when the shunt voltage mask is set.

After this fix, the alarms only read back 1 if their corresponding limit
is set:

        # echo 0 >curr1_lcrit
        # head {curr1,in0}_lcrit_alarm
        ==> curr1_lcrit_alarm <==
        0

        ==> in0_lcrit_alarm <==
        0
        # echo 9999 >curr1_lcrit
        # head {curr1,in0}_lcrit_alarm
        ==> curr1_lcrit_alarm <==
        1

        ==> in0_lcrit_alarm <==
        0
        # echo 9999 >in0_lcrit
        # head {curr1,in0}_lcrit_alarm
        ==> curr1_lcrit_alarm <==
        0

        ==> in0_lcrit_alarm <==
        1

Fixes: 4d5c2d986757 ("hwmon: (ina2xx) Add support for current limits")
Signed-off-by: Jared Kangas <jkangas@redhat.com>
---
 drivers/hwmon/ina2xx.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 74c4e3795ac4d..7e5b412ff47f2 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/bits.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -157,6 +158,7 @@ struct ina2xx_data {
 	const struct ina2xx_config *config;
 	enum ina2xx_ids chip;
 
+	enum ina2xx_alert_type active_alert;
 	long rshunt;
 	long current_lsb_uA;
 	long power_lsb_uW;
@@ -441,6 +443,35 @@ static u32 ina2xx_alert_type_to_mask(enum ina2xx_alert_type alert)
 	}
 }
 
+static enum ina2xx_alert_type ina2xx_mask_to_alert_type(u32 mask)
+{
+	int top_bit = fls(mask & INA226_ALERT_CONFIG_MASK);
+
+	if (!top_bit)
+		return INA2XX_ALERT_NONE;
+
+	/*
+	 * Multiple bits may be set, with the highest-set function taking
+	 * precedence according to the datasheet. Shunt voltage masks are
+	 * assumed to map to voltage monitoring rather than current monitoring,
+	 * since the latter isn't directly implemented in the hardware.
+	 */
+	switch (BIT(top_bit - 1)) {
+	case INA226_SHUNT_OVER_VOLTAGE_MASK:
+		return INA2XX_ALERT_SHUNT_VOLTAGE_HIGH;
+	case INA226_SHUNT_UNDER_VOLTAGE_MASK:
+		return INA2XX_ALERT_SHUNT_VOLTAGE_LOW;
+	case INA226_BUS_OVER_VOLTAGE_MASK:
+		return INA2XX_ALERT_BUS_VOLTAGE_HIGH;
+	case INA226_BUS_UNDER_VOLTAGE_MASK:
+		return INA2XX_ALERT_BUS_VOLTAGE_LOW;
+	case INA226_POWER_OVER_LIMIT_MASK:
+		return INA2XX_ALERT_POWER_HIGH;
+	default:
+		return INA2XX_ALERT_NONE;
+	}
+}
+
 static int ina226_alert_limit_read(struct ina2xx_data *data, enum ina2xx_alert_type alert,
 				   int reg, long *val)
 {
@@ -449,6 +480,12 @@ static int ina226_alert_limit_read(struct ina2xx_data *data, enum ina2xx_alert_t
 	u32 mask;
 	int ret;
 
+	/* Avoid nonzero reads from inactive alerts caused by shared limit register */
+	if (data->active_alert != alert) {
+		*val = 0;
+		return 0;
+	}
+
 	ret = regmap_read(regmap, INA226_MASK_ENABLE, &regval);
 	if (ret)
 		return ret;
@@ -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;
 }
 
@@ -524,6 +566,15 @@ static int ina226_alert_read(struct ina2xx_data *data, enum ina2xx_alert_type al
 	u32 mask;
 	int ret;
 
+	/*
+	 * With alert latching, reading alerts from hardware also clears the
+	 * alert, so return early if the alert is inactive.
+	 */
+	if (data->active_alert != alert) {
+		*val = 0;
+		return 0;
+	}
+
 	ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &regval);
 	if (ret)
 		return ret;
@@ -964,6 +1015,16 @@ static int ina2xx_init(struct device *dev, struct ina2xx_data *data)
 
 	if (data->config->has_alerts) {
 		bool active_high = device_property_read_bool(dev, "ti,alert-polarity-active-high");
+		unsigned int mask_enable;
+
+		/*
+		 * Infer active alert from MASK_ENABLE in case it's already
+		 * configured (e.g., by a past probe or firmware)
+		 */
+		ret = regmap_read(regmap, INA226_MASK_ENABLE, &mask_enable);
+		if (ret < 0)
+			return ret;
+		data->active_alert = ina2xx_mask_to_alert_type(mask_enable);
 
 		regmap_update_bits(regmap, INA226_MASK_ENABLE,
 				   INA226_ALERT_LATCH_ENABLE | INA226_ALERT_POLARITY,

-- 
2.55.0

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

[sashiko-bot]

> 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 semantically separates current from
> shunt voltage in helpers that use function masks, which previously saw
> the same mask for the two functions.
> 
> Signed-off-by: Jared Kangas <jkangas@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v2 1/4] hwmon: (ina2xx) Acquire hwmon_lock in shunt_resistor_show()

[sashiko-bot]

> shunt_resistor_store() currently acquires hwmon_lock to set
> data->rshunt, but the corresponding access in shunt_resistor_show() is
> unprotected. Acquire the lock in shunt_resistor_show() as well to ensure
> proper synchronization.
> 
> Fixes: 3ad867001c91 ("hwmon: (ina2xx) fix sysfs shunt resistor read access")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/20260729162836.89BDF1F00A3A@smtp.kernel.org/
> Signed-off-by: Jared Kangas <jkangas@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v2 2/4] hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read()

[sashiko-bot]

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

Pre-existing issues:
- [High] Arithmetic overflows and incorrect clamping logic cause alert limits to be wrongly configured to 0 instead of maximum for shunt voltage and power alerts on 32-bit systems.
--

--- Patch [2]: [PATCH v2 2/4] hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read() ---
commit 034555c6bd153d8c094cfb7d04cc7d98d34d3774
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Parameterize ina2xx_data in ina226_alert_read()

This commit updates ina226_alert_read() to take a struct ina2xx_data pointer
instead of a struct regmap pointer. This aligns its parameters with
ina226_alert_limit_read() and ina226_alert_limit_write().

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index f6c63ce1ef193..c56e42432673f 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -476,12 +476,12 @@ static int ina2xx_chip_read(struct device *dev, u32 attr, long *val)
>  	return 0;
>  }
>  
> -static int ina226_alert_read(struct regmap *regmap, u32 mask, long *val)
> +static int ina226_alert_read(struct ina2xx_data *data, u32 mask, long *val)
>  {
>  	unsigned int regval;
>  	int ret;

[ ... ]

[Severity: High]
This is a pre-existing issue, and was not introduced by the patch under review,
but does ina226_alert_to_reg() have arithmetic overflows and incorrect clamping
logic on 32-bit systems?

When a user or configuration script sets a valid power limit or a large shunt
voltage limit on a 32-bit system, it appears the values might collapse to zero:

drivers/hwmon/ina2xx.c:ina226_alert_to_reg() {
    ...
	case INA2XX_SHUNT_VOLTAGE:
		val = clamp_val(val, 0, SHRT_MAX * data->config->shunt_div);
		val *= data->config->shunt_div;
		val <<= data->config->shunt_voltage_shift;
		return clamp_val(val, 0, SHRT_MAX);
    ...
	case INA2XX_POWER:
		val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
		val = DIV_ROUND_CLOSEST(val, data->power_lsb_uW);
		return clamp_val(val, 0, USHRT_MAX);
    ...
}

For INA2XX_POWER, could UINT_MAX - data->power_lsb_uW implicitly cast to a
negative long upper bound in the clamp_val macro, forcing all limits to
negative?

And for INA2XX_SHUNT_VOLTAGE, is the clamp mistakenly using multiplication
(SHRT_MAX * data->config->shunt_div) instead of division, allowing large values
that later overflow a signed 32-bit long multiplication?

In both cases, it looks like the final step clamps the negative intermediate
value to 0, which could cause immediate false hardware alerts if the alert pin
is wired to hardware reset logic.

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

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

[sashiko-bot]

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