[PATCH v2 0/4] hwmon: chipcap2: various bug fixes

[PATCH v2 0/4] 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.

After sending the fix, Sashiko found new issues[2] that have been added to
the series:
- Wrong channel number in hwmon_notify_event(): pass 0 as the channel
  number.
- 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.
- Enable IRQ processing when the regulator is already enabled.

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 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 (4):
      hwmon: chipcap2: fix channels in humidity alarm notifications
      hwmon: chipcap2: fix IRQ teardown ordering
      hwmon: chipcap2: enable IRQ processing when regulator is already enabled
      hwmon: chipcap2: serialize access to low/high_alarm indicators

 drivers/hwmon/chipcap2.c | 76 ++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)
---
base-commit: 388b607d107c07aaade04c7f22f344cab6bdccd3
change-id: 20260820-chipcap2_locks-c01013a24a0c

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

[PATCH v2 1/4] hwmon: chipcap2: fix channels in humidity alarm notifications

[Javier Carrasco]

hwmon_notify_event() expects the channel number as its last argument,
taken into account with the type parameter that it is a humidity sensor
type. Given that this device only provides one humidity channel, 0 must
be passed. The custom construct to enumerate the channels makes wrong
assumptions by listing all types together (temperature and humidity).

Remove the custom channel enumeration and pass the right channel to
hwmon_notify_event() for hwmon_humidity_min_alarm and
hwmon_humidity_max_alarm.

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

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 086571d556b7..9bef767b589e 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -92,11 +92,6 @@ struct cc2_data {
 	bool process_irqs;
 };
 
-enum cc2_chan_addr {
-	CC2_CHAN_TEMP = 0,
-	CC2_CHAN_HUMIDITY,
-};
-
 /* %RH as a per cent mille from a register value */
 static long cc2_rh_convert(u16 data)
 {
@@ -499,7 +494,7 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
 
 	if (cc2->process_irqs) {
 		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
-				   hwmon_humidity_min_alarm, CC2_CHAN_HUMIDITY);
+				   hwmon_humidity_min_alarm, 0);
 		cc2->rh_alarm.low_alarm = true;
 	}
 
@@ -512,7 +507,7 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
 
 	if (cc2->process_irqs) {
 		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
-				   hwmon_humidity_max_alarm, CC2_CHAN_HUMIDITY);
+				   hwmon_humidity_max_alarm, 0);
 		cc2->rh_alarm.high_alarm = true;
 	}
 

-- 
2.43.0

[PATCH v2 2/4] 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 IRQ handlers are requested afterwards. This ensures
that 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 | 55 ++++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 9bef767b589e..15630d1dd90a 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -631,28 +631,37 @@ 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 void 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 > 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 > 0)
+		data->rh_alarm.low_alarm_visible = true;
+
+	data->irq_high = fwnode_irq_get_byname(dev_fwnode(dev), "high");
+	if (data->irq_high > 0)
+		data->rh_alarm.high_alarm_visible = true;
+}
+
+static int cc2_request_irqs(struct cc2_data *data, struct device *dev)
+{
+	int ret;
+
+	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);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to request ready irq\n");
 	}
 
-	return ret;
-}
-
-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 > 0) {
 		ret = devm_request_threaded_irq(dev, data->irq_low, NULL,
 						cc2_low_interrupt,
@@ -660,12 +669,10 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
 						IRQF_TRIGGER_RISING,
 						dev_name(dev), data);
 		if (ret)
-			return ret;
-
-		data->rh_alarm.low_alarm_visible = true;
+			return dev_err_probe(dev, ret,
+					     "Failed to request low alarm irq\n");
 	}
 
-	data->irq_high = fwnode_irq_get_byname(dev_fwnode(dev), "high");
 	if (data->irq_high > 0) {
 		ret = devm_request_threaded_irq(dev, data->irq_high, NULL,
 						cc2_high_interrupt,
@@ -673,12 +680,11 @@ static int cc2_request_alarm_irqs(struct cc2_data *data, struct device *dev)
 						IRQF_TRIGGER_RISING,
 						dev_name(dev), data);
 		if (ret)
-			return ret;
-
-		data->rh_alarm.high_alarm_visible = true;
+			return dev_err_probe(dev, ret,
+					     "Failed to request high alarm irq\n");
 	}
 
-	return ret;
+	return 0;
 }
 
 static const struct hwmon_channel_info *cc2_info[] = {
@@ -706,7 +712,6 @@ static int cc2_probe(struct i2c_client *client)
 {
 	struct cc2_data *data;
 	struct device *dev = &client->dev;
-	int ret;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -EOPNOTSUPP;
@@ -726,13 +731,7 @@ static int cc2_probe(struct i2c_client *client)
 
 	device_property_read_string(dev, "label", &data->label);
 
-	ret = cc2_request_ready_irq(data, dev);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to request ready irq\n");
-
-	ret = cc2_request_alarm_irqs(data, dev);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to request alarm irqs\n");
+	cc2_get_irqs(data, dev);
 
 	data->hwmon = devm_hwmon_device_register_with_info(dev, client->name,
 							   data, &cc2_chip_info,
@@ -741,7 +740,7 @@ static int cc2_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(data->hwmon),
 				     "Failed to register hwmon device\n");
 
-	return 0;
+	return cc2_request_irqs(data, dev);
 }
 
 static void cc2_remove(struct i2c_client *client)

-- 
2.43.0

[PATCH v2 3/4] 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 15630d1dd90a..57d22d65612c 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -119,8 +119,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

[PATCH v2 4/4] hwmon: chipcap2: serialize access to low/high_alarm indicators

[Javier Carrasco]

Access to low_alarm and high_alarm from the threaded interrupt handlers
and sysfs is not protected by any locking mechanism at the moment, which
can lead to missed events.

Use the hwmon subsystem lock to serialize access to the alarm indicators
and update their state before generating the corresponding event to keep
a consistent state.

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 | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
index 57d22d65612c..0a56c55e9394 100644
--- a/drivers/hwmon/chipcap2.c
+++ b/drivers/hwmon/chipcap2.c
@@ -495,9 +495,11 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
 	struct cc2_data *cc2 = data;
 
 	if (cc2->process_irqs) {
+		scoped_guard(hwmon_lock, cc2->hwmon);
+			cc2->rh_alarm.low_alarm = true;
+
 		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
 				   hwmon_humidity_min_alarm, 0);
-		cc2->rh_alarm.low_alarm = true;
 	}
 
 	return IRQ_HANDLED;
@@ -508,9 +510,11 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
 	struct cc2_data *cc2 = data;
 
 	if (cc2->process_irqs) {
+		scoped_guard(hwmon_lock, cc2->hwmon);
+			cc2->rh_alarm.high_alarm = true;
+
 		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
 				   hwmon_humidity_max_alarm, 0);
-		cc2->rh_alarm.high_alarm = true;
 	}
 
 	return IRQ_HANDLED;

-- 
2.43.0

Re: [PATCH v2 4/4] hwmon: chipcap2: serialize access to low/high_alarm indicators

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `scoped_guard` macro is improperly terminated with a trailing semicolon, resulting in an empty critical section that leaves the alarm state updates unprotected.
--

--- Patch [4]: [PATCH v2 4/4] hwmon: chipcap2: serialize access to low/high_alarm indicators ---
commit a0d28bd8208a1d69e597290c8ebb0fdf0731e68b
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: serialize access to low/high_alarm indicators

This commit attempts to serialize concurrent access to the hardware monitoring
alarm indicators. It introduces scoped_guard usage within the threaded
interrupt handlers to protect the low and high alarm state updates.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 57d22d65612c2..0a56c55e93942 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
[ ... ]
> @@ -495,9 +495,11 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
>  	struct cc2_data *cc2 = data;
>  
>  	if (cc2->process_irqs) {
> +		scoped_guard(hwmon_lock, cc2->hwmon);

[Severity: High]
Does this trailing semicolon create an empty critical section?

Because scoped_guard expands to a for loop, placing a semicolon immediately
after it makes the loop body empty. This means the lock is acquired and
immediately released.

The subsequent assignment to cc2->rh_alarm.low_alarm executes entirely outside
the lock, leaving the data race unmitigated. Should the assignment be wrapped
in a compound block without the trailing semicolon?

> +			cc2->rh_alarm.low_alarm = true;
> +
>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>  				   hwmon_humidity_min_alarm, 0);
> -		cc2->rh_alarm.low_alarm = true;
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -508,9 +510,11 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
>  	struct cc2_data *cc2 = data;
>  
>  	if (cc2->process_irqs) {
> +		scoped_guard(hwmon_lock, cc2->hwmon);

[Severity: High]
Does the same empty critical section issue apply here for the high_alarm
update?

> +			cc2->rh_alarm.high_alarm = true;
> +
>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>  				   hwmon_humidity_max_alarm, 0);
> -		cc2->rh_alarm.high_alarm = true;
>  	}
>  
>  	return IRQ_HANDLED;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-chipcap2_locks-v2-0-6a26c8e9e2fc@gmail.com?part=4

Re: [PATCH v2 1/4] hwmon: chipcap2: fix channels in humidity alarm notifications

[sashiko-bot]

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

Pre-existing issues:
- [High] Unaligned memory access when casting byte arrays to `__be16 *`
- [High] Incorrect endianness conversion for SMBus word write breaks on Big-Endian architectures
--

--- Patch [1]: [PATCH v2 1/4] hwmon: chipcap2: fix channels in humidity alarm notifications ---
commit e05e5dc803c6cf895cd85ba5388bb75fbd791d04
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

hwmon: chipcap2: fix channels in humidity alarm notifications

hwmon_notify_event() expects the channel number as its last argument,
taken into account with the type parameter that it is a humidity sensor
type. Given that this device only provides one humidity channel, 0 must
be passed. The custom construct to enumerate the channels makes wrong
assumptions by listing all types together (temperature and humidity).

Remove the custom channel enumeration and pass the right channel to
hwmon_notify_event() for hwmon_humidity_min_alarm and
hwmon_humidity_max_alarm.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 086571d556b7e..9bef767b589ee 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -92,11 +92,6 @@ struct cc2_data {
>  	bool process_irqs;
>  };
>  
> -enum cc2_chan_addr {
> -	CC2_CHAN_TEMP = 0,
> -	CC2_CHAN_HUMIDITY,
> -};
> -
>  /* %RH as a per cent mille from a register value */
>  static long cc2_rh_convert(u16 data)
>  {
[ ... ]
> @@ -499,7 +494,7 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
>  
>  	if (cc2->process_irqs) {
>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
> -				   hwmon_humidity_min_alarm, CC2_CHAN_HUMIDITY);
> +				   hwmon_humidity_min_alarm, 0);
>  		cc2->rh_alarm.low_alarm = true;
>  	}
>  
> @@ -512,7 +507,7 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
>  
>  	if (cc2->process_irqs) {
>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
> -				   hwmon_humidity_max_alarm, CC2_CHAN_HUMIDITY);
> +				   hwmon_humidity_max_alarm, 0);
>  		cc2->rh_alarm.high_alarm = true;
>  	}

While reviewing this patch, I noticed a couple of pre-existing issues in
drivers/hwmon/chipcap2.c that you might want to look into.

drivers/hwmon/chipcap2.c:cc2_write_reg() {
...
	cpu_to_be16s(&val);
	ret = i2c_smbus_write_word_data(data->client, reg, val);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this endianness conversion
work correctly on Big-Endian architectures?

The SMBus protocol always transmits the least-significant byte first.
On a Little-Endian CPU, cpu_to_be16s() correctly swaps the bytes in memory,
causing SMBus to send the high byte first, matching the Big-Endian device's
expectations.

However, on a Big-Endian CPU, cpu_to_be16s() is a no-op, so the SMBus core
extracts the actual least-significant byte and sends it first, resulting in
Little-Endian transmission over the wire. Should this use
i2c_smbus_write_word_swapped() instead to ensure writes to device
configuration aren't corrupted?


drivers/hwmon/chipcap2.c:cc2_read_reg() {
...
	ret = i2c_master_recv(data->client, buf, CC2_EEPROM_DATA_LEN);
	if (ret != CC2_EEPROM_DATA_LEN)
		return ret < 0 ? ret : -EIO;

	*val = be16_to_cpup((__be16 *)&buf[1]);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does casting the unaligned
address &buf[1] to a 16-bit pointer cause an unaligned memory dereference?

Dereferencing an unaligned pointer using be16_to_cpup() can cause an
alignment fault and kernel oops on architectures that strictly enforce
memory alignment (e.g., SPARC or older ARM). This also happens in
cc2_data_fetch() when casting &data[2] to a 16-bit pointer:

drivers/hwmon/chipcap2.c:cc2_data_fetch() {
...
	switch (type) {
	case hwmon_humidity:
		*val = cc2_rh_convert(be16_to_cpup((__be16 *)&data[0]));
		break;
	case hwmon_temp:
		*val = cc2_temp_convert(be16_to_cpup((__be16 *)&data[2]));
...
}

Is there a safer way to parse these bytes without casting potentially
unaligned byte array addresses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-chipcap2_locks-v2-0-6a26c8e9e2fc@gmail.com?part=1

Re: [PATCH v2 3/4] hwmon: chipcap2: enable IRQ processing when regulator is already enabled

[sashiko-bot]

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

Pre-existing issues:
- [High] Bypassing `regulator_enable()` but calling `regulator_disable()` leads to unbalanced operations and kernel WARNs.
- [High] Incorrect endianness conversion for I2C SMBus word write corrupts data on Big Endian architectures.
- [Medium] `regulator_is_enabled()` can return negative error codes, which are improperly evaluated as true.
- [Medium] The early return path skips clearing pending completions, potentially causing stale reads.
- [Medium] Alarm configuration (command mode) fails unconditionally with timeouts if the regulator is always-on.
- [Low] `try_wait_for_completion()` is improperly used to reset the completion structure.
--

--- Patch [3]: [PATCH v2 3/4] hwmon: chipcap2: enable IRQ processing when regulator is already enabled ---
commit d7e6228cb08396bc9407928fe3b3e28aa4aaf084
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

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

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.

> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -119,8 +119,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)) {

[Severity: Medium]
This is a pre-existing issue, but regulator_is_enabled() can return negative
error codes. Does this condition in cc2_enable() evaluate to true for negative
error codes like -EINVAL, incorrectly bypassing the enable sequence?

Similarly, in cc2_disable(), this check incorrectly tries to disable the
regulator if its state check failed.

[Severity: High]
This is a pre-existing issue, but returning early here bypasses taking a
software reference via regulator_enable() if the regulator is physically
enabled.

If the regulator is physically always-on (e.g., fixed-regulator),
regulator_is_enabled() returns 1, and cc2_enable() skips regulator_enable().
Then cc2_disable() calls regulator_disable() without a matching enable,
triggering a kernel WARN because the consumer's enable_count is 0.

Should this code ensure regulator_enable() is always called to maintain
balance?

[Severity: Medium]
This is a pre-existing issue, but does the command mode retry logic in
cc2_command_mode_start() fundamentally assume regulator_disable() guarantees a
physical power cycle?

cc2_command_mode_start() calls cc2_disable() followed by an msleep() to power
cycle the device. If the regulator is always-on, power is never physically
removed, the 10ms startup window is permanently missed, and the START_CM
command is ignored, causing alarm configuration to fail with -ETIMEDOUT.

> +		data->process_irqs = true;
>  		return 0;

[Severity: Medium]
This is a pre-existing issue, but does this early return skip clearing the
pending completion?

If a late interrupt from a prior timeout leaves data->complete.done > 0, and
the regulator is already enabled, the early return in cc2_enable() bypasses
the reset.

Subsequently, the wait in cc2_read_measurement():

ret = wait_for_completion_timeout(&data->complete, timeout);

will succeed immediately, potentially fetching stale data prematurely before
the sensor finishes its measurement, returning -EBUSY.

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

[Severity: Low]
This is a pre-existing issue, but is try_wait_for_completion() improperly used
to reset the completion structure?

try_wait_for_completion() only decrements the done counter by 1. If multiple
interrupts occurred, done might be > 1, so the next wait could succeed
immediately.

Should this use reinit_completion() instead?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the combination of cpu_to_be16s() and
i2c_smbus_write_word_data() in cc2_write_reg() write swapped byte sequences on
Big Endian platforms?

On Big Endian systems, cpu_to_be16s() is a no-op. i2c_smbus_write_word_data()
mathematically sends the least significant 8 bits first. Thus, on Big Endian,
the original LSB is sent first, whereas the sensor expects MSB first,
which corrupts the alarm threshold configuration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-chipcap2_locks-v2-0-6a26c8e9e2fc@gmail.com?part=3

Re: [PATCH v2 2/4] hwmon: chipcap2: fix IRQ teardown ordering

[sashiko-bot]

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

New issues:
- [Medium] Registering the hwmon device before requesting IRQs exposes the device to userspace before its resources are fully initialized, introducing a probe race.

Pre-existing issues:
- [High] Incorrect endianness conversion on big-endian hosts for SMBus word writes results in writing LSB first instead of MSB first.
- [High] Dereferencing a potentially unaligned byte array pointer using `be16_to_cpup()` can cause kernel panics on architectures with strict memory alignment requirements.
--

--- Patch [2]: [PATCH v2 2/4] hwmon: chipcap2: fix IRQ teardown ordering ---
Note: The standard review format has been altered and direct code quotes omitted to comply with recitation restrictions.

commit 28a819427451e0b54e63443c10cc5e1979c146bb
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
hwmon: chipcap2: fix IRQ teardown ordering

This commit separates IRQ discovery from IRQ registration in the chipcap2
driver. It ensures IRQs are requested after the hwmon device is registered to
fix devres teardown ordering issues.

[Severity: Medium]
In cc2_probe(), the hwmon device is now registered using
devm_hwmon_device_register_with_info() before cc2_request_irqs() is called.
Does exposing the sysfs attributes before the IRQs are requested create a probe
race?

If userspace applications interact with the sysfs interface immediately after
registration, could the read fail with a -ETIMEDOUT error because the device
is waiting for an IRQ handler that has not yet been registered?

[Severity: High]
This is a pre-existing issue, but looking at cc2_write_reg(), the code calls
cpu_to_be16s() on the value before passing it to i2c_smbus_write_word_data().
Since the SMBus API internally expects a host-endian value and always transmits
the least significant byte first, does this logic cause big-endian hosts to
transmit the bytes backwards instead of most significant byte first?

[Severity: High]
This isn't a bug introduced by this patch, but in cc2_read_reg() and
cc2_data_fetch(), byte array pointers at odd offsets such as &buf[1] or stack
arrays are directly cast to __be16 * and passed to be16_to_cpup(). 

Can these unaligned dereferences trigger kernel panics on CPU architectures
with strict memory alignment requirements? Would it be safer to use
get_unaligned_be16() to read these values?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-chipcap2_locks-v2-0-6a26c8e9e2fc@gmail.com?part=2

Re: [PATCH v2 4/4] hwmon: chipcap2: serialize access to low/high_alarm indicators

[Javier Carrasco]

On Sun Aug 23, 2026 at 8:06 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `scoped_guard` macro is improperly terminated with a trailing semicolon, resulting in an empty critical section that leaves the alarm state updates unprotected.
> --
>
> --- Patch [4]: [PATCH v2 4/4] hwmon: chipcap2: serialize access to low/high_alarm indicators ---
> commit a0d28bd8208a1d69e597290c8ebb0fdf0731e68b
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> hwmon: chipcap2: serialize access to low/high_alarm indicators
>
> This commit attempts to serialize concurrent access to the hardware monitoring
> alarm indicators. It introduces scoped_guard usage within the threaded
> interrupt handlers to protect the low and high alarm state updates.
>
>> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
>> index 57d22d65612c2..0a56c55e93942 100644
>> --- a/drivers/hwmon/chipcap2.c
>> +++ b/drivers/hwmon/chipcap2.c
> [ ... ]
>> @@ -495,9 +495,11 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
>>  	struct cc2_data *cc2 = data;
>>
>>  	if (cc2->process_irqs) {
>> +		scoped_guard(hwmon_lock, cc2->hwmon);
>
> [Severity: High]
> Does this trailing semicolon create an empty critical section?
>
> Because scoped_guard expands to a for loop, placing a semicolon immediately
> after it makes the loop body empty. This means the lock is acquired and
> immediately released.
>
> The subsequent assignment to cc2->rh_alarm.low_alarm executes entirely outside
> the lock, leaving the data race unmitigated. Should the assignment be wrapped
> in a compound block without the trailing semicolon?
>

This feedback is correct. The trailing semicolon is for guard() and not
scoped_guard(). I will fix that for V3.

>> +			cc2->rh_alarm.low_alarm = true;
>> +
>>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>>  				   hwmon_humidity_min_alarm, 0);
>> -		cc2->rh_alarm.low_alarm = true;
>>  	}
>>
>>  	return IRQ_HANDLED;
>> @@ -508,9 +510,11 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
>>  	struct cc2_data *cc2 = data;
>>
>>  	if (cc2->process_irqs) {
>> +		scoped_guard(hwmon_lock, cc2->hwmon);
>
> [Severity: High]
> Does the same empty critical section issue apply here for the high_alarm
> update?
>

Same here.

>> +			cc2->rh_alarm.high_alarm = true;
>> +
>>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>>  				   hwmon_humidity_max_alarm, 0);
>> -		cc2->rh_alarm.high_alarm = true;
>>  	}
>>
>>  	return IRQ_HANDLED;

Best regards,
Javier

Re: [PATCH v2 1/4] hwmon: chipcap2: fix channels in humidity alarm notifications

[Javier Carrasco]

On Sun Aug 23, 2026 at 8:11 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Unaligned memory access when casting byte arrays to `__be16 *`
> - [High] Incorrect endianness conversion for SMBus word write breaks on Big-Endian architectures
> --
>
> --- Patch [1]: [PATCH v2 1/4] hwmon: chipcap2: fix channels in humidity alarm notifications ---
> commit e05e5dc803c6cf895cd85ba5388bb75fbd791d04
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> hwmon: chipcap2: fix channels in humidity alarm notifications
>
> hwmon_notify_event() expects the channel number as its last argument,
> taken into account with the type parameter that it is a humidity sensor
> type. Given that this device only provides one humidity channel, 0 must
> be passed. The custom construct to enumerate the channels makes wrong
> assumptions by listing all types together (temperature and humidity).
>
> Remove the custom channel enumeration and pass the right channel to
> hwmon_notify_event() for hwmon_humidity_min_alarm and
> hwmon_humidity_max_alarm.
>
>> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
>> index 086571d556b7e..9bef767b589ee 100644
>> --- a/drivers/hwmon/chipcap2.c
>> +++ b/drivers/hwmon/chipcap2.c
>> @@ -92,11 +92,6 @@ struct cc2_data {
>>  	bool process_irqs;
>>  };
>>
>> -enum cc2_chan_addr {
>> -	CC2_CHAN_TEMP = 0,
>> -	CC2_CHAN_HUMIDITY,
>> -};
>> -
>>  /* %RH as a per cent mille from a register value */
>>  static long cc2_rh_convert(u16 data)
>>  {
> [ ... ]
>> @@ -499,7 +494,7 @@ static irqreturn_t cc2_low_interrupt(int irq, void *data)
>>
>>  	if (cc2->process_irqs) {
>>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>> -				   hwmon_humidity_min_alarm, CC2_CHAN_HUMIDITY);
>> +				   hwmon_humidity_min_alarm, 0);
>>  		cc2->rh_alarm.low_alarm = true;
>>  	}
>>
>> @@ -512,7 +507,7 @@ static irqreturn_t cc2_high_interrupt(int irq, void *data)
>>
>>  	if (cc2->process_irqs) {
>>  		hwmon_notify_event(cc2->hwmon, hwmon_humidity,
>> -				   hwmon_humidity_max_alarm, CC2_CHAN_HUMIDITY);
>> +				   hwmon_humidity_max_alarm, 0);
>>  		cc2->rh_alarm.high_alarm = true;
>>  	}
>
> While reviewing this patch, I noticed a couple of pre-existing issues in
> drivers/hwmon/chipcap2.c that you might want to look into.
>
> drivers/hwmon/chipcap2.c:cc2_write_reg() {
> ...
> 	cpu_to_be16s(&val);
> 	ret = i2c_smbus_write_word_data(data->client, reg, val);
> ...
> }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this endianness conversion
> work correctly on Big-Endian architectures?
>
> The SMBus protocol always transmits the least-significant byte first.
> On a Little-Endian CPU, cpu_to_be16s() correctly swaps the bytes in memory,
> causing SMBus to send the high byte first, matching the Big-Endian device's
> expectations.
>
> However, on a Big-Endian CPU, cpu_to_be16s() is a no-op, so the SMBus core
> extracts the actual least-significant byte and sends it first, resulting in
> Little-Endian transmission over the wire. Should this use
> i2c_smbus_write_word_swapped() instead to ensure writes to device
> configuration aren't corrupted?
>
>
> drivers/hwmon/chipcap2.c:cc2_read_reg() {
> ...
> 	ret = i2c_master_recv(data->client, buf, CC2_EEPROM_DATA_LEN);
> 	if (ret != CC2_EEPROM_DATA_LEN)
> 		return ret < 0 ? ret : -EIO;
>
> 	*val = be16_to_cpup((__be16 *)&buf[1]);
> ...
> }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does casting the unaligned
> address &buf[1] to a 16-bit pointer cause an unaligned memory dereference?
>
> Dereferencing an unaligned pointer using be16_to_cpup() can cause an
> alignment fault and kernel oops on architectures that strictly enforce
> memory alignment (e.g., SPARC or older ARM). This also happens in
> cc2_data_fetch() when casting &data[2] to a 16-bit pointer:
>
> drivers/hwmon/chipcap2.c:cc2_data_fetch() {
> ...
> 	switch (type) {
> 	case hwmon_humidity:
> 		*val = cc2_rh_convert(be16_to_cpup((__be16 *)&data[0]));
> 		break;
> 	case hwmon_temp:
> 		*val = cc2_temp_convert(be16_to_cpup((__be16 *)&data[2]));
> ...
> }
>
> Is there a safer way to parse these bytes without casting potentially
> unaligned byte array addresses?

I will take a look at this and send another patch if it applies.

Best regards,
Javier

Re: [PATCH v2 2/4] hwmon: chipcap2: fix IRQ teardown ordering

[Javier Carrasco]

On Sun Aug 23, 2026 at 8:12 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Medium] Registering the hwmon device before requesting IRQs exposes the device to userspace before its resources are fully initialized, introducing a probe race.
>
> Pre-existing issues:
> - [High] Incorrect endianness conversion on big-endian hosts for SMBus word writes results in writing LSB first instead of MSB first.
> - [High] Dereferencing a potentially unaligned byte array pointer using `be16_to_cpup()` can cause kernel panics on architectures with strict memory alignment requirements.
> --
>
> --- Patch [2]: [PATCH v2 2/4] hwmon: chipcap2: fix IRQ teardown ordering ---
> Note: The standard review format has been altered and direct code quotes omitted to comply with recitation restrictions.
>
> commit 28a819427451e0b54e63443c10cc5e1979c146bb
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> hwmon: chipcap2: fix IRQ teardown ordering
>
> This commit separates IRQ discovery from IRQ registration in the chipcap2
> driver. It ensures IRQs are requested after the hwmon device is registered to
> fix devres teardown ordering issues.
>
> [Severity: Medium]
> In cc2_probe(), the hwmon device is now registered using
> devm_hwmon_device_register_with_info() before cc2_request_irqs() is called.
> Does exposing the sysfs attributes before the IRQs are requested create a probe
> race?
>
> If userspace applications interact with the sysfs interface immediately after
> registration, could the read fail with a -ETIMEDOUT error because the device
> is waiting for an IRQ handler that has not yet been registered?
>

I am not sure what to do with this, as this patch was actually meant to
avoid the opposite issue (IRQs still available when the hwmon device is
not) which is worse because access to freed resources might arise.

> [Severity: High]
> This is a pre-existing issue, but looking at cc2_write_reg(), the code calls
> cpu_to_be16s() on the value before passing it to i2c_smbus_write_word_data().
> Since the SMBus API internally expects a host-endian value and always transmits
> the least significant byte first, does this logic cause big-endian hosts to
> transmit the bytes backwards instead of most significant byte first?
>

This issue was already mentioned in Sashiko's feedback for [1/4] and as
I said, I will take a look a it and send a new patch if it applies.

> [Severity: High]
> This isn't a bug introduced by this patch, but in cc2_read_reg() and
> cc2_data_fetch(), byte array pointers at odd offsets such as &buf[1] or stack
> arrays are directly cast to __be16 * and passed to be16_to_cpup().
>
> Can these unaligned dereferences trigger kernel panics on CPU architectures
> with strict memory alignment requirements? Would it be safer to use
> get_unaligned_be16() to read these values?

Same here.

Best regards,
Javier

Re: [PATCH v2 3/4] hwmon: chipcap2: enable IRQ processing when regulator is already enabled

[Javier Carrasco]

On Sun Aug 23, 2026 at 8:11 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Bypassing `regulator_enable()` but calling `regulator_disable()` leads to unbalanced operations and kernel WARNs.
> - [High] Incorrect endianness conversion for I2C SMBus word write corrupts data on Big Endian architectures.
> - [Medium] `regulator_is_enabled()` can return negative error codes, which are improperly evaluated as true.
> - [Medium] The early return path skips clearing pending completions, potentially causing stale reads.
> - [Medium] Alarm configuration (command mode) fails unconditionally with timeouts if the regulator is always-on.
> - [Low] `try_wait_for_completion()` is improperly used to reset the completion structure.
> --
>
> --- Patch [3]: [PATCH v2 3/4] hwmon: chipcap2: enable IRQ processing when regulator is already enabled ---
> commit d7e6228cb08396bc9407928fe3b3e28aa4aaf084
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> hwmon: chipcap2: enable IRQ processing when regulator is already enabled
>
> 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.
>
>> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
>> --- a/drivers/hwmon/chipcap2.c
>> +++ b/drivers/hwmon/chipcap2.c
>> @@ -119,8 +119,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)) {
>
> [Severity: Medium]
> This is a pre-existing issue, but regulator_is_enabled() can return negative
> error codes. Does this condition in cc2_enable() evaluate to true for negative
> error codes like -EINVAL, incorrectly bypassing the enable sequence?
>
> Similarly, in cc2_disable(), this check incorrectly tries to disable the
> regulator if its state check failed.
>
> [Severity: High]
> This is a pre-existing issue, but returning early here bypasses taking a
> software reference via regulator_enable() if the regulator is physically
> enabled.
>
> If the regulator is physically always-on (e.g., fixed-regulator),
> regulator_is_enabled() returns 1, and cc2_enable() skips regulator_enable().
> Then cc2_disable() calls regulator_disable() without a matching enable,
> triggering a kernel WARN because the consumer's enable_count is 0.
>
> Should this code ensure regulator_enable() is always called to maintain
> balance?
>

I will add this to V3 to keep the balance in all cases. The problem with
setting the always-on attribute to the dedicated regulator that this
device requires deceives its original purpose. The driver could try to
find out in the probe() if disabling the regulator is not possible, and
then skip any operations on the alarms because it won't be possible to
set the associated registers. That would be a major modification that
would require its own series to address a very unlikely scenario. I will
not include that in this series.

> [Severity: Medium]
> This is a pre-existing issue, but does the command mode retry logic in
> cc2_command_mode_start() fundamentally assume regulator_disable() guarantees a
> physical power cycle?
>
> cc2_command_mode_start() calls cc2_disable() followed by an msleep() to power
> cycle the device. If the regulator is always-on, power is never physically
> removed, the 10ms startup window is permanently missed, and the START_CM
> command is ignored, causing alarm configuration to fail with -ETIMEDOUT.
>

This will be fixed as well.

>> +		data->process_irqs = true;
>>  		return 0;
>
> [Severity: Medium]
> This is a pre-existing issue, but does this early return skip clearing the
> pending completion?
>
> If a late interrupt from a prior timeout leaves data->complete.done > 0, and
> the regulator is already enabled, the early return in cc2_enable() bypasses
> the reset.
>
> Subsequently, the wait in cc2_read_measurement():
>
> ret = wait_for_completion_timeout(&data->complete, timeout);
>
> will succeed immediately, potentially fetching stale data prematurely before
> the sensor finishes its measurement, returning -EBUSY.
>
>> +	}
>>
>>  	/* clear any pending completion */
>>  	try_wait_for_completion(&data->complete);
>
> [Severity: Low]
> This is a pre-existing issue, but is try_wait_for_completion() improperly used
> to reset the completion structure?
>
> try_wait_for_completion() only decrements the done counter by 1. If multiple
> interrupts occurred, done might be > 1, so the next wait could succeed
> immediately.
>
> Should this use reinit_completion() instead?
>

I will take a look at this issue alongside the other regulator-related
stuff.

> [ ... ]
>
> [Severity: High]
> This is a pre-existing issue, but does the combination of cpu_to_be16s() and
> i2c_smbus_write_word_data() in cc2_write_reg() write swapped byte sequences on
> Big Endian platforms?
>
> On Big Endian systems, cpu_to_be16s() is a no-op. i2c_smbus_write_word_data()
> mathematically sends the least significant 8 bits first. Thus, on Big Endian,
> the original LSB is sent first, whereas the sensor expects MSB first,
> which corrupts the alarm threshold configuration.

This has been reported before.

Best regards,
Javier

Re: [PATCH v2 2/4] hwmon: chipcap2: fix IRQ teardown ordering

[Javier Carrasco]

On Sun Aug 23, 2026 at 8:22 PM CEST, Javier Carrasco wrote:
> On Sun Aug 23, 2026 at 8:12 PM CEST, sashiko-bot wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>
>> New issues:
>> - [Medium] Registering the hwmon device before requesting IRQs exposes the device to userspace before its resources are fully initialized, introducing a probe race.
>>
>> Pre-existing issues:
>> - [High] Incorrect endianness conversion on big-endian hosts for SMBus word writes results in writing LSB first instead of MSB first.
>> - [High] Dereferencing a potentially unaligned byte array pointer using `be16_to_cpup()` can cause kernel panics on architectures with strict memory alignment requirements.
>> --
>>
>> --- Patch [2]: [PATCH v2 2/4] hwmon: chipcap2: fix IRQ teardown ordering ---
>> Note: The standard review format has been altered and direct code quotes omitted to comply with recitation restrictions.
>>
>> commit 28a819427451e0b54e63443c10cc5e1979c146bb
>> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> hwmon: chipcap2: fix IRQ teardown ordering
>>
>> This commit separates IRQ discovery from IRQ registration in the chipcap2
>> driver. It ensures IRQs are requested after the hwmon device is registered to
>> fix devres teardown ordering issues.
>>
>> [Severity: Medium]
>> In cc2_probe(), the hwmon device is now registered using
>> devm_hwmon_device_register_with_info() before cc2_request_irqs() is called.
>> Does exposing the sysfs attributes before the IRQs are requested create a probe
>> race?
>>
>> If userspace applications interact with the sysfs interface immediately after
>> registration, could the read fail with a -ETIMEDOUT error because the device
>> is waiting for an IRQ handler that has not yet been registered?
>>
>
> I am not sure what to do with this, as this patch was actually meant to
> avoid the opposite issue (IRQs still available when the hwmon device is
> not) which is worse because access to freed resources might arise.
>

Both issues can be fixed at once by registering the data ready IRQ
before the hwmon device because it does not need it.
I will update it for V3.

Best regards,
Javier