Re: [PATCH] hwmon: add driver for ARCTIC Fan Controller
From: Guenter Roeck
Date: Fri Mar 06 2026 - 01:04:01 EST
On 3/3/26 13:34, Thomas Weißschuh wrote:
On 2026-03-03 12:42:13-0800, Guenter Roeck wrote:Google's experimental code review agent has a different opinion.
On 3/3/26 11:30, Thomas Weißschuh wrote:(...)
On 2026-03-03 11:00:25-0800, Guenter Roeck wrote:
+ } else {
+ spin_unlock_irqrestore(&priv->lock, flags);
You can use the new guard() syntax from cleanup.h to avoid manual
unlocks on error paths.
Why would this code need interrupt disabled spinlocks in the first place ?
I *suspect* that it tries to be compatible with some semaphores in the
HID core.
It reads individual entries from priv, but even if those are updated
in parallel I don't see why that would warrant disabling interrupts,
both here and in arctic_fan_parse_report().
The hwmon core already serializes read and write operations, so
the locks (much less interrupt disabling spinlocks) are not needed
for that either.
The HID callbacks can be fired at any time from the HID core,
concurrently to hwmon core logic. But I also dislike the spinlocks.
Maybe a mutex works, too?
Ah yes, I can see that arctic_fan_parse_report() reads all pwm values and
arctic_fan_write() writes them to the controller. That does not explain
why it would be necessary to disable interrupts, though, and even doing
that is still not safe.
Example: arctic_fan_write() updates the pwm value for channel 1,
writes the new value into priv->pwm_duty[1], and creates an output
buffer with pwm values for all channels. After preparing the message,
it releases the spinlock. The raw event handler receives and handles
updated pwm values which are completely different. Then the old,
now obsolete, values are sent to the controller (and, worse, the
new cached value in priv->pwm_duty[1] would no longer match the value
that was just sent to the controller).
That can never be made safe if the controller updates pwm values
autonomously, no matter if spinlocks are involved or not. That would only
work if fan control is manual, and in that case it would not be necessary
to re-read pwm values from each raw event. The current code isn't safe
even if fan control is manual, since reports from the controller will
overwrite cached values and requests to change a value can overlap with
reports returning the old value.
In this context ...
Other drivers also use complete() from raw events and wait_for_completion()
variants after writing a command, so the code sequence in arctic_fan_send_report()
will require closer scrutiny. It is not obvious to me why the loop and the
msleep() calls would be needed for this driver but not for others.
Ack.
(...)
+static int arctic_fan_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct arctic_fan_data *priv;
+ struct device *hwmon_dev;
+ int ret;
+
+ ret = hid_parse(hdev);
+ if (ret)
+ return ret;
+
+ priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->out_buf = devm_kmalloc(&hdev->dev, ARCTIC_REPORT_LEN, GFP_KERNEL);
+ if (!priv->out_buf)
+ return -ENOMEM;
The 32 byte buffer could be on the stack, saving this allocation and
avoiding a shared resource.
It might need to be cache aligned, but even then it could be part of
struct arctic_fan_data.
What would be the advantage of that over an on-stack placement?
Sorry, I should have said "cache line aligned", not just "cache aligned".
Data on the stack won't be cache line aligned. I don't know if that is needed
here, but some USB transactions require it (which is why USB drivers often
allocate buffers separately).
We can use __aligned() for stack variables I think.
But with a quick search I didn't find any alignment requirements from
the HID subsystem. So IMO it shouldn't matter much for now.
Here is what it has to say about a similar change suggested
for the Gigabyte Waterforce controller.
Does this change violate the DMA API?
The `buffer` is passed to `hid_hw_output_report()`, which for USB HID devices
will map the buffer for DMA.
Because `buffer` is embedded within `struct waterforce_data` without an explicit
cache-line alignment attribute (like `____cacheline_aligned`), it may share a
cache line with the preceding fields, such as `updated`.
Since `updated` is modified in `waterforce_raw_event()` upon receiving input
reports, concurrently accessing it while an output report DMA transfer is
in progress can cause cache coherency issues and memory corruption on
non-cache-coherent architectures.
I didn't verify this, but it matches my memory. Of course, both my memory
and the AI may be wrong, but at this point I'd like for someone to prove
that it is wrong.
Thanks,
Guenter