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

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

Some prep work is done in patches 1-3 to simplify the fix in patch 4.

Signed-off-by: Jared Kangas <jkangas@redhat.com>
---
Jared Kangas (4):
      hwmon: (ina2xx) Replace regmap in ina226_alert_read() with ina2xx_data
      hwmon: (ina2xx) Replace masks with enum in alert functions
      hwmon: (ina2xx) Track active alarm in ina2xx_data
      hwmon: (ina2xx) Return 0 on inactive alarm/limit reads

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

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

[PATCH 1/4] hwmon: (ina2xx) Replace regmap in ina226_alert_read() with ina2xx_data

[Jared Kangas]

Accept a struct ina2xx_data instead of a struct regmap in
ina226_alert_read() to mirror ina226_alert_limit_{read,write}().

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 c4742e84b999d..911bf32e8f215 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

Re: [PATCH 1/4] hwmon: (ina2xx) Replace regmap in ina226_alert_read() with ina2xx_data

[sashiko-bot]

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

Pre-existing issues:
- [High] Reading an inactive alarm attribute clears the hardware alert flag, causing the active alarm to falsely report as inactive (0).
--

--- Patch [1]: [PATCH 1/4] hwmon: (ina2xx) Replace regmap in ina226_alert_read() with ina2xx_data ---
commit 966375fc88632d14769d99061017ac107e2c8899
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Replace regmap in ina226_alert_read() with ina2xx_data

This commit updates ina226_alert_read() to accept a struct ina2xx_data
pointer instead of a struct regmap. This mirrors the signatures used by
the alert limit read and write functions.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> --- 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)

[ ... ]

> -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);

[Severity: High]
This is a pre-existing issue, but I have a question about how the hardware
alert state is managed here in ina226_alert_read().

Since the INA226_MASK_ENABLE register clears the Alert Function Flag when
read, does this bypassed hardware read clear the alert flag unintentionally
if an inactive alarm attribute is polled?

For example, if userspace utilities sequentially read sysfs alarm attributes,
could checking an inactive alarm trigger this clear-on-read behavior before
the actually active alarm is polled? It seems this sequence would cause the
active alarm to falsely report as inactive (0).

>  	if (ret)
>  		return ret;

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

[PATCH 2/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 allows distinguishing current from
shunt voltage, which are both based on the same function in the
hardware.

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 911bf32e8f215..beb851d87f787 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

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

[sashiko-bot]

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

[PATCH 3/4] hwmon: (ina2xx) Track active alarm in ina2xx_data

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

To allow distinguishing current and shunt voltage alarms, track the
active alarm type in ina2xx_data. The new field is initialized based on
the MASK_ENABLE register's set function.

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

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index beb851d87f787..8ba0dce1f9e31 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -157,6 +157,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 +442,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)
 {
@@ -492,9 +522,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 = val ? alert : INA2XX_ALERT_NONE;
 	return 0;
 }
 
@@ -960,6 +994,12 @@ 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;
+
+		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 3/4] hwmon: (ina2xx) Track active alarm in ina2xx_data

[sashiko-bot]

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

New issues:
- [High] Writing 0 to an inactive alert limit unintentionally clears the hardware mask, disabling the unrelated, currently active alert limit.
- [Medium] The `ina226_alert_read` function fails to use the new `active_alert` disambiguation state, causing unconfigured alarm attributes to incorrectly return 1 if another alarm triggers.

Pre-existing issues:
- [High] On 32-bit architectures, setting any positive power limit results in a limit of 0 due to an integer underflow when interacting with `clamp_val()`.
- [High] Setting large shunt voltage limits causes an integer overflow on 32-bit architectures, resulting in a limit of 0 instead of the maximum clamped limit.
--

--- Patch [3]: [PATCH 3/4] hwmon: (ina2xx) Track active alarm in ina2xx_data ---
commit 480990f993b0efa725eee2c5b0c2a864cd60bcfb
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Track active alarm in ina2xx_data

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.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -157,6 +157,7 @@ struct ina2xx_data {
>  	const struct ina2xx_config *config;
>  	enum ina2xx_ids chip;
>  
> +	enum ina2xx_alert_type active_alert;

[Severity: Medium]
Will this new state also be checked in ina226_alert_read()?

If an unconfigured alarm triggers, could it incorrectly return 1 since both
alarms map to the same hardware mask?

drivers/hwmon/ina2xx.c:ina226_alert_read() {
    ...
    mask = ina2xx_alert_type_to_mask(alert);
    *val = (regval & mask) && (regval & INA226_ALERT_FUNCTION_FLAG);
    ...
}

[ ... ]

> @@ -492,9 +522,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 = val ? alert : INA2XX_ALERT_NONE;

[Severity: High]
If a user writes 0 to an inactive alert limit, will it unintentionally clear
the hardware mask at the beginning of ina226_alert_limit_write() and wipe out
the currently active alert?

drivers/hwmon/ina2xx.c:ina226_alert_limit_write() {
    ...
    ret = regmap_update_bits(regmap, INA226_MASK_ENABLE,
                 INA226_ALERT_CONFIG_MASK, 0);
    ...
}

[Severity: High]
This is a pre-existing issue, but on 32-bit architectures, does this clamp
operation compute a negative boundary for INA2XX_POWER?

drivers/hwmon/ina2xx.c:ina226_alert_to_reg() {
    ...
    case INA2XX_POWER:
        val = clamp_val(val, 0, UINT_MAX - data->power_lsb_uW);
    ...
}

Because UINT_MAX is cast to signed long by clamp_val(), it could result in a
negative upper bound, forcing val to a negative number which is then clamped
to 0.

[Severity: High]
This is also a pre-existing issue, but does setting a large shunt voltage
limit cause an integer overflow on 32-bit architectures?

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;
    ...
}

If a user sets a large valid limit on a 32-bit system, the multiplication
might overflow the signed 32-bit long into a negative number, which is then
incorrectly clamped to 0.

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

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

[Jared Kangas]

Guard alarm/limit reads with a check that returns zero if the active
alarm is for a different type. This addresses 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.

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 8ba0dce1f9e31..5521490deba98 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -479,6 +479,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;
@@ -558,6 +564,12 @@ static int ina226_alert_read(struct ina2xx_data *data, enum ina2xx_alert_type al
 	u32 mask;
 	int ret;
 
+	/* Avoid reading inactive alerts, which may clear the active alert */
+	if (data->active_alert != alert) {
+		*val = 0;
+		return 0;
+	}
+
 	ret = regmap_read_bypassed(data->regmap, INA226_MASK_ENABLE, &regval);
 	if (ret)
 		return ret;

-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Writing `0` to an inactive alert limit attribute unintentionally disables the currently active alert limit of a completely different type.
- [High] In `ina2xx_get_value`, calculating `INA2XX_POWER` can overflow a 32-bit signed `int` for small shunt resistor values, resulting in negative power readings.
- [High] In `ina226_alert_to_reg`, `INA2XX_SHUNT_VOLTAGE` limits can overflow a 32-bit signed `long`, causing large limits to be incorrectly clamped to 0 on 32-bit systems.
- [High] In `ina226_alert_to_reg`, `INA2XX_POWER` limits always evaluate to 0 on 32-bit systems because `UINT_MAX` is cast to a negative `long` in the clamping macro.
- [Medium] The `shunt_resistor_show` function accesses shared driver state locklessly, violating HWMON subsystem locking guidelines.
--

--- Patch [4]: [PATCH 4/4] hwmon: (ina2xx) Return 0 on inactive alarm/limit reads ---
commit 4e821bd1003ecc2b04b0b58f9702178e2bd3a2eb
Author: Jared Kangas <jkangas@redhat.com>

hwmon: (ina2xx) Return 0 on inactive alarm/limit reads

Guard alarm/limit reads with a check that returns zero if the active
alarm is for a different type to avoid unintentionally clearing alarms.

> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 8ba0dce1f9e31..5521490deba98 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -479,6 +479,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;
> +	}
> +

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

[ ... ]

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

If a user configures a small shunt resistor (e.g., 1 mOhm), power_lsb_uW
becomes large. When the hardware measures high power, regval can also be
quite large. 

Since val is a 32-bit signed integer, could this multiplication overflow
on 32-bit architectures and result in incorrect negative power readings
being reported to userspace?

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

On a 32-bit system, long is 32 bits. If a user inputs a large limit, and
it passes the first clamp, the multiplication by shunt_div could exceed
the maximum value of a signed 32-bit integer.

Does this overflow the signed long, wrapping to a negative value that
then gets incorrectly clamped to 0 by the final clamp_val()?

[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);

The clamp_val macro casts bounds to typeof(val) which is a signed long. 

On 32-bit systems, does UINT_MAX - data->power_lsb_uW evaluate to a
large positive unsigned value that becomes negative when cast to a
signed long?

If the upper bound is negative, does this force clamp_val to rewrite all
positive user inputs to negative values, guaranteeing the function always
returns 0?

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

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