Re: [PATCH] hwmon: add driver for ARCTIC Fan Controller

[Thomas Weißschuh]

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.

(...)

> > > +     } 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?

(...)

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

> > > +
> > > +     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