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

[Thomas Weißschuh]

On 2026-03-03 12:42:13-0800, Guenter Roeck wrote:
> 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.


Thomas