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

[Guenter Roeck]

On 3/16/26 21:20, Aureo Serrano wrote:
> On 2026-03-13 09:59:56-0700, Guenter Roeck wrote:
> > > +	buf = kmalloc(ARCTIC_REPORT_LEN, GFP_KERNEL);
> > > +	if (!buf)
> > > +		return -ENOMEM;
> >
> > The second problem does not exist since the hwmon core serializes sysfs
> > attribute accesses, and a single once-allocated buffer would be sufficient
> > for the same reason.
>
> Moved the buffer into the struct (u8 buf[ARCTIC_REPORT_LEN]), allocated
> once via devm_kzalloc with the rest of the struct. Per-write kmalloc/kfree
> removed. Does that match what you had in mind?
Yes. Keep in mind that the buffer must be dma aligned.

> > > +	{
> > > +		guard(mutex)(&priv->lock);
> > The { } around the guard() are unnecessary, both here and elsewhere in
> > the code.
> >
> > If the guard() is added is because the call is from an event handler,
> > use hwmon_lock() and hwmon_unlock() to serialize accesses.
>
> Braces removed. arctic_fan_parse_report() now uses hwmon_lock()/
> hwmon_unlock() as suggested.
>
> One consequence worth checking: since read and write callbacks are already
> invoked with the hwmon core lock held, keeping a separate private mutex
> alongside hwmon_lock() would protect the same data with two different locks.
> I removed the private mutex entirely instead of converting it with
> devm_mutex_init(). Is that the right conclusion, or would you prefer keeping
> the mutex?
The separate mutex does not make sense, so ok to remove it.

> > > +	mutex_init(&priv->lock);
> >
> > Use devm_mutex_init().
>
> Addressed as described above.

Thanks,
Guenter