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

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().

[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?

[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?

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1