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

On 8/21/26 02:22, Javier Carrasco wrote:
> 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.
> 
> Add a per-interrupt mutex as the two alarm indicators are completely
> independent.
> 
> Fixes: 3af350929e75 ("hwmon: Add support for Amphenol ChipCap 2")
> Cc: [email protected]
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> This issue 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.
> 
> The fix has been validated on real hardware with an Amphenol
> ChipCap 2 CC2D23S sensor.
> 

Needs explanation: Why can the hwmon subsystem lock not be used ?

Guenter

> Link: [1] https://lore.kernel.org/linux-hwmon/[email protected]/
> ---
>   drivers/hwmon/chipcap2.c | 38 ++++++++++++++++++++++++++++----------
>   1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/chipcap2.c b/drivers/hwmon/chipcap2.c
> index 086571d556b7..1bb4ec96514e 100644
> --- a/drivers/hwmon/chipcap2.c
> +++ b/drivers/hwmon/chipcap2.c
> @@ -73,7 +73,11 @@
>   
>   struct cc2_rh_alarm_info {
>   	bool low_alarm;
> +	/* Serialize accesses to low_alarm from threaded IRQ and sysfs */
> +	struct mutex low_alarm_lock;
>   	bool high_alarm;
> +	/* Serialize accesses to high_alarm from threaded IRQ and sysfs */
> +	struct mutex high_alarm_lock;
>   	bool low_alarm_visible;
>   	bool high_alarm_visible;
>   };
> @@ -500,6 +504,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);
> +		guard(mutex)(&cc2->rh_alarm.low_alarm_lock);
>   		cc2->rh_alarm.low_alarm = true;
>   	}
>   
> @@ -513,6 +518,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);
> +		guard(mutex)(&cc2->rh_alarm.high_alarm_lock);
>   		cc2->rh_alarm.high_alarm = true;
>   	}
>   
> @@ -529,11 +535,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->rh_alarm.low_alarm_lock) {
> +		if (data->rh_alarm.low_alarm) {
> +			*val = (measurement < min_hyst) ? 1 : 0;
> +			data->rh_alarm.low_alarm = *val;
> +		} else {
> +			*val = 0;
> +		}
>   	}
>   
>   	return 0;
> @@ -549,11 +557,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->rh_alarm.high_alarm_lock) {
> +		if (data->rh_alarm.high_alarm) {
> +			*val = (measurement > max_hyst) ? 1 : 0;
> +			data->rh_alarm.high_alarm = *val;
> +		} else {
> +			*val = 0;
> +		}
>   	}
>   
>   	return 0;
> @@ -720,6 +730,14 @@ static int cc2_probe(struct i2c_client *client)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	ret = devm_mutex_init(dev, &data->rh_alarm.low_alarm_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mutex_init(dev, &data->rh_alarm.high_alarm_lock);
> +	if (ret)
> +		return ret;
> +
>   	i2c_set_clientdata(client, data);
>   
>   	data->client = client;
> 
> ---
> base-commit: 818bebeb63dd6bf5f4e07e145f6cdbace520a34c
> change-id: 20260820-chipcap2_locks-c01013a24a0c
> 
> Best regards,