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

On 8/21/26 03:16, Javier Carrasco wrote:
> 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.
> 

Please do. Introducing separate locks for low frequency events such as this one
is overkill.

>> [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.
> 
Thanks!

Guenter