[PATCH v3 00/11] hwmon: chipcap2: various bug fixes

[PATCH v3 00/11] hwmon: chipcap2: various bug fixes

[Javier Carrasco]

The original issue (race condition for the alarms) was found by Sashiko[1]
when an unrelated patch affected chipcap2.c. The issue is real as the
access to the variables is not protected by any locking mechanism
although 2 different sources (threaded interrupts and sysfs) could
modify them.

In further iterations Sashiko found new issues[2] that have been added to
the series:

- Fix be16 data handling.
- Defer probe when the IRQs are not available yet.
- IRQ teardown ordering: split the current implementation to provide the
  available interrupts before registering the hwmon device, and register
  the IRQs after the hwmon device has been registered.
- Disable the device after the hwmon device has been unregistered.
- Enable IRQ processing when the regulator is already enabled.
- Balance regulator counter.
- Do not discard the pending alarms when the device is disabled.
- Reset the completion to account for counter > 1.
- IRQ serialization and synchronization.

The last item required a dedicated mutex because the hwmon_lock was not
suitable to close the gaps where an interrupt might go unnoticed because
there is some dependency between the data read and the interrupt
generation, and also in the completion mechanism. I went through the
different alternatives with a combination of local Sashiko and a second
AI agent, and discarded some complex solutions to close gaps that are
more theoretical than practical e.g. the interrupt was fired, but it did
not reach the controller due to nested GPIO expanders. I added the
Assisted-by: tags to those patches accordingly, although it seems that
this requirement will soon vanish.

Additionally, a harmless off-by-one in a for loop has been fixed to save
~80 ms after the last failed command retry, and the error reporting has
been fixed to show the right issue. These two ones have not been tagged
for stable as they are only meant to improve reaction time and logging
inaccuracies after a command fails. The deferred probe does not have the
stable tag as it is just a reaction if interrupts were provided but
still not available. Of course, I can add the tags to all patches
if required.

The fixes have been validated on real hardware with an Amphenol
ChipCap 2 CC2D23S sensor.

[1] Link: https://lore.kernel.org/linux-hwmon/20260625162114.417EA1F000E9@smtp.kernel.org/
[2] Link: https://lore.kernel.org/linux-hwmon/20260821093706.D1EB11F000E9@smtp.kernel.org/

To: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Changes in v3:
- Use a dedicated mutex instead of the hwmon subsystem lock to serialize
  the access to the alarm indicators, which no longer blocks the alarm
  handlers behind a sysfs read.
- New patch: do not discard the pending alarms on disable.
- New patch: synchronize the ready IRQ state.
- Register the data ready IRQ before registering the hwmon device (its
  handler does not use hwmon).
- Split the fixes into one patch per issue.
- Drop the patch to fix the channels in the humidity alarm
  notifications, which is already applied.
- Link to v2: https://lore.kernel.org/r/20260823-chipcap2_locks-v2-0-6a26c8e9e2fc@gmail.com

Changes in v2:
- Add fixes to the new issues reported by Sashiko.
- Use hwmon subsystem lock.
- Link to v1: https://lore.kernel.org/r/20260821-chipcap2_locks-v1-1-1d8ccabfc1b2@gmail.com

---
Javier Carrasco (11):
      hwmon: chipcap2: fix command error diagnostics
      hwmon: chipcap2: fix off-by-one in command mode retry loop
      hwmon: chipcap2: fix be16 handling
      hwmon: chipcap2: defer probe when IRQs are unavailable
      hwmon: chipcap2: fix IRQ teardown ordering
      hwmon: chipcap2: disable device after hwmon teardown
      hwmon: chipcap2: enable IRQ processing when regulator is already enabled
      hwmon: chipcap2: balance regulator state
      hwmon: chipcap2: do not discard pending alarms on disable
      hwmon: chipcap2: serialize access to the alarm indicators
      hwmon: chipcap2: synchronize ready IRQ state

 drivers/hwmon/chipcap2.c | 192 +++++++++++++++++++++++++++++++----------------
 1 file changed, 128 insertions(+), 64 deletions(-)
---
base-commit: 3857c2fe5449541e24afc5efdb0f81a8a8f9a3a0
change-id: 20260820-chipcap2_locks-c01013a24a0c

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH v3 01/11] hwmon: chipcap2: fix command error diagnostics

[Javier Carrasco]

The command response field only contains bits 1:0, while the
error flags are stored in bits 2:5. The existing diagnostic
checks test the extracted response field, so they can never report
an error flag.

Test the full status byte for the error flags, and check the
configuration-load flag instead of RAM parity for the last diagnostic.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 9bef767b589e..855057892852 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -167,13 +167,13 @@ static int cc2_cmd_response_diagnostic(struct device *dev, u8 status)
 	case CC2_RESPONSE_BUSY:
 		return -EBUSY;
 	case CC2_RESPONSE_NACK:
-		if (resp & CC2_ERR_CORR_EEPROM)
+		if (status & CC2_ERR_CORR_EEPROM)
 			dev_dbg(dev, "Command failed: corrected EEPROM\n");
-		if (resp & CC2_ERR_UNCORR_EEPROM)
+		if (status & CC2_ERR_UNCORR_EEPROM)
 			dev_dbg(dev, "Command failed: uncorrected EEPROM\n");
-		if (resp & CC2_ERR_RAM_PARITY)
+		if (status & CC2_ERR_RAM_PARITY)
 			dev_dbg(dev, "Command failed: RAM parity\n");
-		if (resp & CC2_ERR_RAM_PARITY)
+		if (status & CC2_ERR_CONFIG_LOAD)
 			dev_dbg(dev, "Command failed: configuration error\n");
 		return -ENODATA;
 	default:

-- 
2.43.0

Re: [PATCH v3 01/11] hwmon: chipcap2: fix command error diagnostics

[sashiko-bot]

> The command response field only contains bits 1:0, while the
> error flags are stored in bits 2:5. The existing diagnostic
> checks test the extracted response field, so they can never report
> an error flag.
> 
> Test the full status byte for the error flags, and check the
> configuration-load flag instead of RAM parity for the last diagnostic.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=1

[PATCH v3 02/11] hwmon: chipcap2: fix off-by-one in command mode retry loop

[Javier Carrasco]

i == CC2_CM_RETRIES can never be true inside the loop, since it
only runs while i < CC2_CM_RETRIES. Use CC2_CM_RETRIES - 1 so the
last failed retry breaks out immediately instead of doing a useless
disable + 80 ms power-cycle delay before the loop condition ends it
anyway.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 855057892852..8102b6851d3d 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -226,7 +226,7 @@ static int cc2_command_mode_start(struct cc2_data *data)
 				     2 * CC2_RESP_START_CM_US);
 		}
 		ret = cc2_read_command_status(data->client);
-		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES)
+		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES - 1)
 			break;
 
 		/* command window missed, prepare for a retry */

-- 
2.43.0

Re: [PATCH v3 02/11] hwmon: chipcap2: fix off-by-one in command mode retry loop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hwmon: chipcap2: premature timeout due to 1-jiffy wait_for_completion_timeout
--

commit 529f4f489744495e9cdde8072b4894cc25895048
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: fix off-by-one in command mode retry loop

i == CC2_CM_RETRIES can never be true inside the loop, since it
only runs while i < CC2_CM_RETRIES. Use CC2_CM_RETRIES - 1 so the
last failed retry breaks out immediately instead of doing a useless
disable + 80 ms power-cycle delay before the loop condition ends it
anyway.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 8550578928523..8102b6851d3d1 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -226,7 +226,7 @@ static int cc2_command_mode_start(struct cc2_data *data)
>  				     2 * CC2_RESP_START_CM_US);
>  		}
>  		ret = cc2_read_command_status(data->client);
> -		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES)
> +		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES - 1)

[Severity: High]
Can this still cause premature timeouts during device initialization and reads?

Looking at cc2_command_mode_start() just above this check, the timeout
calculation evaluates to exactly 1 jiffy on standard HZ configurations
(e.g., HZ=100 or HZ=250), because 200 microseconds is significantly less
than one tick:

	timeout = usecs_to_jiffies(2 * CC2_RESP_START_CM_US);
	ret = wait_for_completion_timeout(&data->complete, timeout);

When a 1-jiffy timeout is passed to wait_for_completion_timeout(), it can
expire almost instantly if the next timer interrupt occurs immediately after
the call.

This can cause the driver to prematurely abort the device initialization
without triggering a retry, returning -ETIMEDOUT sporadically. This leads
to random failures in device communication and user-space reads even if
the device properly signals completion within its 100-microsecond window.

This same 1-jiffy timeout issue also affects the EEPROM read sequence in
cc2_read_reg():

	timeout = usecs_to_jiffies(2 * CC2_RESP_EEPROM_R_US);
	ret = wait_for_completion_timeout(&data->complete, timeout);

>  			break;
>  
>  		/* command window missed, prepare for a retry */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=2

Re: [PATCH v3 02/11] hwmon: chipcap2: fix off-by-one in command mode retry loop

[Javier Carrasco]

On Wed Oct 14, 2026 at 8:01 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] hwmon: chipcap2: premature timeout due to 1-jiffy wait_for_completion_timeout
> --
>
> commit 529f4f489744495e9cdde8072b4894cc25895048
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> hwmon: chipcap2: fix off-by-one in command mode retry loop
>
> i == CC2_CM_RETRIES can never be true inside the loop, since it
> only runs while i < CC2_CM_RETRIES. Use CC2_CM_RETRIES - 1 so the
> last failed retry breaks out immediately instead of doing a useless
> disable + 80 ms power-cycle delay before the loop condition ends it
> anyway.
>
>> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
>> index 8550578928523..8102b6851d3d1 100644
>> --- a/drivers/hwmon/chipcap2.c
>> +++ b/drivers/hwmon/chipcap2.c
>> @@ -226,7 +226,7 @@ static int cc2_command_mode_start(struct cc2_data *data)
>>  				     2 * CC2_RESP_START_CM_US);
>>  		}
>>  		ret = cc2_read_command_status(data->client);
>> -		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES)
>> +		if (ret != -ETIMEDOUT || i == CC2_CM_RETRIES - 1)
>
> [Severity: High]
> Can this still cause premature timeouts during device initialization and reads?
>
> Looking at cc2_command_mode_start() just above this check, the timeout
> calculation evaluates to exactly 1 jiffy on standard HZ configurations
> (e.g., HZ=100 or HZ=250), because 200 microseconds is significantly less
> than one tick:
>
> 	timeout = usecs_to_jiffies(2 * CC2_RESP_START_CM_US);
> 	ret = wait_for_completion_timeout(&data->complete, timeout);
>
> When a 1-jiffy timeout is passed to wait_for_completion_timeout(), it can
> expire almost instantly if the next timer interrupt occurs immediately after
> the call.
>
> This can cause the driver to prematurely abort the device initialization
> without triggering a retry, returning -ETIMEDOUT sporadically. This leads
> to random failures in device communication and user-space reads even if
> the device properly signals completion within its 100-microsecond window.
>
> This same 1-jiffy timeout issue also affects the EEPROM read sequence in
> cc2_read_reg():
>
> 	timeout = usecs_to_jiffies(2 * CC2_RESP_EEPROM_R_US);
> 	ret = wait_for_completion_timeout(&data->complete, timeout);
>
>>  			break;
>>
>>  		/* command window missed, prepare for a retry */

This is a separate issue from what the patch addresses, which is the
off-by-one error in the retry loop, but it makes sense.

I have never observed a timeout during communication with the device,
but statistically, this seems plausible given the conversion from
microseconds to jiffies.

The most sensible approach would be to send a follow-up patch that
ensures the timeout is at least 2 jiffies, regardless of the HZ
configuration, using something like:

timeout = max(usecs_to_jiffies(2 * CC2_RESP_START_CM_US), 2UL);

The same change would apply to the EEPROM read timeout in cc2_read_reg().

Best regards,
Javier

[PATCH v3 03/11] hwmon: chipcap2: fix be16 handling

[Javier Carrasco]

The driver currently only works correctly on little-endian
architectures when writing 16-bit register values.

Use i2c_smbus_write_word_swapped() for writes and get_unaligned_be16()
for reads to correctly handle the ChipCap 2's big-endian data format
on all architectures.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 8102b6851d3d..aec4adf98658 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -21,6 +21,7 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/module.h>
+#include <linux/unaligned.h>
 #include <linux/regulator/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/property.h>
@@ -260,8 +261,7 @@ static int cc2_write_reg(struct cc2_data *data, u8 reg, u16 val)
 	if (ret < 0)
 		goto disable;
 
-	cpu_to_be16s(&val);
-	ret = i2c_smbus_write_word_data(data->client, reg, val);
+	ret = i2c_smbus_write_word_swapped(data->client, reg, val);
 	if (ret < 0)
 		goto disable;
 
@@ -311,7 +311,7 @@ static int cc2_read_reg(struct cc2_data *data, u8 reg, u16 *val)
 	if (ret != CC2_EEPROM_DATA_LEN)
 		return ret < 0 ? ret : -EIO;
 
-	*val = be16_to_cpup((__be16 *)&buf[1]);
+	*val = get_unaligned_be16(&buf[1]);
 
 	return cc2_read_command_status(data->client);
 }
@@ -351,10 +351,10 @@ static int cc2_data_fetch(struct i2c_client *client,
 
 	switch (type) {
 	case hwmon_humidity:
-		*val = cc2_rh_convert(be16_to_cpup((__be16 *)&data[0]));
+		*val = cc2_rh_convert(get_unaligned_be16(&data[0]));
 		break;
 	case hwmon_temp:
-		*val = cc2_temp_convert(be16_to_cpup((__be16 *)&data[2]));
+		*val = cc2_temp_convert(get_unaligned_be16(&data[2]));
 		break;
 	default:
 		return -EINVAL;

-- 
2.43.0

Re: [PATCH v3 03/11] hwmon: chipcap2: fix be16 handling

[sashiko-bot]

> The driver currently only works correctly on little-endian
> architectures when writing 16-bit register values.
> 
> Use i2c_smbus_write_word_swapped() for writes and get_unaligned_be16()
> for reads to correctly handle the ChipCap 2's big-endian data format
> on all architectures.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=3

[PATCH v3 04/11] hwmon: chipcap2: defer probe when IRQs are unavailable

[Javier Carrasco]

This device can work with or without interrupts, but if they
are provided and still not available, but if -EPROBE_DEFER is read,
its value must be propagated to enable probe retries.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index aec4adf98658..37dbf6a76fbc 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -636,6 +636,8 @@ static int cc2_request_ready_irq(struct cc2_data *data, struct device *dev)
 	int ret = 0;
 
 	data->irq_ready = fwnode_irq_get_byname(dev_fwnode(dev), "ready");
+	if (data->irq_ready == -EPROBE_DEFER)
+		return data->irq_ready;
 	if (data->irq_ready > 0) {
 		init_completion(&data->complete);
 		ret = devm_request_threaded_irq(dev, data->irq_ready, NULL,
@@ -653,6 +655,8 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
 	int ret = 0;
 
 	data->irq_low = fwnode_irq_get_byname(dev_fwnode(dev), "low");
+	if (data->irq_low == -EPROBE_DEFER)
+		return data->irq_low;
 	if (data->irq_low > 0) {
 		ret = devm_request_threaded_irq(dev, data->irq_low, NULL,
 						cc2_low_interrupt,
@@ -666,6 +670,8 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
 	}
 
 	data->irq_high = fwnode_irq_get_byname(dev_fwnode(dev), "high");
+	if (data->irq_high == -EPROBE_DEFER)
+		return data->irq_high;
 	if (data->irq_high > 0) {
 		ret = devm_request_threaded_irq(dev, data->irq_high, NULL,
 						cc2_high_interrupt,

-- 
2.43.0

Re: [PATCH v3 04/11] hwmon: chipcap2: defer probe when IRQs are unavailable

[sashiko-bot]

> This device can work with or without interrupts, but if they
> are provided and still not available, but if -EPROBE_DEFER is read,
> its value must be propagated to enable probe retries.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=4

[PATCH v3 05/11] hwmon: chipcap2: fix IRQ teardown ordering

[Javier Carrasco]

The threaded IRQs are currently requested before the hwmon device
is registered. Since devres releases resources in reverse order,
the hwmon device is unregistered before the IRQs are freed during
driver teardown. An IRQ may therefore still run after the hwmon
device has been released and access the stale data->hwmon pointer.

Separate IRQ discovery from IRQ registration so that the IRQ numbers
and alarm visibility flags are initialized before registering the
hwmon device while the IRQs are released before the hwmon device
during devres cleanup.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 52 +++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 37dbf6a76fbc..21dc6db27c40 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -631,32 +631,47 @@ static int cc2_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	}
 }
 
-static int cc2_request_ready_irq(struct cc2_data *data, struct device *dev)
+static int cc2_get_irqs(struct cc2_data *data, struct device *dev)
 {
-	int ret = 0;
-
 	data->irq_ready = fwnode_irq_get_byname(dev_fwnode(dev), "ready");
 	if (data->irq_ready == -EPROBE_DEFER)
 		return data->irq_ready;
-	if (data->irq_ready > 0) {
+	if (data->irq_ready > 0)
 		init_completion(&data->complete);
+
+	data->irq_low = fwnode_irq_get_byname(dev_fwnode(dev), "low");
+	if (data->irq_low == -EPROBE_DEFER)
+		return data->irq_low;
+	if (data->irq_low > 0)
+		data->rh_alarm.low_alarm_visible = true;
+
+	data->irq_high = fwnode_irq_get_byname(dev_fwnode(dev), "high");
+	if (data->irq_high == -EPROBE_DEFER)
+		return data->irq_high;
+	if (data->irq_high > 0)
+		data->rh_alarm.high_alarm_visible = true;
+
+	return 0;
+}
+
+static int cc2_request_data_ready(struct cc2_data *data, struct device *dev)
+{
+	int ret = 0;
+
+	if (data->irq_ready > 0)
 		ret = devm_request_threaded_irq(dev, data->irq_ready, NULL,
 						cc2_ready_interrupt,
 						IRQF_ONESHOT |
 						IRQF_TRIGGER_RISING,
 						dev_name(dev), data);
-	}
 
 	return ret;
 }
 
-static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
+static int cc2_request_alarms(struct cc2_data *data, struct device *dev)
 {
 	int ret = 0;
 
-	data->irq_low = fwnode_irq_get_byname(dev_fwnode(dev), "low");
-	if (data->irq_low == -EPROBE_DEFER)
-		return data->irq_low;
 	if (data->irq_low > 0) {
 		ret = devm_request_threaded_irq(dev, data->irq_low, NULL,
 						cc2_low_interrupt,
@@ -665,13 +680,8 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
 						dev_name(dev), data);
 		if (ret)
 			return ret;
-
-		data->rh_alarm.low_alarm_visible = true;
 	}
 
-	data->irq_high = fwnode_irq_get_byname(dev_fwnode(dev), "high");
-	if (data->irq_high == -EPROBE_DEFER)
-		return data->irq_high;
 	if (data->irq_high > 0) {
 		ret = devm_request_threaded_irq(dev, data->irq_high, NULL,
 						cc2_high_interrupt,
@@ -680,8 +690,6 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
 						dev_name(dev), data);
 		if (ret)
 			return ret;
-
-		data->rh_alarm.high_alarm_visible = true;
 	}
 
 	return ret;
@@ -732,13 +740,13 @@ static int cc2_probe(struct i2c_client *client)
 
 	device_property_read_string(dev, "label", &data->label);
 
-	ret = cc2_request_ready_irq(data, dev);
+	ret = cc2_get_irqs(data, dev);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to request ready irq\n");
+		return dev_err_probe(dev, ret, "Failed to get IRQs\n");
 
-	ret = cc2_request_alarm_irqs(data, dev);
+	ret = cc2_request_data_ready(data, dev);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to request alarm irqs\n");
+		return dev_err_probe(dev, ret, "Failed to request ready irq\n");
 
 	data->hwmon = devm_hwmon_device_register_with_info(dev, client->name,
 							   data, &cc2_chip_info,
@@ -747,6 +755,10 @@ static int cc2_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(data->hwmon),
 				     "Failed to register hwmon device\n");
 
+	ret = cc2_request_alarms(data, dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request alarm irqs\n");
+
 	return 0;
 }
 

-- 
2.43.0

Re: [PATCH v3 05/11] hwmon: chipcap2: fix IRQ teardown ordering

[sashiko-bot]

> The threaded IRQs are currently requested before the hwmon device
> is registered. Since devres releases resources in reverse order,
> the hwmon device is unregistered before the IRQs are freed during
> driver teardown. An IRQ may therefore still run after the hwmon
> device has been released and access the stale data->hwmon pointer.
> 
> Separate IRQ discovery from IRQ registration so that the IRQ numbers
> and alarm visibility flags are initialized before registering the
> hwmon device while the IRQs are released before the hwmon device
> during devres cleanup.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=5

[PATCH v3 06/11] hwmon: chipcap2: disable device after hwmon teardown

[Javier Carrasco]

cc2_remove() disables the device while the device-managed hwmon device is
still registered. A concurrent sysfs callback can then enable the device
after it has been disabled during removal.

Register a device-managed disable action before registering the hwmon
device, so devres unregisters the hwmon device before disabling the
physical device.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 21dc6db27c40..654f6420da7a 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -152,6 +152,11 @@ static void cc2_disable(struct cc2_data *data)
 	}
 }
 
+static void cc2_disable_action(void *data)
+{
+	cc2_disable(data);
+}
+
 static int cc2_cmd_response_diagnostic(struct device *dev, u8 status)
 {
 	int resp;
@@ -744,6 +749,10 @@ static int cc2_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to get IRQs\n");
 
+	ret = devm_add_action_or_reset(dev, cc2_disable_action, data);
+	if (ret)
+		return ret;
+
 	ret = cc2_request_data_ready(data, dev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to request ready irq\n");
@@ -762,13 +771,6 @@ static int cc2_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void cc2_remove(struct i2c_client *client)
-{
-	struct cc2_data *data = i2c_get_clientdata(client);
-
-	cc2_disable(data);
-}
-
 static const struct i2c_device_id cc2_id[] = {
 	{ .name = "cc2d23" },
 	{ .name = "cc2d23s" },
@@ -801,7 +803,6 @@ static struct i2c_driver cc2_driver = {
 		.of_match_table = cc2_of_match,
 	},
 	.probe		= cc2_probe,
-	.remove		= cc2_remove,
 	.id_table = cc2_id,
 };
 module_i2c_driver(cc2_driver);

-- 
2.43.0

Re: [PATCH v3 06/11] hwmon: chipcap2: disable device after hwmon teardown

[sashiko-bot]

> cc2_remove() disables the device while the device-managed hwmon device is
> still registered. A concurrent sysfs callback can then enable the device
> after it has been disabled during removal.
> 
> Register a device-managed disable action before registering the hwmon
> device, so devres unregisters the hwmon device before disabling the
> physical device.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=6

[PATCH v3 07/11] hwmon: chipcap2: enable IRQ processing when regulator is already enabled

[Javier Carrasco]

cc2_enable() returns early when the regulator is already enabled.
This can happen if a previous regulator_disable() failed or if the
regulator was configured as always-on.

In this case, process_irqs remains false, causing the ready interrupt
to be ignored. Subsequent operations waiting for the ready completion
then time out.

Set process_irqs before returning when the regulator is already
enabled, so that the driver continues to process ready interrupts.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 654f6420da7a..837b1830f2e4 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -120,8 +120,10 @@ static int cc2_enable(struct cc2_data *data)
 	int ret;
 
 	/* exclusive regulator, check in case a disable failed */
-	if (regulator_is_enabled(data->regulator))
+	if (regulator_is_enabled(data->regulator)) {
+		data->process_irqs = true;
 		return 0;
+	}
 
 	/* clear any pending completion */
 	try_wait_for_completion(&data->complete);

-- 
2.43.0

Re: [PATCH v3 07/11] hwmon: chipcap2: enable IRQ processing when regulator is already enabled

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: chipcap2: edge-triggered alarms lost for always-on devices
--

commit d3d969f96bc4b1b75c3078d08aa3eae3d286d7f8
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: enable IRQ processing when regulator is already enabled

This commit sets process_irqs to true before returning from cc2_enable()
when the regulator is already enabled. This prevents operations from timing
out by ensuring the ready interrupt is not ignored.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 654f6420da7ab..837b1830f2e4e 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -120,8 +120,10 @@ static int cc2_enable(struct cc2_data *data)
>  	int ret;
>  
>  	/* exclusive regulator, check in case a disable failed */
> -	if (regulator_is_enabled(data->regulator))
> +	if (regulator_is_enabled(data->regulator)) {
> +		data->process_irqs = true;
>  		return 0;
> +	}

[Severity: Medium]
While this ensures process_irqs is set to true during cc2_enable() for
always-on regulators, does this fully resolve the missed interrupts?

Looking at cc2_disable() in drivers/hwmon/chipcap2.c, process_irqs is
cleared when the driver becomes idle:

drivers/hwmon/chipcap2.c:cc2_disable() {
    ...
	/* ignore alarms triggered by voltage toggling when powering up */
	data->process_irqs = false;
    ...
}

For always-on regulators, the device remains powered and actively
measuring even when the driver is idle. If an alarm threshold is crossed
during this time, the device will assert the edge-triggered alarm pin
(IRQF_TRIGGER_RISING) high, triggering the interrupt.

Since process_irqs is false, the interrupt handler ignores the edge, and
the pin remains high. Subsequent sysfs reads will call cc2_enable() and
set process_irqs to true, but because the pin is already high, no new
edge is generated.

Patch 9 in this series adds synchronize_irq() to wait for running
handlers, but does this handle new edge interrupts arriving after
process_irqs is set to false?

Are edge-triggered alarms permanently lost for always-on devices because
process_irqs was cleared in cc2_disable()?

>  
>  	/* clear any pending completion */
>  	try_wait_for_completion(&data->complete);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=7

[PATCH v3 08/11] hwmon: chipcap2: balance regulator state

[Javier Carrasco]

The current implementation does not guarantee that the calls to
regulator_enable() and regulator_disable() are balanced.
regulator_is_enabled() might return a negative value, and an always-on
attribute in the DT might break the symmetry anyway.

Add a regulator_enabled flag to keep balance within this regulator
consumer.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 837b1830f2e4..952789fdb9db 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -91,6 +91,7 @@ struct cc2_data {
 	int irq_low;
 	int irq_high;
 	bool process_irqs;
+	bool regulator_enabled;
 };
 
 /* %RH as a per cent mille from a register value */
@@ -120,7 +121,7 @@ static int cc2_enable(struct cc2_data *data)
 	int ret;
 
 	/* exclusive regulator, check in case a disable failed */
-	if (regulator_is_enabled(data->regulator)) {
+	if (data->regulator_enabled) {
 		data->process_irqs = true;
 		return 0;
 	}
@@ -132,6 +133,8 @@ static int cc2_enable(struct cc2_data *data)
 	if (ret < 0)
 		return ret;
 
+	data->regulator_enabled = true;
+
 	usleep_range(CC2_STARTUP_TIME_US, CC2_STARTUP_TIME_US + 125);
 
 	data->process_irqs = true;
@@ -147,11 +150,16 @@ static void cc2_disable(struct cc2_data *data)
 	data->process_irqs = false;
 
 	/* exclusive regulator, check in case an enable failed */
-	if (regulator_is_enabled(data->regulator)) {
-		err = regulator_disable(data->regulator);
-		if (err)
-			dev_dbg(&data->client->dev, "Failed to disable device");
+	if (!data->regulator_enabled)
+		return;
+
+	err = regulator_disable(data->regulator);
+	if (err) {
+		dev_dbg(&data->client->dev, "Failed to disable device");
+		return;
 	}
+
+	data->regulator_enabled = false;
 }
 
 static void cc2_disable_action(void *data)

-- 
2.43.0

Re: [PATCH v3 08/11] hwmon: chipcap2: balance regulator state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hwmon: permanent timeout when regulator is always-on
--

commit 5c82eb519f9635342c52ec29776d87d3ed1c197e
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Subject: hwmon: chipcap2: balance regulator state

This commit adds a regulator_enabled flag to track the regulator state
within the driver, ensuring balanced calls to regulator_enable() and
regulator_disable(). This addresses issues where regulator_is_enabled()
might return unexpected values or when the regulator is configured as
always-on in the device tree.

[Severity: High]
Could this cause a permanent timeout when reading measurements if the device
is connected to an always-on regulator?

In drivers/hwmon/chipcap2.c around line 407, cc2_measurement() expects the
device to automatically start a measurement upon being enabled:

cc2_measurement()
    ret = cc2_enable(data);

If the regulator is marked as always-on in the device tree, the earlier
call to cc2_disable() will successfully drop the regulator reference, but it
will not physically remove power from the chip.

When cc2_enable() is called again, it will see data->regulator_enabled is
false, enable the regulator reference, and return successfully. Because the
chip was never physically power-cycled, it will not initiate an automatic
measurement upon power-on reset.

Since the driver does not explicitly send a measurement command in
cc2_measurement(), the device will never produce new data or fire the ready
interrupt. This appears to cause the wait_for_completion_timeout() call in
cc2_read_measurement() (around line 389) to permanently time out:

    ret = wait_for_completion_timeout(&data->complete, timeout);
    if (!ret)
        return -ETIMEDOUT;

Is there a need to explicitly trigger a measurement command if the regulator
was not actually toggled, or to verify if a power-on reset physically
occurred?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=8

[PATCH v3 09/11] hwmon: chipcap2: do not discard pending alarms on disable

[Javier Carrasco]

cc2_disable() stops processing interrupts before the device is powered
down to ignore the alarms caused by voltage toggling. The handler of an
alarm raised by the last measurement might not have run at that point,
and the alarm is discarded when it finally does.

The sysfs read that triggered the measurement can then report no alarm
although the device raised one.

Wait for the alarm handlers that have already been raised before the
interrupt processing is stopped. This covers the case where the handler
thread has not been scheduled yet; an alarm that has not reached the
interrupt core at that point is discarded.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/hwmon/chipcap2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 952789fdb9db..eba6469940ab 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -146,6 +146,13 @@ static void cc2_disable(struct cc2_data *data)
 {
 	int err;
 
+	/* let the alarms already raised by the last measurement be processed */
+	if (data->irq_low > 0)
+		synchronize_irq(data->irq_low);
+
+	if (data->irq_high > 0)
+		synchronize_irq(data->irq_high);
+
 	/* ignore alarms triggered by voltage toggling when powering up */
 	data->process_irqs = false;
 

-- 
2.43.0

Re: [PATCH v3 09/11] hwmon: chipcap2: do not discard pending alarms on disable

[sashiko-bot]

> cc2_disable() stops processing interrupts before the device is powered
> down to ignore the alarms caused by voltage toggling. The handler of an
> alarm raised by the last measurement might not have run at that point,
> and the alarm is discarded when it finally does.
> 
> The sysfs read that triggered the measurement can then report no alarm
> although the device raised one.
> 
> Wait for the alarm handlers that have already been raised before the
> interrupt processing is stopped. This covers the case where the handler
> thread has not been scheduled yet; an alarm that has not reached the
> interrupt core at that point is discarded.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=9

[PATCH v3 10/11] hwmon: chipcap2: serialize access to the alarm indicators

[Javier Carrasco]

low_alarm and high_alarm are accessed from the threaded interrupt
handlers and from sysfs without any locking, which can lead to lost
alarms.

Add a mutex to serialize the access to the alarm indicators and to
process_irqs, which the handlers use to decide whether an interrupt
belongs to an ongoing operation.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Assisted-by: opencode:claude-opus-5
---
 drivers/hwmon/chipcap2.c | 58 ++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index eba6469940ab..b3b725cc7224 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -20,6 +20,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/mutex.h>
 #include <linux/module.h>
 #include <linux/unaligned.h>
 #include <linux/regulator/consumer.h>
@@ -91,6 +92,8 @@ struct cc2_data {
 	int irq_low;
 	int irq_high;
 	bool process_irqs;
+	/* protects process_irqs and the alarm indicators */
+	struct mutex irq_lock;
 	bool regulator_enabled;
 };
 
@@ -122,7 +125,8 @@ static int cc2_enable(struct cc2_data *data)
 
 	/* exclusive regulator, check in case a disable failed */
 	if (data->regulator_enabled) {
-		data->process_irqs = true;
+		scoped_guard(mutex, &data->irq_lock)
+			data->process_irqs = true;
 		return 0;
 	}
 
@@ -137,7 +141,8 @@ static int cc2_enable(struct cc2_data *data)
 
 	usleep_range(CC2_STARTUP_TIME_US, CC2_STARTUP_TIME_US + 125);
 
-	data->process_irqs = true;
+	scoped_guard(mutex, &data->irq_lock)
+		data->process_irqs = true;
 
 	return 0;
 }
@@ -154,7 +159,8 @@ static void cc2_disable(struct cc2_data *data)
 		synchronize_irq(data->irq_high);
 
 	/* ignore alarms triggered by voltage toggling when powering up */
-	data->process_irqs = false;
+	scoped_guard(mutex, &data->irq_lock)
+		data->process_irqs = false;
 
 	/* exclusive regulator, check in case an enable failed */
 	if (!data->regulator_enabled)
@@ -514,10 +520,12 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
 {
 	struct cc2_data *cc2 = data;
 
-	if (cc2->process_irqs) {
-		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
-				   hwmon_humidity_min_alarm, 0);
-		cc2->rh_alarm.low_alarm = true;
+	scoped_guard(mutex, &cc2->irq_lock) {
+		if (cc2->process_irqs) {
+			hwmon_notify_event(cc2->hwmon, hwmon_humidity,
+					   hwmon_humidity_min_alarm, 0);
+			cc2->rh_alarm.low_alarm = true;
+		}
 	}
 
 	return IRQ_HANDLED;
@@ -527,10 +535,12 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
 {
 	struct cc2_data *cc2 = data;
 
-	if (cc2->process_irqs) {
-		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
-				   hwmon_humidity_max_alarm, 0);
-		cc2->rh_alarm.high_alarm = true;
+	scoped_guard(mutex, &cc2->irq_lock) {
+		if (cc2->process_irqs) {
+			hwmon_notify_event(cc2->hwmon, hwmon_humidity,
+					   hwmon_humidity_max_alarm, 0);
+			cc2->rh_alarm.high_alarm = true;
+		}
 	}
 
 	return IRQ_HANDLED;
@@ -546,11 +556,13 @@ static int cc2_humidity_min_alarm_status(struct cc2_data *data, long *val)
 	if (ret < 0)
 		return ret;
 
-	if (data->rh_alarm.low_alarm) {
-		*val = (measurement < min_hyst) ? 1 : 0;
-		data->rh_alarm.low_alarm = *val;
-	} else {
-		*val = 0;
+	scoped_guard(mutex, &data->irq_lock) {
+		if (data->rh_alarm.low_alarm) {
+			*val = (measurement < min_hyst) ? 1 : 0;
+			data->rh_alarm.low_alarm = *val;
+		} else {
+			*val = 0;
+		}
 	}
 
 	return 0;
@@ -566,11 +578,13 @@ static int cc2_humidity_max_alarm_status(struct cc2_data *data, long *val)
 	if (ret < 0)
 		return ret;
 
-	if (data->rh_alarm.high_alarm) {
-		*val = (measurement > max_hyst) ? 1 : 0;
-		data->rh_alarm.high_alarm = *val;
-	} else {
-		*val = 0;
+	scoped_guard(mutex, &data->irq_lock) {
+		if (data->rh_alarm.high_alarm) {
+			*val = (measurement > max_hyst) ? 1 : 0;
+			data->rh_alarm.high_alarm = *val;
+		} else {
+			*val = 0;
+		}
 	}
 
 	return 0;
@@ -753,6 +767,8 @@ static int cc2_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, data);
 
+	mutex_init(&data->irq_lock);
+
 	data->client = client;
 
 	data->regulator = devm_regulator_get_exclusive(dev, "vdd");

-- 
2.43.0

Re: [PATCH v3 10/11] hwmon: chipcap2: serialize access to the alarm indicators

[sashiko-bot]

> low_alarm and high_alarm are accessed from the threaded interrupt
> handlers and from sysfs without any locking, which can lead to lost
> alarms.
> 
> Add a mutex to serialize the access to the alarm indicators and to
> process_irqs, which the handlers use to decide whether an interrupt
> belongs to an ongoing operation.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=10

[PATCH v3 11/11] hwmon: chipcap2: synchronize ready IRQ state

[Javier Carrasco]

The ready IRQ handler can race with reinit_completion() in the enable
path. In particular, a handler from an earlier operation can complete
the completion after it is reset for a new operation.

Use irq_lock to serialize access to the completion and process_irqs.
Disable IRQ processing and synchronize any running handler before resetting
the completion for a new operation.

Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
Cc: stable@vger.kernel.org
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Assisted-by: opencode:claude-opus-5
---
 drivers/hwmon/chipcap2.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index b3b725cc7224..00f62a33d463 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -123,6 +123,16 @@ static int cc2_enable(struct cc2_data *data)
 {
 	int ret;
 
+	if (data->irq_ready > 0) {
+		scoped_guard(mutex, &data->irq_lock)
+			data->process_irqs = false;
+
+		synchronize_irq(data->irq_ready);
+
+		scoped_guard(mutex, &data->irq_lock)
+			reinit_completion(&data->complete);
+	}
+
 	/* exclusive regulator, check in case a disable failed */
 	if (data->regulator_enabled) {
 		scoped_guard(mutex, &data->irq_lock)
@@ -130,9 +140,6 @@ static int cc2_enable(struct cc2_data *data)
 		return 0;
 	}
 
-	/* clear any pending completion */
-	try_wait_for_completion(&data->complete);
-
 	ret = regulator_enable(data->regulator);
 	if (ret < 0)
 		return ret;
@@ -162,6 +169,9 @@ static void cc2_disable(struct cc2_data *data)
 	scoped_guard(mutex, &data->irq_lock)
 		data->process_irqs = false;
 
+	if (data->irq_ready > 0)
+		synchronize_irq(data->irq_ready);
+
 	/* exclusive regulator, check in case an enable failed */
 	if (!data->regulator_enabled)
 		return;
@@ -510,8 +520,10 @@ static irqreturn_t cc2_ready_interrupt(int irq, void *data)
 {
 	struct cc2_data *cc2 = data;
 
-	if (cc2->process_irqs)
-		complete(&cc2->complete);
+	scoped_guard(mutex, &cc2->irq_lock) {
+		if (cc2->process_irqs)
+			complete(&cc2->complete);
+	}
 
 	return IRQ_HANDLED;
 }

-- 
2.43.0

Re: [PATCH v3 11/11] hwmon: chipcap2: synchronize ready IRQ state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwmon: chipcap2: spurious completion due to threaded IRQ scheduling delay
- [Medium] hwmon: chipcap2: spurious alarm IRQs after regulator enable
--

commit 14087cc4e23152ff414e4188a7fe5a671a791a15
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: synchronize ready IRQ state

The ready IRQ handler can race with reinit_completion() in the enable
path. In particular, a handler from an earlier operation can complete
the completion after it is reset for a new operation.

Use irq_lock to serialize access to the completion and process_irqs.
Disable IRQ processing and synchronize any running handler before resetting
the completion for a new operation.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index b3b725cc7224f..00f62a33d4637 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -123,6 +123,16 @@ static int cc2_enable(struct cc2_data *data)
>  {
>  	int ret;
>  
> +	if (data->irq_ready > 0) {
> +		scoped_guard(mutex, &data->irq_lock)
> +			data->process_irqs = false;
> +
> +		synchronize_irq(data->irq_ready);
> +
> +		scoped_guard(mutex, &data->irq_lock)
> +			reinit_completion(&data->complete);
> +	}
> +
>  	/* exclusive regulator, check in case a disable failed */
>  	if (data->regulator_enabled) {
>  		scoped_guard(mutex, &data->irq_lock)
> @@ -130,9 +140,6 @@ static int cc2_enable(struct cc2_data *data)
>  		return 0;
>  	}
>  
> -	/* clear any pending completion */
> -	try_wait_for_completion(&data->complete);
> -
>  	ret = regulator_enable(data->regulator);
>  	if (ret < 0)
>  		return ret;

[Severity: Medium]
If a power-up glitch from regulator_enable() triggers an IRQ, could the
threaded IRQ handlers be delayed by scheduling and run after process_irqs is
set to true further down in this function?

Looking at the rest of cc2_enable():

	data->regulator_enabled = true;

	usleep_range(CC2_STARTUP_TIME_US, CC2_STARTUP_TIME_US + 125);

	scoped_guard(mutex, &data->irq_lock)
		data->process_irqs = true;

Since there is no primary hardirq handler, the IRQ subsystem immediately
queues the threaded handler. If it is delayed by more than ~1.25ms, wouldn't
it observe process_irqs as true and prematurely unblock the measurement wait
by calling complete(&cc2->complete) in cc2_ready_interrupt()?

[Severity: Medium]
Similarly, is it possible for the threaded alarm handlers (irq_low, irq_high)
triggered by regulator toggling to execute after process_irqs is set to true
here?

If they are delayed and observe process_irqs as true, wouldn't they
incorrectly fire an alarm by unconditionally calling hwmon_notify_event() in
cc2_low_interrupt() and cc2_high_interrupt()?

Should there be a synchronize_irq() for the alarm and ready IRQs before
enabling process_irqs?

[ ... ]

> @@ -510,8 +520,10 @@ static irqreturn_t cc2_ready_interrupt(int irq, void *data)
>  {
>  	struct cc2_data *cc2 = data;
>  
> -	if (cc2->process_irqs)
> -		complete(&cc2->complete);
> +	scoped_guard(mutex, &cc2->irq_lock) {
> +		if (cc2->process_irqs)
> +			complete(&cc2->complete);
> +	}
>  
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Since this check happens in thread context rather than hardirq context, could
a delayed thread incorrectly observe the true state here and prematurely
unblock the measurement wait?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-chipcap2_locks-v3-0-27ebfc2c5b9e@gmail.com?part=11