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

Some extra homework, but the issues are real and I am open to tackle
them all.

On Fri Aug 21, 2026 at 11:37 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [Low] The driver implements custom locks for interrupt synchronization instead of using the hwmon subsystem's mandated locking API.
>
> Pre-existing issues:
> - [High] The threaded interrupt handlers pass the wrong channel index to hwmon_notify_event(), causing the sysfs notification to fail silently.
> - [High] Threaded IRQs are requested before the hwmon device is fully registered, leading to a Use-After-Free (UAF) during driver teardown.
> - [High] The driver permanently times out on all measurements if the regulator is already enabled at boot or if a previous disable failed.
> --
>
> --- Patch [1]: [PATCH] hwmon: chipcap2: serialize access to low/high_alarm indicators ---
> commit f294324394a3b551fc4139386f0f74282d1fdb24
> Author: Javier Carrasco <[email protected]>
>
> hwmon: chipcap2: serialize access to low/high_alarm indicators
>
> This patch adds per-interrupt mutexes to protect low_alarm and high_alarm
> accesses from threaded interrupt handlers and sysfs, fixing a potential race
> condition that could lead to missed events.
>
> Link: https://lore.kernel.org/linux-hwmon/[email protected]/
>
> Note: This review is provided in a free-form summary format without direct
> inline patch quoting due to recitation restrictions.
>
> [Severity: Low]
> Is it correct to introduce custom per-alarm mutexes for interrupt
> synchronization here? Looking at the hwmon subsystem guidelines, drivers
> should use the mandated locking API via hwmon_lock(dev) and hwmon_unlock(dev)
> instead of defining and using custom locks like low_alarm_lock and
> high_alarm_lock in struct cc2_rh_alarm_info and cc2_low_interrupt().
>

I used two mutexes to decouple the alarms as they are independent, so
actions taken for any of them is not delayed by the other. On the other
hand, that delay will be very short, so I am open to use hwmon_lock(dev)
if it is preferred in this case too.

> [Severity: High]
> This isn't a bug introduced by this patch, but does the threaded interrupt
> handler pass the correct channel index to hwmon_notify_event()?
>
> In cc2_low_interrupt(), hwmon_notify_event() receives CC2_CHAN_HUMIDITY
> (which is 1). The hwmon core adds this to the humidity base index (1),
> looking for humidity2_min_alarm.
>
> However, the driver only registers one humidity channel (index 0), which is
> created as humidity1_min_alarm. Will this cause the sysfs notification to
> fail silently when an alarm fires?
>

This is a real issue. I will remove the enum with the channel definition
because it is only used here and pass 0 instead (there is only one
humidity channel).

> [Severity: High]
> This is a pre-existing issue, but is there a teardown ordering problem with
> devres in cc2_probe()?
>
> The threaded IRQs are requested via cc2_request_ready_irq() before
> devm_hwmon_device_register_with_info() is called. During devres unbind,
> resources are released in LIFO order, meaning the hwmon device is
> unregistered and freed while the IRQs are still active.
>
> Concurrent sysfs reads during unbind could re-enable process_irqs. If an
> interrupt fires during this window, could it dereference the freed
> cc2->hwmon pointer, resulting in a use-after-free?
>

I will analyze this in more detail and get back with the results.

> [Severity: High]
> This isn't a bug introduced by this patch, but does the driver permanently
> time out on measurements if the regulator is already enabled?
>
> In cc2_enable(), if regulator_is_enabled() returns true (e.g., for an
> always-on DT regulator), the function returns 0 immediately without setting
> data->process_irqs = true.
>
> As a result, cc2_ready_interrupt() will ignore all interrupts, and the
> measurement wait will unconditionally time out. Should data->process_irqs
> be set even if the regulator is already enabled?


I will analyze this in more detail and get back with the results.

Best regards,
Javier