Re: [PATCH] hwmon: add driver for ARCTIC Fan Controller
From: Guenter Roeck
Date: Tue Mar 03 2026 - 15:42:24 EST
On 3/3/26 11:30, Thomas Weißschuh wrote:
On 2026-03-03 11:00:25-0800, Guenter Roeck wrote:
On 3/3/26 10:10, Thomas Weißschuh wrote:
Hi!
On 2026-03-03 08:25:04+0000, Aureo Serrano wrote:
From 1cc962124ca4343e682219372b08dec5d611d1af Mon Sep 17 00:00:00 2001
From: Aureo Serrano de Souza <aureo.serrano@xxxxxxxxx>
Date: Tue, 3 Mar 2026 15:06:35 +0800
Subject: [PATCH] hwmon: add driver for ARCTIC Fan Controller
Add hwmon driver for the ARCTIC Fan Controller (USB HID VID 0x3904,
PID 0xF001) with 10 fan channels. Exposes fan RPM and PWM via sysfs.
Device pushes IN reports ~1 Hz; PWM set via OUT reports.
Signed-off-by: Aureo Serrano de Souza <aureo.serrano@xxxxxxxxx>
---
checkpatch reports:
total: 11 errors, 53 warnings, 6 checks, 360 lines checked
primarily because the patch uses spaces instead of tabs.
It looks like it was pushed through some Microsoft mail product with
copious amounts of force.
(...)
+ }
+ for (i = 0; i < ARCTIC_NUM_FANS; i++) {
+ priv->fan_rpm[i] = (u32)(buf[rpm_off + i * 2] |
+ (buf[rpm_off + i * 2 + 1] << 8));
get_unaligned_u32()
That doesn't seem to exist. get_unaligned_le16(), maybe, but the data
is never unaligned. le16_to_cpup() might do.
Indeed, get_unaligned_le16() is the one.
Does the HID core guarantee that raw event buffers are always aligned
sufficiently to access them as *u32? Personally I don't know all the
alignment requirements of all the supported architectures.
get_unaligned_le16() will always do the right thing and avoids typecasts.
Good point.
(...)
+ } 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.
(...)
+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).
+
+ priv->hdev = hdev;
+ spin_lock_init(&priv->lock);
+ hid_set_drvdata(hdev, priv);
+
+ ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
+ if (ret)
+ return ret;
+
+ ret = hid_hw_open(hdev);
+ if (ret)
+ goto out_stop;
+
+ hid_device_io_start(hdev);
+
+ hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "arctic_fan",
+ priv, &arctic_fan_chip_info,
+ NULL);
You could assign directly to priv->hwmon_dev.
I don't immediately see where priv->hwmon_dev is used in the first place.
Indeed.
Thomas