Re: [PATCH 1/2] dt-bindings: hwmon: chipcap2: Add label property

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The 'label' property description incorrectly refers to 'this channel', but the property is applied to the top-level device node.

Pre-existing issues:
- [High] Threaded interrupt handlers modify shared state without holding any locks, violating hwmon subsystem guidelines and causing potential loss of alarm events.
--

commit 6b42d122bc193119c40ff5ee9be18fcec09fc567
Author: Flaviu Nistor <flaviu.nistor@gmail.com>

dt-bindings: hwmon: chipcap2: Add label property

Add support for an optional label property similar to other hwmon devices.
This allows, in case of boards with multiple CHIPCAP2 sensors, to assign
distinct names to each instance.

> diff --git a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
> index 17351fdbefce6..f00b5a4b14ddd 100644
> --- a/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/amphenol,chipcap2.yaml
> @@ -33,6 +33,10 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  label:
> +    description:
> +      A descriptive name for this channel, like "ambient" or "psu".

[Severity: Low]
Is this description accurate? The label property is being added to the
root node, so it applies to the entire device/instance rather than a single
channel.

The chipcap2 sensor provides multiple channels via a single device
node. Could the description be updated to clarify it names the entire sensor
rather than a single channel?

> +
>    interrupts:
>      items:
>        - description: measurement ready indicator

[Severity: High]
This is a pre-existing issue, but does drivers/hwmon/chipcap2.c lack
locking between the threaded interrupt handlers and the sysfs paths, which
could lead to lost alarm events?

The threaded interrupt handlers modify shared state without holding any
locks, which violates hwmon subsystem guidelines.

If an interrupt fires between the sysfs read and write:

CPU1 in drivers/hwmon/chipcap2.c:cc2_humidity_min_alarm_status() {
	...
	if (data->rh_alarm.low_alarm) {
		*val = (measurement < min_hyst) ? 1 : 0;

CPU2 in drivers/hwmon/chipcap2.c:cc2_low_interrupt() {
		cc2->rh_alarm.low_alarm = true;
}

CPU1 continues:
		data->rh_alarm.low_alarm = *val;
	} else {
		*val = 0;
	}
	...
}

The interrupt's update will be overwritten with 0 and permanently lost. The
hwmon subsystem guidelines require drivers to implement locking for interrupt
handling and sysfs attributes, recommending the use of hwmon_lock() and
hwmon_unlock().

Could this concurrent access to data->rh_alarm.low_alarm be protected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622122200.14245-1-flaviu.nistor@gmail.com?part=1