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

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

[Ali Ahmet Memis]

corsairpsu_request() sends a rail select command and then the actual
read as two separate transfers, both going through the single shared
cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
serializes its own callers, but the debugfs files call
corsairpsu_get_value() directly and never take that lock, so a debugfs
read can land between another reader's rail select and its value read.

The result is a value from the wrong rail reported as the right one,
because corsairpsu_usb_cmd() only checks the command echo and both
transfers echo the command it expects. It can also make a caller consume
the reply meant for the other one, since raw_event() writes into the
shared buffer and completes whoever happens to be waiting.

Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
on subsystem locking") on the grounds that the subsystem serializes for
us, which holds for sysfs but not for these files. Take
the same lock in the debugfs paths that issue commands, using the guard
added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
for subsystem locks"), as suggested in [1].

The lock cannot go into corsairpsu_request() itself: the hwmon core
already holds it across ->read, so every sysfs read would deadlock.
vendor_show() and product_show() only print strings cached during probe
and issue no command, and corsairpsu_get_criticals() and
corsairpsu_check_cmd_support() run before either interface is
registered, so none of them need it.

[1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/

Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
This is the fix Guenter asked for in the May thread, written the way he
suggested there. Wilken's patch used a driver private mutex around
corsairpsu_request(); that thread stalled and the race is still present.

Wilken, does this cover the chained command case you were worried about?
As far as I can tell it does: the whole select-rail plus read sequence
now runs under the same lock the hwmon core takes around ->read, so a
debugfs reader cannot land in the middle of one. If you had a case in
mind that this misses, I would rather hear it than guess.

I have no Corsair PSU, so this is reasoned from the code rather than
measured on hardware. What I did check:

  - hwmon_lock() takes hwdev->lock, and the hwmon core takes the same
    mutex around ->read and ->write, so this really does serialize the
    two entry points
  - the lock therefore cannot go into corsairpsu_request(), the sysfs
    path would deadlock on itself
  - probe registers the hwmon device before creating the debugfs files
    and remove tears them down in the opposite order, so priv->hwmon_dev
    is always valid inside a debugfs read

Prior discussion:
  https://lore.kernel.org/all/agR9YW7hGTJ_l7ms@monster.localdomain/

 drivers/hwmon/corsair-psu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index ce958cdaef58..24100519cd83 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
 	long val;
 	int ret;
 
+	guard(hwmon_lock)(priv->hwmon_dev);
+
 	ret = corsairpsu_get_value(priv, cmd, 0, &val);
 	if (ret < 0) {
 		seq_puts(seqf, "N/A\n");
@@ -730,6 +732,8 @@ static int ocpmode_show(struct seq_file *seqf, void *unused)
 	 * getting of the value itself can also fail during this. Because of this every other value
 	 * than OCP_MULTI_RAIL can be considered as "single rail".
 	 */
+	guard(hwmon_lock)(priv->hwmon_dev);
+
 	ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
 	if (ret < 0)
 		seq_puts(seqf, "N/A\n");

base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952
-- 
2.55.0

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

[Wilken Gottwalt]

On Sun,  2 Aug 2026 12:36:53 +0000
Ali Ahmet Memis <ali@iusegentoo.com> wrote:

> corsairpsu_request() sends a rail select command and then the actual
> read as two separate transfers, both going through the single shared
> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
> serializes its own callers, but the debugfs files call
> corsairpsu_get_value() directly and never take that lock, so a debugfs
> read can land between another reader's rail select and its value read.
> 
> The result is a value from the wrong rail reported as the right one,
> because corsairpsu_usb_cmd() only checks the command echo and both
> transfers echo the command it expects. It can also make a caller consume
> the reply meant for the other one, since raw_event() writes into the
> shared buffer and completes whoever happens to be waiting.
> 
> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
> on subsystem locking") on the grounds that the subsystem serializes for
> us, which holds for sysfs but not for these files. Take
> the same lock in the debugfs paths that issue commands, using the guard
> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
> for subsystem locks"), as suggested in [1].
> 
> The lock cannot go into corsairpsu_request() itself: the hwmon core
> already holds it across ->read, so every sysfs read would deadlock.
> vendor_show() and product_show() only print strings cached during probe
> and issue no command, and corsairpsu_get_criticals() and
> corsairpsu_check_cmd_support() run before either interface is
> registered, so none of them need it.
> 
> [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
> 
> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> This is the fix Guenter asked for in the May thread, written the way he
> suggested there. Wilken's patch used a driver private mutex around
> corsairpsu_request(); that thread stalled and the race is still present.
> 
> Wilken, does this cover the chained command case you were worried about?
> As far as I can tell it does: the whole select-rail plus read sequence
> now runs under the same lock the hwmon core takes around ->read, so a
> debugfs reader cannot land in the middle of one. If you had a case in
> mind that this misses, I would rather hear it than guess.

Yes, I think that is what Guenter asked me to test. There is actually a
way to get all values from the PSU at once. You can chain together all
the commands and everything supported should even fit into a single USB
HID frame (64bytes). That would make everything a bit easier. Though,
sorry that I did not go on with that. About a day after this someone put
basically all my open source projects through an AI agent and since then
I get bombarded with AI slop. I currently have not much energy (and fun)
left doing my projects.

I think I will test it in the next days.

greetings Wilken

> I have no Corsair PSU, so this is reasoned from the code rather than
> measured on hardware. What I did check:
> 
>   - hwmon_lock() takes hwdev->lock, and the hwmon core takes the same
>     mutex around ->read and ->write, so this really does serialize the
>     two entry points
>   - the lock therefore cannot go into corsairpsu_request(), the sysfs
>     path would deadlock on itself
>   - probe registers the hwmon device before creating the debugfs files
>     and remove tears them down in the opposite order, so priv->hwmon_dev
>     is always valid inside a debugfs read
> 
> Prior discussion:
>   https://lore.kernel.org/all/agR9YW7hGTJ_l7ms@monster.localdomain/
> 
>  drivers/hwmon/corsair-psu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index ce958cdaef58..24100519cd83 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
>  	long val;
>  	int ret;
>  
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, cmd, 0, &val);
>  	if (ret < 0) {
>  		seq_puts(seqf, "N/A\n");
> @@ -730,6 +732,8 @@ static int ocpmode_show(struct seq_file *seqf, void *unused)
>  	 * getting of the value itself can also fail during this. Because of this every other
> value
>  	 * than OCP_MULTI_RAIL can be considered as "single rail".
>  	 */
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
>  	if (ret < 0)
>  		seq_puts(seqf, "N/A\n");
> 
> base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952

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

[Guenter Roeck]

On 8/2/26 05:57, Wilken Gottwalt wrote:
> On Sun,  2 Aug 2026 12:36:53 +0000
> Ali Ahmet Memis <ali@iusegentoo.com> wrote:
> 
>> corsairpsu_request() sends a rail select command and then the actual
>> read as two separate transfers, both going through the single shared
>> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
>> serializes its own callers, but the debugfs files call
>> corsairpsu_get_value() directly and never take that lock, so a debugfs
>> read can land between another reader's rail select and its value read.
>>
>> The result is a value from the wrong rail reported as the right one,
>> because corsairpsu_usb_cmd() only checks the command echo and both
>> transfers echo the command it expects. It can also make a caller consume
>> the reply meant for the other one, since raw_event() writes into the
>> shared buffer and completes whoever happens to be waiting.
>>
>> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
>> on subsystem locking") on the grounds that the subsystem serializes for
>> us, which holds for sysfs but not for these files. Take
>> the same lock in the debugfs paths that issue commands, using the guard
>> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
>> for subsystem locks"), as suggested in [1].
>>
>> The lock cannot go into corsairpsu_request() itself: the hwmon core
>> already holds it across ->read, so every sysfs read would deadlock.
>> vendor_show() and product_show() only print strings cached during probe
>> and issue no command, and corsairpsu_get_criticals() and
>> corsairpsu_check_cmd_support() run before either interface is
>> registered, so none of them need it.
>>
>> [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
>>
>> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
>> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
>> ---
>> This is the fix Guenter asked for in the May thread, written the way he
>> suggested there. Wilken's patch used a driver private mutex around
>> corsairpsu_request(); that thread stalled and the race is still present.
>>
>> Wilken, does this cover the chained command case you were worried about?
>> As far as I can tell it does: the whole select-rail plus read sequence
>> now runs under the same lock the hwmon core takes around ->read, so a
>> debugfs reader cannot land in the middle of one. If you had a case in
>> mind that this misses, I would rather hear it than guess.
> 
> Yes, I think that is what Guenter asked me to test. There is actually a
> way to get all values from the PSU at once. You can chain together all
> the commands and everything supported should even fit into a single USB
> HID frame (64bytes). That would make everything a bit easier. Though,
> sorry that I did not go on with that. About a day after this someone put
> basically all my open source projects through an AI agent and since then
> I get bombarded with AI slop. I currently have not much energy (and fun)
> left doing my projects.
> 

Welcome to the club. I currently have more than 100 bugs reported by Sashiko
open against the hardware monitoring subsystem, and more than 50 against the
watchdog subsystem. I try to fix the critical issues as well as the security
issues, but trying to fix all of them would be futile (and Sashiko would
probably end up finding more bugs when analyzing the fixes). And, then,
yes, people submit more and more fixes for cosmetic issues reported and/or
suggested by some AI.

> I think I will test it in the next days.

Please let me know if you can find the time (or not). In most cases like this
I end up trusting the Sashiko review.

Thanks,
Guenter

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

[Guenter Roeck]

On 8/2/26 05:57, Wilken Gottwalt wrote:
> On Sun,  2 Aug 2026 12:36:53 +0000
> Ali Ahmet Memis <ali@iusegentoo.com> wrote:
> 
>> corsairpsu_request() sends a rail select command and then the actual
>> read as two separate transfers, both going through the single shared
>> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
>> serializes its own callers, but the debugfs files call
>> corsairpsu_get_value() directly and never take that lock, so a debugfs
>> read can land between another reader's rail select and its value read.
>>
>> The result is a value from the wrong rail reported as the right one,
>> because corsairpsu_usb_cmd() only checks the command echo and both
>> transfers echo the command it expects. It can also make a caller consume
>> the reply meant for the other one, since raw_event() writes into the
>> shared buffer and completes whoever happens to be waiting.
>>
>> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
>> on subsystem locking") on the grounds that the subsystem serializes for
>> us, which holds for sysfs but not for these files. Take
>> the same lock in the debugfs paths that issue commands, using the guard
>> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
>> for subsystem locks"), as suggested in [1].
>>
>> The lock cannot go into corsairpsu_request() itself: the hwmon core
>> already holds it across ->read, so every sysfs read would deadlock.
>> vendor_show() and product_show() only print strings cached during probe
>> and issue no command, and corsairpsu_get_criticals() and
>> corsairpsu_check_cmd_support() run before either interface is
>> registered, so none of them need it.
>>
>> [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
>>
>> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
>> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
>> ---
>> This is the fix Guenter asked for in the May thread, written the way he
>> suggested there. Wilken's patch used a driver private mutex around
>> corsairpsu_request(); that thread stalled and the race is still present.
>>
>> Wilken, does this cover the chained command case you were worried about?
>> As far as I can tell it does: the whole select-rail plus read sequence
>> now runs under the same lock the hwmon core takes around ->read, so a
>> debugfs reader cannot land in the middle of one. If you had a case in
>> mind that this misses, I would rather hear it than guess.
> 
> Yes, I think that is what Guenter asked me to test. There is actually a
> way to get all values from the PSU at once. You can chain together all
> the commands and everything supported should even fit into a single USB
> HID frame (64bytes). That would make everything a bit easier. Though,
> sorry that I did not go on with that. About a day after this someone put
> basically all my open source projects through an AI agent and since then
> I get bombarded with AI slop. I currently have not much energy (and fun)
> left doing my projects.
> 
> I think I will test it in the next days.
> 
Turns out I had written pretty much exactly the same patch earlier this year.
I have no idea why I did not send it out.

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 ?

I'll send a separate patch to fix the sign extension and the shifting of
negative values in corsairpsu_linear11_to_int().

Thanks,
Guenter

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

[Ali Ahmet Memis]

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.

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

[Guenter Roeck]

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

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

[Wilken Gottwalt]

On Mon, 3 Aug 2026 19:54:49 -0700
Guenter Roeck <linux@roeck-us.net> 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

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

[Ali Ahmet Memis]

On Tue, 04 Aug 2026 04:11:11 +0000 Wilken Gottwalt wrote:
> 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.

Two different things are getting mixed together here, which may be why the
answers you got do not agree with each other.

->raw_event is not the userspace side. It is how the driver receives its
replies: corsairpsu_usb_cmd() sends the report and then blocks on
wait_completion, and corsairpsu_raw_event() is what fills cmd_buffer and
completes it. Take that away and no command ever returns.

The raw HID mode you can actually drop is HID_CONNECT_HIDRAW in
hid_hw_start(), which is what creates /dev/hidrawN. That one is allowed to
go: hid_connect() only refuses a device with no listeners if the driver has
no ->raw_event, and this driver has one.

Dropping it does not fix the cmd_buffer problem though, it only removes one
way to trigger it. The window is internal: usb_cmd() times out, the reply
arrives late anyway, the next command's reinit_completion() re-arms the
guard at the top of raw_event, and that stale reply is taken as the answer
to the new command. Nothing in userspace is needed for that.

One thing worth checking before going with (1): if your libhidapi tool uses
the hidraw backend it goes through /dev/hidrawN, so removing
HID_CONNECT_HIDRAW would take that tool's access away as well.

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

[Ali Ahmet Memis]

> the next command's reinit_completion() re-arms the guard at the top of
> raw_event, and that stale reply is taken as the answer to the new command

Correcting myself on the mechanism, since it changes what a fix has to do.

reinit_completion() is not what defeats that guard. completion_done() is
just x->done != 0, and x->done is zero in every state between commands: a
successful wait_for_completion_timeout() decrements it back to zero, and a
timeout leaves it at zero because it was never set. So the check at the top
of raw_event practically never rejects anything, and reinit_completion() on
the next command writes zero over a value that is already zero.

The outcome I described is the same, a late reply still lands in cmd_buffer
and completes the next waiter with the previous command's data. But that
check is not a "command in flight" flag and cannot be turned into one by
moving it around, so whatever fixes this needs a way to tell which command a
reply actually belongs to.

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

[Guenter Roeck]

On Tue, Aug 04, 2026 at 04:11:11AM +0000, Wilken Gottwalt wrote:
> > 
> > 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.
> 

Have a look at the patch below. It is part AI (Gemini) generated and part me.
Sashiko is happy with it, but of course that doesn't mean it is perfect or
even correct. It does look good to me, though.

Making Sashiko happy required all core elements of the patch:
- the spinlock
- the separate receive buffer
- the rcv_pending boolean
- the size check in corsairpsu_raw_event()

Sashiko reports race conditions if I drop just one of those elements.

Guenter

---
From 50fae138603a9a6b1929cbe9a644310495688eea Mon Sep 17 00:00:00 2001
From: Guenter Roeck <groeck@google.com>
Date: Mon, 3 Aug 2026 17:39:21 -0700
Subject: [PATCH] hwmon: (corsair-psu) Separate request/response buffers and
 validate reply echo

In corsairpsu_usb_cmd(), a single shared buffer (priv->cmd_buffer) is used
both for transmitting command reports via hid_hw_output_report() and for
receiving device responses in corsairpsu_raw_event().

If a command sent via corsairpsu_usb_cmd() times out, the caller stops
waiting, but the hardware may still process the command and send a delayed
response later. If a subsequent command is being prepared or transmitted
when this delayed response arrives, corsairpsu_raw_event() blindly copies
the incoming report into priv->cmd_buffer and completes wait_completion:

corsairpsu_raw_event()
    if (completion_done(&priv->wait_completion))
        return 0;

    memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
    complete(&priv->wait_completion);

This causes a data race where priv->cmd_buffer can be overwritten with
the old delayed response while hid_hw_output_report() is transmitting the
new command, potentially causing the PSU to receive invalid parameters or
shut down. In addition, the caller of the subsequent command will wake up
early and either consume stale data or fail unexpectedly.

Fix the problem by:
- Allocating a separate response buffer (priv->res_buffer) so that
  corsairpsu_raw_event() never modifies priv->cmd_buffer during outgoing
  transfers.
- Validating incoming reports in corsairpsu_raw_event() to ensure that the
  echoed length and command opcode match the pending command in
  priv->cmd_buffer (or indicate an unsupported command opcode with 0).
- Protecting buffer initialization, reinit_completion(), and report
  validation/completion with a spinlock (wait_completion_lock).
- Adding a boolean flag indicating that the code is waiting for a response,
  and only copying the reply into the receive buffer if that is the case.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Guenter Roeck <groeck@google.com>
---
 drivers/hwmon/corsair-psu.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index 5592b927e9d4..3eebf8494ed8 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 /*
@@ -122,7 +123,10 @@ struct corsairpsu_data {
 	struct device *hwmon_dev;
 	struct dentry *debugfs;
 	struct completion wait_completion;
+	spinlock_t completion_lock;	/* locks wait_completion, cmd_buffer, and res_buffer */
 	u8 *cmd_buffer;
+	u8 *res_buffer;
+	bool rcv_pending;
 	char vendor[REPLY_SIZE];
 	char product[REPLY_SIZE];
 	long temp_crit[TEMP_COUNT];
@@ -158,15 +162,19 @@ static int corsairpsu_dutycycle_to_pwm(const long dutycycle)
 
 static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data)
 {
+	unsigned long flags;
 	unsigned long time;
 	int ret;
 
+	spin_lock_irqsave(&priv->completion_lock, flags);
 	memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
+	memset(priv->res_buffer, 0, CMD_BUFFER_SIZE);
 	priv->cmd_buffer[0] = p0;
 	priv->cmd_buffer[1] = p1;
 	priv->cmd_buffer[2] = p2;
-
 	reinit_completion(&priv->wait_completion);
+	priv->rcv_pending = true;
+	spin_unlock_irqrestore(&priv->completion_lock, flags);
 
 	ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
 	if (ret < 0)
@@ -182,11 +190,11 @@ static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2,
 	 * was send, not every command is supported on every device class, if a command is not
 	 * supported, the length value in the reply is okay, but the command value is set to 0
 	 */
-	if (p0 != priv->cmd_buffer[0] || p1 != priv->cmd_buffer[1])
+	if (p0 != priv->res_buffer[0] || p1 != priv->res_buffer[1])
 		return -EOPNOTSUPP;
 
 	if (data)
-		memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
+		memcpy(data, priv->res_buffer + 2, REPLY_SIZE);
 
 	return 0;
 }
@@ -781,6 +789,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 	if (!priv->cmd_buffer)
 		return -ENOMEM;
 
+	priv->res_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL);
+	if (!priv->res_buffer)
+		return -ENOMEM;
+
 	ret = hid_parse(hdev);
 	if (ret)
 		return ret;
@@ -795,6 +807,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 
 	priv->hdev = hdev;
 	hid_set_drvdata(hdev, priv);
+	spin_lock_init(&priv->completion_lock);
 	init_completion(&priv->wait_completion);
 
 	hid_device_io_start(hdev);
@@ -848,12 +861,20 @@ static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report *repo
 				int size)
 {
 	struct corsairpsu_data *priv = hid_get_drvdata(hdev);
+	unsigned long flags;
 
-	if (completion_done(&priv->wait_completion))
+	if (size < 2)
 		return 0;
 
-	memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
-	complete(&priv->wait_completion);
+	spin_lock_irqsave(&priv->completion_lock, flags);
+	if (priv->rcv_pending && !completion_done(&priv->wait_completion) &&
+	    data[0] == priv->cmd_buffer[0] &&
+	    (data[1] == priv->cmd_buffer[1] || data[1] == 0)) {
+		memcpy(priv->res_buffer, data, min(CMD_BUFFER_SIZE, size));
+		complete(&priv->wait_completion);
+		priv->rcv_pending = false;
+	}
+	spin_unlock_irqrestore(&priv->completion_lock, flags);
 
 	return 0;
 }
-- 
2.55.0

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

[Wilken Gottwalt]

On Tue, 4 Aug 2026 09:34:38 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Tue, Aug 04, 2026 at 04:11:11AM +0000, Wilken Gottwalt wrote:
> > > 
> > > 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.
> > 
> 
> Have a look at the patch below. It is part AI (Gemini) generated and part me.
> Sashiko is happy with it, but of course that doesn't mean it is perfect or
> even correct. It does look good to me, though.

Yeah, that is the standard proposal basically all decent LLMs produce. Deepseek V4
calls it rx_buffer and tx_buffer and talks about preventing race conditions by
adding a spinlock and quote "don't use a mutex". Hmm, I put it on my "check it in
the next days" list.

greetings Wilken

> Making Sashiko happy required all core elements of the patch:
> - the spinlock
> - the separate receive buffer
> - the rcv_pending boolean
> - the size check in corsairpsu_raw_event()
> 
> Sashiko reports race conditions if I drop just one of those elements.
> 
> Guenter
> 
> ---
> From 50fae138603a9a6b1929cbe9a644310495688eea Mon Sep 17 00:00:00 2001
> From: Guenter Roeck <groeck@google.com>
> Date: Mon, 3 Aug 2026 17:39:21 -0700
> Subject: [PATCH] hwmon: (corsair-psu) Separate request/response buffers and
>  validate reply echo
> 
> In corsairpsu_usb_cmd(), a single shared buffer (priv->cmd_buffer) is used
> both for transmitting command reports via hid_hw_output_report() and for
> receiving device responses in corsairpsu_raw_event().
> 
> If a command sent via corsairpsu_usb_cmd() times out, the caller stops
> waiting, but the hardware may still process the command and send a delayed
> response later. If a subsequent command is being prepared or transmitted
> when this delayed response arrives, corsairpsu_raw_event() blindly copies
> the incoming report into priv->cmd_buffer and completes wait_completion:
> 
> corsairpsu_raw_event()
>     if (completion_done(&priv->wait_completion))
>         return 0;
> 
>     memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
>     complete(&priv->wait_completion);
> 
> This causes a data race where priv->cmd_buffer can be overwritten with
> the old delayed response while hid_hw_output_report() is transmitting the
> new command, potentially causing the PSU to receive invalid parameters or
> shut down. In addition, the caller of the subsequent command will wake up
> early and either consume stale data or fail unexpectedly.
> 
> Fix the problem by:
> - Allocating a separate response buffer (priv->res_buffer) so that
>   corsairpsu_raw_event() never modifies priv->cmd_buffer during outgoing
>   transfers.
> - Validating incoming reports in corsairpsu_raw_event() to ensure that the
>   echoed length and command opcode match the pending command in
>   priv->cmd_buffer (or indicate an unsupported command opcode with 0).
> - Protecting buffer initialization, reinit_completion(), and report
>   validation/completion with a spinlock (wait_completion_lock).
> - Adding a boolean flag indicating that the code is waiting for a response,
>   and only copying the reply into the receive buffer if that is the case.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Guenter Roeck <groeck@google.com>
> ---
>  drivers/hwmon/corsair-psu.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index 5592b927e9d4..3eebf8494ed8 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -122,7 +123,10 @@ struct corsairpsu_data {
>  	struct device *hwmon_dev;
>  	struct dentry *debugfs;
>  	struct completion wait_completion;
> +	spinlock_t completion_lock;	/* locks wait_completion, cmd_buffer, and res_buffer
> */ u8 *cmd_buffer;
> +	u8 *res_buffer;
> +	bool rcv_pending;
>  	char vendor[REPLY_SIZE];
>  	char product[REPLY_SIZE];
>  	long temp_crit[TEMP_COUNT];
> @@ -158,15 +162,19 @@ static int corsairpsu_dutycycle_to_pwm(const long dutycycle)
>  
>  static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data)
>  {
> +	unsigned long flags;
>  	unsigned long time;
>  	int ret;
>  
> +	spin_lock_irqsave(&priv->completion_lock, flags);
>  	memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
> +	memset(priv->res_buffer, 0, CMD_BUFFER_SIZE);
>  	priv->cmd_buffer[0] = p0;
>  	priv->cmd_buffer[1] = p1;
>  	priv->cmd_buffer[2] = p2;
> -
>  	reinit_completion(&priv->wait_completion);
> +	priv->rcv_pending = true;
> +	spin_unlock_irqrestore(&priv->completion_lock, flags);
>  
>  	ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
>  	if (ret < 0)
> @@ -182,11 +190,11 @@ static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1,
> u8 p2,
>  	 * was send, not every command is supported on every device class, if a command is not
>  	 * supported, the length value in the reply is okay, but the command value is set to 0
>  	 */
> -	if (p0 != priv->cmd_buffer[0] || p1 != priv->cmd_buffer[1])
> +	if (p0 != priv->res_buffer[0] || p1 != priv->res_buffer[1])
>  		return -EOPNOTSUPP;
>  
>  	if (data)
> -		memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
> +		memcpy(data, priv->res_buffer + 2, REPLY_SIZE);
>  
>  	return 0;
>  }
> @@ -781,6 +789,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id if (!priv->cmd_buffer)
>  		return -ENOMEM;
>  
> +	priv->res_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL);
> +	if (!priv->res_buffer)
> +		return -ENOMEM;
> +
>  	ret = hid_parse(hdev);
>  	if (ret)
>  		return ret;
> @@ -795,6 +807,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id 
>  	priv->hdev = hdev;
>  	hid_set_drvdata(hdev, priv);
> +	spin_lock_init(&priv->completion_lock);
>  	init_completion(&priv->wait_completion);
>  
>  	hid_device_io_start(hdev);
> @@ -848,12 +861,20 @@ static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report
> *repo int size)
>  {
>  	struct corsairpsu_data *priv = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
> -	if (completion_done(&priv->wait_completion))
> +	if (size < 2)
>  		return 0;
>  
> -	memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
> -	complete(&priv->wait_completion);
> +	spin_lock_irqsave(&priv->completion_lock, flags);
> +	if (priv->rcv_pending && !completion_done(&priv->wait_completion) &&
> +	    data[0] == priv->cmd_buffer[0] &&
> +	    (data[1] == priv->cmd_buffer[1] || data[1] == 0)) {
> +		memcpy(priv->res_buffer, data, min(CMD_BUFFER_SIZE, size));
> +		complete(&priv->wait_completion);
> +		priv->rcv_pending = false;
> +	}
> +	spin_unlock_irqrestore(&priv->completion_lock, flags);
>  
>  	return 0;
>  }

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

[Wilken Gottwalt]

On Tue, 4 Aug 2026 09:34:38 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Tue, Aug 04, 2026 at 04:11:11AM +0000, Wilken Gottwalt wrote:
> > > 
> > > 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.
> > 
> 
> Have a look at the patch below. It is part AI (Gemini) generated and part me.
> Sashiko is happy with it, but of course that doesn't mean it is perfect or
> even correct. It does look good to me, though.
> 
> Making Sashiko happy required all core elements of the patch:
> - the spinlock
> - the separate receive buffer
> - the rcv_pending boolean
> - the size check in corsairpsu_raw_event()
> 
> Sashiko reports race conditions if I drop just one of those elements.
> 
> Guenter
> 
> ---
> From 50fae138603a9a6b1929cbe9a644310495688eea Mon Sep 17 00:00:00 2001
> From: Guenter Roeck <groeck@google.com>
> Date: Mon, 3 Aug 2026 17:39:21 -0700
> Subject: [PATCH] hwmon: (corsair-psu) Separate request/response buffers and
>  validate reply echo
> 
> In corsairpsu_usb_cmd(), a single shared buffer (priv->cmd_buffer) is used
> both for transmitting command reports via hid_hw_output_report() and for
> receiving device responses in corsairpsu_raw_event().
> 
> If a command sent via corsairpsu_usb_cmd() times out, the caller stops
> waiting, but the hardware may still process the command and send a delayed
> response later. If a subsequent command is being prepared or transmitted
> when this delayed response arrives, corsairpsu_raw_event() blindly copies
> the incoming report into priv->cmd_buffer and completes wait_completion:
> 
> corsairpsu_raw_event()
>     if (completion_done(&priv->wait_completion))
>         return 0;
> 
>     memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
>     complete(&priv->wait_completion);
> 
> This causes a data race where priv->cmd_buffer can be overwritten with
> the old delayed response while hid_hw_output_report() is transmitting the
> new command, potentially causing the PSU to receive invalid parameters or
> shut down. In addition, the caller of the subsequent command will wake up
> early and either consume stale data or fail unexpectedly.
> 
> Fix the problem by:
> - Allocating a separate response buffer (priv->res_buffer) so that
>   corsairpsu_raw_event() never modifies priv->cmd_buffer during outgoing
>   transfers.
> - Validating incoming reports in corsairpsu_raw_event() to ensure that the
>   echoed length and command opcode match the pending command in
>   priv->cmd_buffer (or indicate an unsupported command opcode with 0).
> - Protecting buffer initialization, reinit_completion(), and report
>   validation/completion with a spinlock (wait_completion_lock).
> - Adding a boolean flag indicating that the code is waiting for a response,
>   and only copying the reply into the receive buffer if that is the case.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Guenter Roeck <groeck@google.com>
> ---
>  drivers/hwmon/corsair-psu.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index 5592b927e9d4..3eebf8494ed8 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -122,7 +123,10 @@ struct corsairpsu_data {
>  	struct device *hwmon_dev;
>  	struct dentry *debugfs;
>  	struct completion wait_completion;
> +	spinlock_t completion_lock;	/* locks wait_completion, cmd_buffer, and res_buffer
> */ u8 *cmd_buffer;
> +	u8 *res_buffer;
> +	bool rcv_pending;
>  	char vendor[REPLY_SIZE];
>  	char product[REPLY_SIZE];
>  	long temp_crit[TEMP_COUNT];
> @@ -158,15 +162,19 @@ static int corsairpsu_dutycycle_to_pwm(const long dutycycle)
>  
>  static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data)
>  {
> +	unsigned long flags;
>  	unsigned long time;
>  	int ret;
>  
> +	spin_lock_irqsave(&priv->completion_lock, flags);
>  	memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
> +	memset(priv->res_buffer, 0, CMD_BUFFER_SIZE);
>  	priv->cmd_buffer[0] = p0;
>  	priv->cmd_buffer[1] = p1;
>  	priv->cmd_buffer[2] = p2;
> -
>  	reinit_completion(&priv->wait_completion);
> +	priv->rcv_pending = true;
> +	spin_unlock_irqrestore(&priv->completion_lock, flags);
>  
>  	ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
>  	if (ret < 0)
> @@ -182,11 +190,11 @@ static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1,
> u8 p2,
>  	 * was send, not every command is supported on every device class, if a command is not
>  	 * supported, the length value in the reply is okay, but the command value is set to 0
>  	 */
> -	if (p0 != priv->cmd_buffer[0] || p1 != priv->cmd_buffer[1])
> +	if (p0 != priv->res_buffer[0] || p1 != priv->res_buffer[1])
>  		return -EOPNOTSUPP;
>  
>  	if (data)
> -		memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
> +		memcpy(data, priv->res_buffer + 2, REPLY_SIZE);
>  
>  	return 0;
>  }
> @@ -781,6 +789,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id if (!priv->cmd_buffer)
>  		return -ENOMEM;
>  
> +	priv->res_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL);
> +	if (!priv->res_buffer)
> +		return -ENOMEM;
> +
>  	ret = hid_parse(hdev);
>  	if (ret)
>  		return ret;
> @@ -795,6 +807,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id 
>  	priv->hdev = hdev;
>  	hid_set_drvdata(hdev, priv);
> +	spin_lock_init(&priv->completion_lock);
>  	init_completion(&priv->wait_completion);
>  
>  	hid_device_io_start(hdev);
> @@ -848,12 +861,20 @@ static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report
> *repo int size)
>  {
>  	struct corsairpsu_data *priv = hid_get_drvdata(hdev);
> +	unsigned long flags;
>  
> -	if (completion_done(&priv->wait_completion))
> +	if (size < 2)
>  		return 0;
>  
> -	memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
> -	complete(&priv->wait_completion);
> +	spin_lock_irqsave(&priv->completion_lock, flags);
> +	if (priv->rcv_pending && !completion_done(&priv->wait_completion) &&
> +	    data[0] == priv->cmd_buffer[0] &&
> +	    (data[1] == priv->cmd_buffer[1] || data[1] == 0)) {
> +		memcpy(priv->res_buffer, data, min(CMD_BUFFER_SIZE, size));
> +		complete(&priv->wait_completion);
> +		priv->rcv_pending = false;
> +	}
> +	spin_unlock_irqrestore(&priv->completion_lock, flags);
>  
>  	return 0;
>  }

I don't know, something is odd about this. I'm not 100% sure, but the lock is
released before hid_hw_output_report(), which is necessary because a spinlock
must not be held by a potentially sleeping call. This leaves a window in which
a delayed reply can satisfy a freshly reinitialized wait_completion() even
before the actual send, if the new command has the same echo (p0/p1) as the old
one. And that is precisely the typical scenario when polling the same sensor
attributes (always 3, 0x8B for rail voltage). Or I just misinterpret this whole
thing entirely, I'm getting tired...

greetings Wilken

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

[Guenter Roeck]

On 8/4/26 11:37, Wilken Gottwalt wrote:
> On Tue, 4 Aug 2026 09:34:38 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Tue, Aug 04, 2026 at 04:11:11AM +0000, Wilken Gottwalt wrote:
>>>>
>>>> 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.
>>>
>>
>> Have a look at the patch below. It is part AI (Gemini) generated and part me.
>> Sashiko is happy with it, but of course that doesn't mean it is perfect or
>> even correct. It does look good to me, though.
>>
>> Making Sashiko happy required all core elements of the patch:
>> - the spinlock
>> - the separate receive buffer
>> - the rcv_pending boolean
>> - the size check in corsairpsu_raw_event()
>>
>> Sashiko reports race conditions if I drop just one of those elements.
>>
>> Guenter
>>
>> ---
>>  From 50fae138603a9a6b1929cbe9a644310495688eea Mon Sep 17 00:00:00 2001
>> From: Guenter Roeck <groeck@google.com>
>> Date: Mon, 3 Aug 2026 17:39:21 -0700
>> Subject: [PATCH] hwmon: (corsair-psu) Separate request/response buffers and
>>   validate reply echo
>>
>> In corsairpsu_usb_cmd(), a single shared buffer (priv->cmd_buffer) is used
>> both for transmitting command reports via hid_hw_output_report() and for
>> receiving device responses in corsairpsu_raw_event().
>>
>> If a command sent via corsairpsu_usb_cmd() times out, the caller stops
>> waiting, but the hardware may still process the command and send a delayed
>> response later. If a subsequent command is being prepared or transmitted
>> when this delayed response arrives, corsairpsu_raw_event() blindly copies
>> the incoming report into priv->cmd_buffer and completes wait_completion:
>>
>> corsairpsu_raw_event()
>>      if (completion_done(&priv->wait_completion))
>>          return 0;
>>
>>      memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
>>      complete(&priv->wait_completion);
>>
>> This causes a data race where priv->cmd_buffer can be overwritten with
>> the old delayed response while hid_hw_output_report() is transmitting the
>> new command, potentially causing the PSU to receive invalid parameters or
>> shut down. In addition, the caller of the subsequent command will wake up
>> early and either consume stale data or fail unexpectedly.
>>
>> Fix the problem by:
>> - Allocating a separate response buffer (priv->res_buffer) so that
>>    corsairpsu_raw_event() never modifies priv->cmd_buffer during outgoing
>>    transfers.
>> - Validating incoming reports in corsairpsu_raw_event() to ensure that the
>>    echoed length and command opcode match the pending command in
>>    priv->cmd_buffer (or indicate an unsupported command opcode with 0).
>> - Protecting buffer initialization, reinit_completion(), and report
>>    validation/completion with a spinlock (wait_completion_lock).
>> - Adding a boolean flag indicating that the code is waiting for a response,
>>    and only copying the reply into the receive buffer if that is the case.
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Signed-off-by: Guenter Roeck <groeck@google.com>
>> ---
>>   drivers/hwmon/corsair-psu.c | 33 +++++++++++++++++++++++++++------
>>   1 file changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
>> index 5592b927e9d4..3eebf8494ed8 100644
>> --- a/drivers/hwmon/corsair-psu.c
>> +++ b/drivers/hwmon/corsair-psu.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/slab.h>
>> +#include <linux/spinlock.h>
>>   #include <linux/types.h>
>>   
>>   /*
>> @@ -122,7 +123,10 @@ struct corsairpsu_data {
>>   	struct device *hwmon_dev;
>>   	struct dentry *debugfs;
>>   	struct completion wait_completion;
>> +	spinlock_t completion_lock;	/* locks wait_completion, cmd_buffer, and res_buffer
>> */ u8 *cmd_buffer;
>> +	u8 *res_buffer;
>> +	bool rcv_pending;
>>   	char vendor[REPLY_SIZE];
>>   	char product[REPLY_SIZE];
>>   	long temp_crit[TEMP_COUNT];
>> @@ -158,15 +162,19 @@ static int corsairpsu_dutycycle_to_pwm(const long dutycycle)
>>   
>>   static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data)
>>   {
>> +	unsigned long flags;
>>   	unsigned long time;
>>   	int ret;
>>   
>> +	spin_lock_irqsave(&priv->completion_lock, flags);
>>   	memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
>> +	memset(priv->res_buffer, 0, CMD_BUFFER_SIZE);
>>   	priv->cmd_buffer[0] = p0;
>>   	priv->cmd_buffer[1] = p1;
>>   	priv->cmd_buffer[2] = p2;
>> -
>>   	reinit_completion(&priv->wait_completion);
>> +	priv->rcv_pending = true;
>> +	spin_unlock_irqrestore(&priv->completion_lock, flags);
>>   
>>   	ret = hid_hw_output_report(priv->hdev, priv->cmd_buffer, CMD_BUFFER_SIZE);
>>   	if (ret < 0)
>> @@ -182,11 +190,11 @@ static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1,
>> u8 p2,
>>   	 * was send, not every command is supported on every device class, if a command is not
>>   	 * supported, the length value in the reply is okay, but the command value is set to 0
>>   	 */
>> -	if (p0 != priv->cmd_buffer[0] || p1 != priv->cmd_buffer[1])
>> +	if (p0 != priv->res_buffer[0] || p1 != priv->res_buffer[1])
>>   		return -EOPNOTSUPP;
>>   
>>   	if (data)
>> -		memcpy(data, priv->cmd_buffer + 2, REPLY_SIZE);
>> +		memcpy(data, priv->res_buffer + 2, REPLY_SIZE);
>>   
>>   	return 0;
>>   }
>> @@ -781,6 +789,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
>> hid_device_id if (!priv->cmd_buffer)
>>   		return -ENOMEM;
>>   
>> +	priv->res_buffer = devm_kmalloc(&hdev->dev, CMD_BUFFER_SIZE, GFP_KERNEL);
>> +	if (!priv->res_buffer)
>> +		return -ENOMEM;
>> +
>>   	ret = hid_parse(hdev);
>>   	if (ret)
>>   		return ret;
>> @@ -795,6 +807,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
>> hid_device_id
>>   	priv->hdev = hdev;
>>   	hid_set_drvdata(hdev, priv);
>> +	spin_lock_init(&priv->completion_lock);
>>   	init_completion(&priv->wait_completion);
>>   
>>   	hid_device_io_start(hdev);
>> @@ -848,12 +861,20 @@ static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report
>> *repo int size)
>>   {
>>   	struct corsairpsu_data *priv = hid_get_drvdata(hdev);
>> +	unsigned long flags;
>>   
>> -	if (completion_done(&priv->wait_completion))
>> +	if (size < 2)
>>   		return 0;
>>   
>> -	memcpy(priv->cmd_buffer, data, min(CMD_BUFFER_SIZE, size));
>> -	complete(&priv->wait_completion);
>> +	spin_lock_irqsave(&priv->completion_lock, flags);
>> +	if (priv->rcv_pending && !completion_done(&priv->wait_completion) &&
>> +	    data[0] == priv->cmd_buffer[0] &&
>> +	    (data[1] == priv->cmd_buffer[1] || data[1] == 0)) {
>> +		memcpy(priv->res_buffer, data, min(CMD_BUFFER_SIZE, size));
>> +		complete(&priv->wait_completion);
>> +		priv->rcv_pending = false;
>> +	}
>> +	spin_unlock_irqrestore(&priv->completion_lock, flags);
>>   
>>   	return 0;
>>   }
> 
> I don't know, something is odd about this. I'm not 100% sure, but the lock is
> released before hid_hw_output_report(), which is necessary because a spinlock
> must not be held by a potentially sleeping call. This leaves a window in which
> a delayed reply can satisfy a freshly reinitialized wait_completion() even
> before the actual send, if the new command has the same echo (p0/p1) as the old
> one. And that is precisely the typical scenario when polling the same sensor
> attributes (always 3, 0x8B for rail voltage). Or I just misinterpret this whole
> thing entirely, I'm getting tired...
> 
Unless I am missing something there is nothing you can do about this unless there
is a sequence number in the message. I think this is why the AI insists that there
is a separate receive buffer (to avoid overwriting the command buffer in this
scenario).

Guenter

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

[Wilken Gottwalt]

On Sun,  2 Aug 2026 12:36:53 +0000
Ali Ahmet Memis <ali@iusegentoo.com> wrote:

> corsairpsu_request() sends a rail select command and then the actual
> read as two separate transfers, both going through the single shared
> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
> serializes its own callers, but the debugfs files call
> corsairpsu_get_value() directly and never take that lock, so a debugfs
> read can land between another reader's rail select and its value read.
> 
> The result is a value from the wrong rail reported as the right one,
> because corsairpsu_usb_cmd() only checks the command echo and both
> transfers echo the command it expects. It can also make a caller consume
> the reply meant for the other one, since raw_event() writes into the
> shared buffer and completes whoever happens to be waiting.
> 
> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
> on subsystem locking") on the grounds that the subsystem serializes for
> us, which holds for sysfs but not for these files. Take
> the same lock in the debugfs paths that issue commands, using the guard
> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
> for subsystem locks"), as suggested in [1].
> 
> The lock cannot go into corsairpsu_request() itself: the hwmon core
> already holds it across ->read, so every sysfs read would deadlock.
> vendor_show() and product_show() only print strings cached during probe
> and issue no command, and corsairpsu_get_criticals() and
> corsairpsu_check_cmd_support() run before either interface is
> registered, so none of them need it.
> 
> [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
> 
> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> This is the fix Guenter asked for in the May thread, written the way he
> suggested there. Wilken's patch used a driver private mutex around
> corsairpsu_request(); that thread stalled and the race is still present.
> 
> Wilken, does this cover the chained command case you were worried about?
> As far as I can tell it does: the whole select-rail plus read sequence
> now runs under the same lock the hwmon core takes around ->read, so a
> debugfs reader cannot land in the middle of one. If you had a case in
> mind that this misses, I would rather hear it than guess.
> 
> I have no Corsair PSU, so this is reasoned from the code rather than
> measured on hardware. What I did check:
> 
>   - hwmon_lock() takes hwdev->lock, and the hwmon core takes the same
>     mutex around ->read and ->write, so this really does serialize the
>     two entry points
>   - the lock therefore cannot go into corsairpsu_request(), the sysfs
>     path would deadlock on itself
>   - probe registers the hwmon device before creating the debugfs files
>     and remove tears them down in the opposite order, so priv->hwmon_dev
>     is always valid inside a debugfs read
> 
> Prior discussion:
>   https://lore.kernel.org/all/agR9YW7hGTJ_l7ms@monster.localdomain/
> 
>  drivers/hwmon/corsair-psu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index ce958cdaef58..24100519cd83 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
>  	long val;
>  	int ret;
>  
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, cmd, 0, &val);
>  	if (ret < 0) {
>  		seq_puts(seqf, "N/A\n");
> @@ -730,6 +732,8 @@ static int ocpmode_show(struct seq_file *seqf, void *unused)
>  	 * getting of the value itself can also fail during this. Because of this every other
> value
>  	 * than OCP_MULTI_RAIL can be considered as "single rail".
>  	 */
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
>  	if (ret < 0)
>  		seq_puts(seqf, "N/A\n");
> 
> base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952

That does not even compile on a current 7.1.5/7.1.6 kernel. Though, not sure
yet, what that is. But I can not risk running a trunk kernel on my workstation.

/usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:302:9: error: unknown type name ‘class_hwmon_lock_t’; did you mean ‘class_task_lock_t’?
  302 |         class_##_name##_t var __cleanup(class_##_name##_destructor) =   \
      |         ^~~~~~
/usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:422:9: note: in expansion of macro ‘CLASS’
  422 |         CLASS(_name, __UNIQUE_ID(guard))
      |         ^~~~~
corsair-psu.c:667:9: note: in expansion of macro ‘guard’
  667 |         guard(hwmon_lock)(priv->hwmon_dev);
      |         ^~~~~
corsair-psu.c:667:9: error: cleanup argument not a function
  667 |         guard(hwmon_lock)(priv->hwmon_dev);
      |         ^~~~~

greetings,
Wilken

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

[Guenter Roeck]

On 8/5/26 22:23, Wilken Gottwalt wrote:

> 
> That does not even compile on a current 7.1.5/7.1.6 kernel. Though, not sure
> yet, what that is. But I can not risk running a trunk kernel on my workstation.
> 
> /usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:302:9: error: unknown type name ‘class_hwmon_lock_t’; did you mean ‘class_task_lock_t’?
>    302 |         class_##_name##_t var __cleanup(class_##_name##_destructor) =   \
>        |         ^~~~~~
> /usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:422:9: note: in expansion of macro ‘CLASS’
>    422 |         CLASS(_name, __UNIQUE_ID(guard))
>        |         ^~~~~
> corsair-psu.c:667:9: note: in expansion of macro ‘guard’
>    667 |         guard(hwmon_lock)(priv->hwmon_dev);
>        |         ^~~~~
> corsair-psu.c:667:9: error: cleanup argument not a function
>    667 |         guard(hwmon_lock)(priv->hwmon_dev);
>        |         ^~~~~
> 

You'll need commit d1e720c7328e ("hwmon: Support guard() and scoped_guard for subsystem locks").

Guenter

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

[Wilken Gottwalt]

On Thu, 6 Aug 2026 02:23:16 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 8/5/26 22:23, Wilken Gottwalt wrote:
> 
> > 
> > That does not even compile on a current 7.1.5/7.1.6 kernel. Though, not sure
> > yet, what that is. But I can not risk running a trunk kernel on my workstation.
> > 
> > /usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:302:9: error: unknown type name
> > ‘class_hwmon_lock_t’; did you mean ‘class_task_lock_t’? 302 |         class_##_name##_t var
> > __cleanup(class_##_name##_destructor) =   \ |         ^~~~~~
> > /usr/lib/modules/7.1.5-arch1-2/build/include/linux/cleanup.h:422:9: note: in expansion of macro
> > ‘CLASS’ 422 |         CLASS(_name, __UNIQUE_ID(guard))
> >        |         ^~~~~
> > corsair-psu.c:667:9: note: in expansion of macro ‘guard’
> >    667 |         guard(hwmon_lock)(priv->hwmon_dev);
> >        |         ^~~~~
> > corsair-psu.c:667:9: error: cleanup argument not a function
> >    667 |         guard(hwmon_lock)(priv->hwmon_dev);
> >        |         ^~~~~
> > 
> 
> You'll need commit d1e720c7328e ("hwmon: Support guard() and scoped_guard for subsystem locks").

Oh, I got that confused. I saw the commit was added to Torvalds repo in May.
But I wasn't aware that it is still not part of the official kernel releases.
Ah, I can just add the two lines from the commit to the headers of my local
kernel build files. I will test it.

greetings,
Wilken

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

[Wilken Gottwalt]

On Sun,  2 Aug 2026 12:36:53 +0000
Ali Ahmet Memis <ali@iusegentoo.com> wrote:

> corsairpsu_request() sends a rail select command and then the actual
> read as two separate transfers, both going through the single shared
> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
> serializes its own callers, but the debugfs files call
> corsairpsu_get_value() directly and never take that lock, so a debugfs
> read can land between another reader's rail select and its value read.
> 
> The result is a value from the wrong rail reported as the right one,
> because corsairpsu_usb_cmd() only checks the command echo and both
> transfers echo the command it expects. It can also make a caller consume
> the reply meant for the other one, since raw_event() writes into the
> shared buffer and completes whoever happens to be waiting.
> 
> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
> on subsystem locking") on the grounds that the subsystem serializes for
> us, which holds for sysfs but not for these files. Take
> the same lock in the debugfs paths that issue commands, using the guard
> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
> for subsystem locks"), as suggested in [1].
> 
> The lock cannot go into corsairpsu_request() itself: the hwmon core
> already holds it across ->read, so every sysfs read would deadlock.
> vendor_show() and product_show() only print strings cached during probe
> and issue no command, and corsairpsu_get_criticals() and
> corsairpsu_check_cmd_support() run before either interface is
> registered, so none of them need it.
> 
> [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
> 
> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
> This is the fix Guenter asked for in the May thread, written the way he
> suggested there. Wilken's patch used a driver private mutex around
> corsairpsu_request(); that thread stalled and the race is still present.
> 
> Wilken, does this cover the chained command case you were worried about?
> As far as I can tell it does: the whole select-rail plus read sequence
> now runs under the same lock the hwmon core takes around ->read, so a
> debugfs reader cannot land in the middle of one. If you had a case in
> mind that this misses, I would rather hear it than guess.
> 
> I have no Corsair PSU, so this is reasoned from the code rather than
> measured on hardware. What I did check:
> 
>   - hwmon_lock() takes hwdev->lock, and the hwmon core takes the same
>     mutex around ->read and ->write, so this really does serialize the
>     two entry points
>   - the lock therefore cannot go into corsairpsu_request(), the sysfs
>     path would deadlock on itself
>   - probe registers the hwmon device before creating the debugfs files
>     and remove tears them down in the opposite order, so priv->hwmon_dev
>     is always valid inside a debugfs read
> 
> Prior discussion:
>   https://lore.kernel.org/all/agR9YW7hGTJ_l7ms@monster.localdomain/
> 
>  drivers/hwmon/corsair-psu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index ce958cdaef58..24100519cd83 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
>  	long val;
>  	int ret;
>  
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, cmd, 0, &val);
>  	if (ret < 0) {
>  		seq_puts(seqf, "N/A\n");
> @@ -730,6 +732,8 @@ static int ocpmode_show(struct seq_file *seqf, void *unused)
>  	 * getting of the value itself can also fail during this. Because of this every other
> value
>  	 * than OCP_MULTI_RAIL can be considered as "single rail".
>  	 */
> +	guard(hwmon_lock)(priv->hwmon_dev);
> +
>  	ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
>  	if (ret < 0)
>  		seq_puts(seqf, "N/A\n");
> 
> base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952

That works so far.

Running tests without the guard basically end all in variantes similar to this:
-----
# cat /sys/class/hwmon/hwmon9/in1_input
12000
# cat /sys/kernel/debug/corsair-psu-0003:1B1C:1C1F.0006/uptime
08:18:30
# cat /sys/class/hwmon/hwmon9/in1_input &; cat /sys/kernel/debug/corsair-psu-0003:1B1C:1C1F.0006/uptime
cat: /sys/class/hwmon/hwmon9/in1_input: Operation not supported
N/A
-----
Sometimes, there is only the "N/A" and sometimes there is only the "Operation not
supported", but most of the time both happen in one try. And very rarely both go
through without an issue. That hints on two different things that can go wrong.
"in1_input" is one of the commands with a rail change, aka rail + request.
The first scenario is that one, where the debugfs exactly goes between the rail
change and the final request. The second scenario is the one, where the 
corsairpsu_get_value() is more or less called at the same time. But the change
fixes both issues.

Several hundred tests with the guard included always result in this:
-----
# cat /sys/class/hwmon/hwmon9/in1_input
12015
# cat /sys/kernel/debug/corsair-psu-0003:1B1C:1C1F.0006/uptime
08:21:48
cat /sys/class/hwmon/hwmon9/in1_input &; cat /sys/kernel/debug/corsair-psu-0003:1B1C:1C1F.0006/uptime
12015
08:21:49
-----

Though, I would like to see the second guard() go before the big comment, beyond
that, it looks fine to me:

Tested-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

greetings,
Wilken

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

[Ali Ahmet Memis]

corsairpsu_request() sends a rail select command and then the actual
read as two separate transfers, both going through the single shared
cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
serializes its own callers, but the debugfs files call
corsairpsu_get_value() directly and never take that lock, so a debugfs
read can land between another reader's rail select and its value read.

The result is a value from the wrong rail reported as the right one,
because corsairpsu_usb_cmd() only checks the command echo and both
transfers echo the command it expects. It can also make a caller consume
the reply meant for the other one, since raw_event() writes into the
shared buffer and completes whoever happens to be waiting.

Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
on subsystem locking") on the grounds that the subsystem serializes for
us, which holds for sysfs but not for these files. Take
the same lock in the debugfs paths that issue commands, using the guard
added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
for subsystem locks"), as suggested in [1].

The lock cannot go into corsairpsu_request() itself: the hwmon core
already holds it across ->read, so every sysfs read would deadlock.
vendor_show() and product_show() only print strings cached during probe
and issue no command, and corsairpsu_get_criticals() and
corsairpsu_check_cmd_support() run before either interface is
registered, so none of them need it.

[1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/

Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
Tested-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
v2: move the guard in ocpmode_show() above the comment, as asked for in
    the test report. No other change.

Test report, with the runs before and after the change:
  https://lore.kernel.org/all/20260806161028.42218ebd@posteo.net/

v1: https://lore.kernel.org/all/20260802123653.19532-1-ali@iusegentoo.com/

 drivers/hwmon/corsair-psu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index ce958cdaef58..033166db6bc4 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -664,6 +664,8 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
 	long val;
 	int ret;
 
+	guard(hwmon_lock)(priv->hwmon_dev);
+
 	ret = corsairpsu_get_value(priv, cmd, 0, &val);
 	if (ret < 0) {
 		seq_puts(seqf, "N/A\n");
@@ -723,6 +725,8 @@ static int ocpmode_show(struct seq_file *seqf, void *unused)
 	long val;
 	int ret;
 
+	guard(hwmon_lock)(priv->hwmon_dev);
+
 	/*
 	 * The rail mode is switchable on the fly. The RAW interface can be used for this. But it
 	 * will not be included here, because I consider it somewhat dangerous for the health of the

base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952
-- 
2.55.0

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

[Guenter Roeck]

On Thu, Aug 06, 2026 at 02:21:39PM +0000, Ali Ahmet Memis wrote:
> corsairpsu_request() sends a rail select command and then the actual
> read as two separate transfers, both going through the single shared
> cmd_buffer and wait_completion in corsairpsu_usb_cmd(). The hwmon core
> serializes its own callers, but the debugfs files call
> corsairpsu_get_value() directly and never take that lock, so a debugfs
> read can land between another reader's rail select and its value read.
> 
> The result is a value from the wrong rail reported as the right one,
> because corsairpsu_usb_cmd() only checks the command echo and both
> transfers echo the command it expects. It can also make a caller consume
> the reply meant for the other one, since raw_event() writes into the
> shared buffer and completes whoever happens to be waiting.
> 
> Locking was dropped in commit 4207069edbf0 ("hwmon: (corsair-psu) Rely
> on subsystem locking") on the grounds that the subsystem serializes for
> us, which holds for sysfs but not for these files. Take
> the same lock in the debugfs paths that issue commands, using the guard
> added in commit d1e720c7328e ("hwmon: Support guard() and scoped_guard
> for subsystem locks"), as suggested in [1].
> 
> The lock cannot go into corsairpsu_request() itself: the hwmon core
> already holds it across ->read, so every sysfs read would deadlock.
> vendor_show() and product_show() only print strings cached during probe
> and issue no command, and corsairpsu_get_criticals() and
> corsairpsu_check_cmd_support() run before either interface is
> registered, so none of them need it.
> 
> [1] https://lore.kernel.org/all/5f0406fa-9692-49f0-bcfe-c013f5fc7b62@roeck-us.net/
> 
> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> Tested-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

Applied.

Thanks,
Guenter