Re: [PATCH] hwmon: (corsair-psu) serialize debugfs access against hwmon

On Mon, 3 Aug 2026 19:54:49 -0700
Guenter Roeck <[email protected]> wrote:

> On 8/3/26 16:56, Ali Ahmet Memis wrote:
> > On Mon, 3 Aug 2026 16:19:57 -0700 Guenter Roeck wrote:
> >> Anyway, I (and Sashiko) think the patch is incomplete. It does not protect
> >> cmd_buffer when handling raw events (while executing corsairpsu_raw_event).
> >> That is a pre-existing issue, though. Not sure if that should be fixed in a
> >> separate patch or with this one. Thoughts ?
> > 
> > Separate, I think, and not really by choice: it cannot use the same lock.
> > corsairpsu_raw_event() is reached from the URB completion handler,
> > hid_irq_in() -> hid_safe_input_report() -> hdrv->raw_event(), and
> > Documentation/driver-api/usb/URB.rst is blunt about that path ("NEVER SLEEP
> > IN A COMPLETION HANDLER"). So whatever protects cmd_buffer there cannot be
> > the hwmon mutex this patch relies on, and has to be its own mechanism rather
> > than an extension of this one.
> > 
> > The window I see is the timeout. corsairpsu_usb_cmd() gives up with
> > -ETIMEDOUT, but the device can still deliver that reply afterwards. The next
> > command calls reinit_completion(), which makes completion_done() false again,
> > so the guard at the top of raw_event no longer rejects the late reply: it
> > memcpys into cmd_buffer and completes the new waiter with the previous
> > command's data. The echo check only catches that when the two commands
> > differ. For two reads of the same command on different rails it does not,
> > which is the same wrong-rail symptom this patch is about, reached the other
> > way round.
> > 
> > The two also blame differently, for whatever that is worth: this patch is
> > 4207069edbf0, while the raw_event side goes back to d115b51e0e56 ("hwmon:
> > add Corsair PSU HID controller driver").
> > 
> > Nobody has written the raw_event one as far as I know, so tell me which way
> > you want it: I can send it, or leave it to you. Same for this patch, if you
> > would rather use the version you already had sitting around.
> > 
> 
> For this patch, I'd rather take yours. I am inclined to take it as-is even
> if Wilken doesn't have time to test it. After all, it won't make the situation
> worse. Wilken, WDYT ?
> 
> Gemini tells me that fixing the raw event problem will require a spinlock to
> protect the completion and a separate receive buffer. No idea if it is correct,
> but other drivers do the same, so it may have a point. Either case, this is a
> bit too much to do without hardware to test, and I'd rather prefer to leave this
> up to Wilken.

I was working at that one, too, because I saw Claude Opus hinting on that one.
But it drove me crazy, because every AI is saying something slighty different. I
can not really pin down which one is actually the real solution. I tried to read
through the subsystems code and other drivers, but, argh, I don't know. I was
playing with the idea to (1) remove the raw HID mode completely or (2) make the
driver switchable, raw HID or normal HID, but not both at the same time. On the
other hand, in my Github repo where I develop the driver, I also have a tool
which demonstrates how to access the PSU completely in userspace via libhidpi.
There is actually no need to provide the raw HID access.

greetins Wilken