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

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.

Thanks,
Guenter