[PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Wilken Gottwalt]

Fix removed locking mechanism. The locking mechanism does protect
chained commands (set rail + get value), which are two separate calls
to the low level access function. The hwmon (temps for example) and
debugfs (uptimes for example) subsystem can trigger that chain of
commands in parallel. The serialization in the hw monioring core alone
is not enough.

Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
 drivers/hwmon/corsair-psu.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index 76f3e1da68d0..6db899b37ede 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -12,6 +12,7 @@
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
@@ -122,6 +123,7 @@ struct corsairpsu_data {
 	struct device *hwmon_dev;
 	struct dentry *debugfs;
 	struct completion wait_completion;
+	struct mutex lock; /* serializes chained commands and parallel debugfs/hwmon access */
 	u8 *cmd_buffer;
 	char vendor[REPLY_SIZE];
 	char product[REPLY_SIZE];
@@ -194,12 +196,14 @@ static int corsairpsu_init(struct corsairpsu_data *priv)
 	/*
 	 * PSU_CMD_INIT uses swapped length/command and expects 2 parameter bytes, this command
 	 * actually generates a reply, but we don't need it
+	 * only runs during probe/resume and does not switch rails, no locking required
 	 */
 	return corsairpsu_usb_cmd(priv, PSU_CMD_INIT, 3, 0, NULL);
 }
 
 static int corsairpsu_fwinfo(struct corsairpsu_data *priv)
 {
+	/* only runs in probe and does not switch rails, no locking required */
 	int ret;
 
 	ret = corsairpsu_usb_cmd(priv, 3, PSU_CMD_VEND_STR, 0, priv->vendor);
@@ -217,6 +221,7 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
 {
 	int ret;
 
+	mutex_lock(&priv->lock);
 	switch (cmd) {
 	case PSU_CMD_RAIL_VOLTS_HCRIT:
 	case PSU_CMD_RAIL_VOLTS_LCRIT:
@@ -226,13 +231,17 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
 	case PSU_CMD_RAIL_WATTS:
 		ret = corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL);
 		if (ret < 0)
-			return ret;
+			goto cmd_fail;
 		break;
 	default:
 		break;
 	}
 
-	return corsairpsu_usb_cmd(priv, 3, cmd, 0, data);
+	ret = corsairpsu_usb_cmd(priv, 3, cmd, 0, data);
+
+cmd_fail:
+	mutex_unlock(&priv->lock);
+	return ret;
 }
 
 static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, long *val)
@@ -789,6 +798,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 
 	priv->hdev = hdev;
 	hid_set_drvdata(hdev, priv);
+	mutex_init(&priv->lock);
 	init_completion(&priv->wait_completion);
 
 	hid_device_io_start(hdev);
-- 
2.54.0

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Guenter Roeck]

On 5/13/26 06:32, Wilken Gottwalt wrote:
> Fix removed locking mechanism. The locking mechanism does protect
> chained commands (set rail + get value), which are two separate calls
> to the low level access function. The hwmon (temps for example) and
> debugfs (uptimes for example) subsystem can trigger that chain of
> commands in parallel. The serialization in the hw monioring core alone
> is not enough.
> 
> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

You'll need to explain why using the subsystem lock for debugfs
accesses does not work.

Guenter

> ---
>   drivers/hwmon/corsair-psu.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index 76f3e1da68d0..6db899b37ede 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -12,6 +12,7 @@
>   #include <linux/jiffies.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/mutex.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
>   
> @@ -122,6 +123,7 @@ struct corsairpsu_data {
>   	struct device *hwmon_dev;
>   	struct dentry *debugfs;
>   	struct completion wait_completion;
> +	struct mutex lock; /* serializes chained commands and parallel debugfs/hwmon access */
>   	u8 *cmd_buffer;
>   	char vendor[REPLY_SIZE];
>   	char product[REPLY_SIZE];
> @@ -194,12 +196,14 @@ static int corsairpsu_init(struct corsairpsu_data *priv)
>   	/*
>   	 * PSU_CMD_INIT uses swapped length/command and expects 2 parameter bytes, this command
>   	 * actually generates a reply, but we don't need it
> +	 * only runs during probe/resume and does not switch rails, no locking required
>   	 */
>   	return corsairpsu_usb_cmd(priv, PSU_CMD_INIT, 3, 0, NULL);
>   }
>   
>   static int corsairpsu_fwinfo(struct corsairpsu_data *priv)
>   {
> +	/* only runs in probe and does not switch rails, no locking required */
>   	int ret;
>   
>   	ret = corsairpsu_usb_cmd(priv, 3, PSU_CMD_VEND_STR, 0, priv->vendor);
> @@ -217,6 +221,7 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
>   {
>   	int ret;
>   
> +	mutex_lock(&priv->lock);
>   	switch (cmd) {
>   	case PSU_CMD_RAIL_VOLTS_HCRIT:
>   	case PSU_CMD_RAIL_VOLTS_LCRIT:
> @@ -226,13 +231,17 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
>   	case PSU_CMD_RAIL_WATTS:
>   		ret = corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL);
>   		if (ret < 0)
> -			return ret;
> +			goto cmd_fail;
>   		break;
>   	default:
>   		break;
>   	}
>   
> -	return corsairpsu_usb_cmd(priv, 3, cmd, 0, data);
> +	ret = corsairpsu_usb_cmd(priv, 3, cmd, 0, data);
> +
> +cmd_fail:
> +	mutex_unlock(&priv->lock);
> +	return ret;
>   }
>   
>   static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, long *val)
> @@ -789,6 +798,7 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
>   
>   	priv->hdev = hdev;
>   	hid_set_drvdata(hdev, priv);
> +	mutex_init(&priv->lock);
>   	init_completion(&priv->wait_completion);
>   
>   	hid_device_io_start(hdev);

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Guenter Roeck]

On 5/13/26 06:43, Guenter Roeck wrote:
> On 5/13/26 06:32, Wilken Gottwalt wrote:
>> Fix removed locking mechanism. The locking mechanism does protect
>> chained commands (set rail + get value), which are two separate calls
>> to the low level access function. The hwmon (temps for example) and
>> debugfs (uptimes for example) subsystem can trigger that chain of
>> commands in parallel. The serialization in the hw monioring core alone
>> is not enough.
>>
>> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> 
> You'll need to explain why using the subsystem lock for debugfs
> accesses does not work.
> 

Clarifying: "Why using hwmon_lock() / hwmon_unlock() in the debugfs functions
would be insufficient".

Guenter

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Wilken Gottwalt]

On Wed, 13 May 2026 07:05:41 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 5/13/26 06:43, Guenter Roeck wrote:
> > On 5/13/26 06:32, Wilken Gottwalt wrote:
> >> Fix removed locking mechanism. The locking mechanism does protect
> >> chained commands (set rail + get value), which are two separate calls
> >> to the low level access function. The hwmon (temps for example) and
> >> debugfs (uptimes for example) subsystem can trigger that chain of
> >> commands in parallel. The serialization in the hw monioring core alone
> >> is not enough.
> >>
> >> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> >> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > 
> > You'll need to explain why using the subsystem lock for debugfs
> > accesses does not work.
> > 
> 
> Clarifying: "Why using hwmon_lock() / hwmon_unlock() in the debugfs functions
> would be insufficient".

Yes, I understand that. You gave me an idea for a nice "hack" that would
demonstrate the problem. I will try it and look if it really happens.
Though, my thought process is, that debugfs and hwmon are two subsystems
which do not run in the same thread context. Each one of them would
trigger a call to corsairpsu_request(), one comming from a *_show callback
of the debufs and one comming from a hwmon_ops.read callback.
corsairpsu_request() often calls corsairpsu_usb_cmd() twice, one for
setting the rail, the second for reading a value of the rail. The mutex
protects that chain of calls. I really don't think that the debugfs
callbacks are serailized against the hwmon callbacks. I could create a
nice demonstration if I passthrough a little hint via function parameters
from where a callback is triggered. Do you understand, what I mean?

greetings,
Wilken

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Guenter Roeck]

On 5/13/26 07:21, Wilken Gottwalt wrote:
> On Wed, 13 May 2026 07:05:41 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 5/13/26 06:43, Guenter Roeck wrote:
>>> On 5/13/26 06:32, Wilken Gottwalt wrote:
>>>> Fix removed locking mechanism. The locking mechanism does protect
>>>> chained commands (set rail + get value), which are two separate calls
>>>> to the low level access function. The hwmon (temps for example) and
>>>> debugfs (uptimes for example) subsystem can trigger that chain of
>>>> commands in parallel. The serialization in the hw monioring core alone
>>>> is not enough.
>>>>
>>>> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
>>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
>>>
>>> You'll need to explain why using the subsystem lock for debugfs
>>> accesses does not work.
>>>
>>
>> Clarifying: "Why using hwmon_lock() / hwmon_unlock() in the debugfs functions
>> would be insufficient".
> 
> Yes, I understand that. You gave me an idea for a nice "hack" that would
> demonstrate the problem. I will try it and look if it really happens.
> Though, my thought process is, that debugfs and hwmon are two subsystems
> which do not run in the same thread context. Each one of them would
> trigger a call to corsairpsu_request(), one comming from a *_show callback
> of the debufs and one comming from a hwmon_ops.read callback.
> corsairpsu_request() often calls corsairpsu_usb_cmd() twice, one for
> setting the rail, the second for reading a value of the rail. The mutex
> protects that chain of calls. I really don't think that the debugfs
> callbacks are serailized against the hwmon callbacks. I could create a
> nice demonstration if I passthrough a little hint via function parameters
> from where a callback is triggered. Do you understand, what I mean?
> 

No. That does not explain why calling hwmon_lock() / hwmon_unlock()
would not work when called from the debugfs functions.

Guenter

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Wilken Gottwalt]

On Wed, 13 May 2026 07:58:14 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 5/13/26 07:21, Wilken Gottwalt wrote:
> > On Wed, 13 May 2026 07:05:41 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> On 5/13/26 06:43, Guenter Roeck wrote:
> >>> On 5/13/26 06:32, Wilken Gottwalt wrote:
> >>>> Fix removed locking mechanism. The locking mechanism does protect
> >>>> chained commands (set rail + get value), which are two separate calls
> >>>> to the low level access function. The hwmon (temps for example) and
> >>>> debugfs (uptimes for example) subsystem can trigger that chain of
> >>>> commands in parallel. The serialization in the hw monioring core alone
> >>>> is not enough.
> >>>>
> >>>> Fixes: 4207069edbf0 ("hwmon: (corsair-psu) Rely on subsystem locking")
> >>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> >>>
> >>> You'll need to explain why using the subsystem lock for debugfs
> >>> accesses does not work.
> >>>
> >>
> >> Clarifying: "Why using hwmon_lock() / hwmon_unlock() in the debugfs functions
> >> would be insufficient".
> > 
> > Yes, I understand that. You gave me an idea for a nice "hack" that would
> > demonstrate the problem. I will try it and look if it really happens.
> > Though, my thought process is, that debugfs and hwmon are two subsystems
> > which do not run in the same thread context. Each one of them would
> > trigger a call to corsairpsu_request(), one comming from a *_show callback
> > of the debufs and one comming from a hwmon_ops.read callback.
> > corsairpsu_request() often calls corsairpsu_usb_cmd() twice, one for
> > setting the rail, the second for reading a value of the rail. The mutex
> > protects that chain of calls. I really don't think that the debugfs
> > callbacks are serailized against the hwmon callbacks. I could create a
> > nice demonstration if I passthrough a little hint via function parameters
> > from where a callback is triggered. Do you understand, what I mean?
> > 
> 
> No. That does not explain why calling hwmon_lock() / hwmon_unlock()
> would not work when called from the debugfs functions.

Okay, that will get a bit complex now, because I added my hack and I see
exactly what I assumed is happening.

What I did: I let init functions (only running once during probe()) pass
through an 'i' to corsairpsu_request() (where the mutex should be, but is
not anymore). Callbacks from the debugfs (uptime in that case) pass through
a 'd', hwmon functions (12v line, triggers a rail change) pass through a
'h'. I do two printk() in corsairpsu_request(), one beforce the first
corsairpsu_usb_cmd() (the rail change) and the second corsairpsu_usb_cmd(),
which runs the command that collects values. Both printk also print the
rail number "rX".

This is the result of of the driver where the mutex is removed in
corsairpsu_request():
--- probe() ---
[41458.336453] corsair-psu 0003:1B1C:1C1F.0006: hidraw5: USB HID v1.11 Device [CORSAIR HX1500i
Power Supply] on usb-0000:2c:00.1-5.4.2/input0 [41458.393231] corsairpsu_request: cmd 2 (r0)(i)
[41458.395240] corsairpsu_request: cmd 2 (r1)(i)
[41458.397215] corsairpsu_request: cmd 1 (r0)(i)
[41458.399201] corsairpsu_request: cmd 2 (r0)(i)
[41458.401198] corsairpsu_request: cmd 1 (r0)(i)
[41458.403198] corsairpsu_request: cmd 2 (r0)(i)
[41458.405198] corsairpsu_request: cmd 1 (r0)(i)
[41458.407197] corsairpsu_request: cmd 2 (r0)(i)
[41458.409197] corsairpsu_request: cmd 1 (r1)(i)
[41458.411198] corsairpsu_request: cmd 2 (r1)(i)
[41458.413198] corsairpsu_request: cmd 1 (r1)(i)
[41458.415197] corsairpsu_request: cmd 2 (r1)(i)
[41458.417196] corsairpsu_request: cmd 1 (r1)(i)
[41458.419196] corsairpsu_request: cmd 2 (r1)(i)
[41458.421196] corsairpsu_request: cmd 1 (r2)(i)
[41458.423196] corsairpsu_request: cmd 2 (r2)(i)
[41458.425196] corsairpsu_request: cmd 1 (r2)(i)
[41458.427198] corsairpsu_request: cmd 2 (r2)(i)
[41458.429198] corsairpsu_request: cmd 1 (r2)(i)
[41458.431198] corsairpsu_request: cmd 2 (r2)(i)
[41458.433199] corsairpsu_request: cmd 2 (r0)(i)

--- single read of v12 line ---
[41488.217072] corsairpsu_request: cmd 2 (r0)(h)
[41488.219077] corsairpsu_request: cmd 2 (r0)(h)

--- parallel read of v12 line and debugfs uptime ---
[41520.169651] corsairpsu_request: cmd 1 (r0)(h)
[41520.170485] corsairpsu_request: cmd 2 (r0)(d)

the single v12 read:
# cat /sys/class/hwmon/hwmon8/in1_input
11984

the parallel read:
# cat /sys/class/hwmon/hwmon8/in1_input &; cat
/sys/kernel/debug/corsair-psu-0003:1B1C:1C1F.0006/uptime [1] 192598
cat: /sys/class/hwmon/hwmon8/in1_input: Operation not supported
[1]  + 192598 exit 1     cat /sys/class/hwmon/hwmon8/in1_input
11:30:22

The parallel read does not succeed. The second part of the v12 line read
after the rail switch, the second call to corsairpsu_request() is blocked
by the call from the debugfs uptime read. The debugfs callback gets exactly
between the first (rail change) and the second call (get value). It is
some form of race condition.

The last part should look like this.
---- read of v12 line and debugfs uptime ---
[41520.169651] corsairpsu_request: cmd 1 (r0)(h)
[41520.169XXX] corsairpsu_request: cmd 2 (r0)(h)
[41520.170485] corsairpsu_request: cmd 2 (r0)(d)



Now let me show you how it looks like if the removed mutex is back in
corsairpsu_request() protecting the chained commands.

--- probe ---
[42898.208992] corsair-psu 0003:1B1C:1C1F.0006: hidraw5: USB HID v1.11 Device [CORSAIR HX1500i Power Supply] on usb-0000:2c:00.1-5.4.2/input0
[42898.266492] corsairpsu_request: cmd 2 (r0)(i)
[42898.268492] corsairpsu_request: cmd 2 (r1)(i)
[42898.270492] corsairpsu_request: cmd 1 (r0)(i)
[42898.272491] corsairpsu_request: cmd 2 (r0)(i)
[42898.274491] corsairpsu_request: cmd 1 (r0)(i)
[42898.276494] corsairpsu_request: cmd 2 (r0)(i)
[42898.278492] corsairpsu_request: cmd 1 (r0)(i)
[42898.280491] corsairpsu_request: cmd 2 (r0)(i)
[42898.282493] corsairpsu_request: cmd 1 (r1)(i)
[42898.284489] corsairpsu_request: cmd 2 (r1)(i)
[42898.286493] corsairpsu_request: cmd 1 (r1)(i)
[42898.288495] corsairpsu_request: cmd 2 (r1)(i)
[42898.290504] corsairpsu_request: cmd 1 (r1)(i)
[42898.292499] corsairpsu_request: cmd 2 (r1)(i)
[42898.294498] corsairpsu_request: cmd 1 (r2)(i)
[42898.296505] corsairpsu_request: cmd 2 (r2)(i)
[42898.298495] corsairpsu_request: cmd 1 (r2)(i)
[42898.300493] corsairpsu_request: cmd 2 (r2)(i)
[42898.302494] corsairpsu_request: cmd 1 (r2)(i)
[42898.304493] corsairpsu_request: cmd 2 (r2)(i)
[42898.306491] corsairpsu_request: cmd 2 (r0)(i)

--- single read of v12 line ---
[43003.146035] corsairpsu_request: cmd 1 (r0)(h)
[43003.148029] corsairpsu_request: cmd 2 (r0)(h)

--- parallel read of v12 line and debugfs uptime ---
[43113.129615] corsairpsu_request: cmd 1 (r0)(h)
[43113.131515] corsairpsu_request: cmd 2 (r0)(h)
[43113.133503] corsairpsu_request: cmd 2 (r0)(d)

And now the parallel read succeeds, no blocking, because the chained
commands are protected by the mutex. I get the v12 line value and the
uptime.

# cat /sys/class/hwmon/hwmon8/in1_input &; cat /sys/kernel/debug/corsair-psu-0003:1B1C:1C1F.0006/uptime
[1] 196389
11984
[1]  + 196389 done       cat /sys/class/hwmon/hwmon8/in1_input
11:56:47

If this does not explain the obvious issue, I have not idea how explain
it further. My English is limited. This is a HID driver with data gathering
functions running in the context of the USB-HID context. Callbacks from the
hwmon and the debugfs subsystem call these data gathering functions, and the
first function in that context, corsairpsu_request(), which can run several
instances in paralellel, needs the mutex.

greetings,
Wilken

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Guenter Roeck]

On Wed, May 13, 2026 at 03:53:51PM +0000, Wilken Gottwalt wrote:
> On Wed, 13 May 2026 07:58:14 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
...
> Okay, that will get a bit complex now, because I added my hack and I see
> exactly what I assumed is happening.
> 
...
> 
> If this does not explain the obvious issue, I have not idea how explain
> it further. My English is limited. This is a HID driver with data gathering
> functions running in the context of the USB-HID context. Callbacks from the
> hwmon and the debugfs subsystem call these data gathering functions, and the
> first function in that context, corsairpsu_request(), which can run several
> instances in paralellel, needs the mutex.
> 

You don't explain why the patches below are insufficient.

I used guard() to keep the changes simple, but hwmon_lock() / hwmon_unlock()
would be similar. Please provide evidence that this does not work.

Thanks,
Guenter
--
From aa3ec1484bdd619e8fa2ce569ec653d35fbf3615 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 13 May 2026 07:14:33 -0700
Subject: [PATCH 1/4] hwmon: Support guard() and scoped_guard for subsystem
 locks

Add support for guard() and scoped_guard() for the hwmon subsystem lock
to simplify its use.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/hwmon/hwmon-kernel-api.rst | 7 ++++---
 include/linux/hwmon.h                    | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
index 1d7f1397a827..9fcde32a140d 100644
--- a/Documentation/hwmon/hwmon-kernel-api.rst
+++ b/Documentation/hwmon/hwmon-kernel-api.rst
@@ -85,9 +85,10 @@ removal.
 When using ``[devm_]hwmon_device_register_with_info()`` to register the
 hardware monitoring device, accesses using the associated access functions
 are serialised by the hardware monitoring core. If a driver needs locking
-for other functions such as interrupt handlers or for attributes which are
-fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
-to ensure that calls to those functions are serialized.
+for other functions such as interrupt handlers, attributes which are fully
+implemented in the driver, or debugfs functions, hwmon_lock() and hwmon_unlock()
+can be used to ensure that calls to those functions are serialized. Those
+functions also support guard() and scoped_guard() variants.
 
 Using devm_hwmon_device_register_with_info()
 --------------------------------------------
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 301a83afbd66..04959e044fd0 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -495,6 +495,8 @@ char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
 void hwmon_lock(struct device *dev);
 void hwmon_unlock(struct device *dev);
 
+DEFINE_GUARD(hwmon_lock, struct device *, hwmon_lock(_T), hwmon_unlock(_T))
+
 /**
  * hwmon_is_bad_char - Is the char invalid in a hwmon name
  * @ch: the char to be considered
-- 
2.45.2

---
From bf0a3d1a69d123eee3126f6a360f0ee3e54f7b17 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 13 May 2026 09:25:46 -0700
Subject: [PATCH] hwmon: (corsair-psu) Protect debugfs accesses with subsystem
 lock

Debugfs accesses need to be mutext protected. Acquire hwmon
subsystem lock to avoid race conditions against hwmon sysfs
accesses.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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 76f3e1da68d0..4a456ba44a9b 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
-- 
2.45.2

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Wilken Gottwalt]

On Wed, 13 May 2026 09:42:08 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Wed, May 13, 2026 at 03:53:51PM +0000, Wilken Gottwalt wrote:
> > On Wed, 13 May 2026 07:58:14 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> ...
> > Okay, that will get a bit complex now, because I added my hack and I see
> > exactly what I assumed is happening.
> > 
> ...
> > 
> > If this does not explain the obvious issue, I have not idea how explain
> > it further. My English is limited. This is a HID driver with data gathering
> > functions running in the context of the USB-HID context. Callbacks from the
> > hwmon and the debugfs subsystem call these data gathering functions, and the
> > first function in that context, corsairpsu_request(), which can run several
> > instances in paralellel, needs the mutex.
> > 
> 
> You don't explain why the patches below are insufficient.
> 
> I used guard() to keep the changes simple, but hwmon_lock() / hwmon_unlock()
> would be similar. Please provide evidence that this does not work.
> 
> Thanks,
> Guenter
> --
> From aa3ec1484bdd619e8fa2ce569ec653d35fbf3615 Mon Sep 17 00:00:00 2001
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Wed, 13 May 2026 07:14:33 -0700
> Subject: [PATCH 1/4] hwmon: Support guard() and scoped_guard for subsystem
>  locks
> 
> Add support for guard() and scoped_guard() for the hwmon subsystem lock
> to simplify its use.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  Documentation/hwmon/hwmon-kernel-api.rst | 7 ++++---
>  include/linux/hwmon.h                    | 2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
> index 1d7f1397a827..9fcde32a140d 100644
> --- a/Documentation/hwmon/hwmon-kernel-api.rst
> +++ b/Documentation/hwmon/hwmon-kernel-api.rst
> @@ -85,9 +85,10 @@ removal.
>  When using ``[devm_]hwmon_device_register_with_info()`` to register the
>  hardware monitoring device, accesses using the associated access functions
>  are serialised by the hardware monitoring core. If a driver needs locking
> -for other functions such as interrupt handlers or for attributes which are
> -fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
> -to ensure that calls to those functions are serialized.
> +for other functions such as interrupt handlers, attributes which are fully
> +implemented in the driver, or debugfs functions, hwmon_lock() and hwmon_unlock()
> +can be used to ensure that calls to those functions are serialized. Those
> +functions also support guard() and scoped_guard() variants.
>  
>  Using devm_hwmon_device_register_with_info()
>  --------------------------------------------
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 301a83afbd66..04959e044fd0 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -495,6 +495,8 @@ char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
>  void hwmon_lock(struct device *dev);
>  void hwmon_unlock(struct device *dev);
>  
> +DEFINE_GUARD(hwmon_lock, struct device *, hwmon_lock(_T), hwmon_unlock(_T))
> +
>  /**
>   * hwmon_is_bad_char - Is the char invalid in a hwmon name
>   * @ch: the char to be considered

Now I am completely confused. What is that guard() and scoped_guard() patch?
It is not part of Torvalds repo or your
git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git repo. Not
in branch hwmon, hwmon-next, hwmon-playground, hwmon-staging, testing, fixes
or master.
I mean, it is a bit hard to check against this, if it is not upstream. Did
you maybe forgot to push it? I really fetched everything to be on the
latest commits and grepped for "DEFINE_GUARD". The only one I see is the
one in drivers/hwmon/pmbus/pmbus.h.

greetings,
Wilken

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Guenter Roeck]

On 5/13/26 10:50, Wilken Gottwalt wrote:
> On Wed, 13 May 2026 09:42:08 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Wed, May 13, 2026 at 03:53:51PM +0000, Wilken Gottwalt wrote:
>>> On Wed, 13 May 2026 07:58:14 -0700
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>> ...
>>> Okay, that will get a bit complex now, because I added my hack and I see
>>> exactly what I assumed is happening.
>>>
>> ...
>>>
>>> If this does not explain the obvious issue, I have not idea how explain
>>> it further. My English is limited. This is a HID driver with data gathering
>>> functions running in the context of the USB-HID context. Callbacks from the
>>> hwmon and the debugfs subsystem call these data gathering functions, and the
>>> first function in that context, corsairpsu_request(), which can run several
>>> instances in paralellel, needs the mutex.
>>>
>>
>> You don't explain why the patches below are insufficient.
>>
>> I used guard() to keep the changes simple, but hwmon_lock() / hwmon_unlock()
>> would be similar. Please provide evidence that this does not work.
>>
>> Thanks,
>> Guenter
>> --
>>  From aa3ec1484bdd619e8fa2ce569ec653d35fbf3615 Mon Sep 17 00:00:00 2001
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Wed, 13 May 2026 07:14:33 -0700
>> Subject: [PATCH 1/4] hwmon: Support guard() and scoped_guard for subsystem
>>   locks
>>
>> Add support for guard() and scoped_guard() for the hwmon subsystem lock
>> to simplify its use.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   Documentation/hwmon/hwmon-kernel-api.rst | 7 ++++---
>>   include/linux/hwmon.h                    | 2 ++
>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst
>> index 1d7f1397a827..9fcde32a140d 100644
>> --- a/Documentation/hwmon/hwmon-kernel-api.rst
>> +++ b/Documentation/hwmon/hwmon-kernel-api.rst
>> @@ -85,9 +85,10 @@ removal.
>>   When using ``[devm_]hwmon_device_register_with_info()`` to register the
>>   hardware monitoring device, accesses using the associated access functions
>>   are serialised by the hardware monitoring core. If a driver needs locking
>> -for other functions such as interrupt handlers or for attributes which are
>> -fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
>> -to ensure that calls to those functions are serialized.
>> +for other functions such as interrupt handlers, attributes which are fully
>> +implemented in the driver, or debugfs functions, hwmon_lock() and hwmon_unlock()
>> +can be used to ensure that calls to those functions are serialized. Those
>> +functions also support guard() and scoped_guard() variants.
>>   
>>   Using devm_hwmon_device_register_with_info()
>>   --------------------------------------------
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index 301a83afbd66..04959e044fd0 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -495,6 +495,8 @@ char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
>>   void hwmon_lock(struct device *dev);
>>   void hwmon_unlock(struct device *dev);
>>   
>> +DEFINE_GUARD(hwmon_lock, struct device *, hwmon_lock(_T), hwmon_unlock(_T))
>> +
>>   /**
>>    * hwmon_is_bad_char - Is the char invalid in a hwmon name
>>    * @ch: the char to be considered
> 
> Now I am completely confused. What is that guard() and scoped_guard() patch?

Why do you think I attached it ? Why would I do that if it was already upstream ?
I wrote it this morning, that is why. You'd find it on the mailing list if
you looked.

Ok, fine, I'll send another patch without it if you don't want to apply it
even for testing.

Guenter

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Wilken Gottwalt]

On Wed, 13 May 2026 11:10:26 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 5/13/26 10:50, Wilken Gottwalt wrote:
> > On Wed, 13 May 2026 09:42:08 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> On Wed, May 13, 2026 at 03:53:51PM +0000, Wilken Gottwalt wrote:
> >>> On Wed, 13 May 2026 07:58:14 -0700
> >>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>
> >> ...
> >>> Okay, that will get a bit complex now, because I added my hack and I see
> >>> exactly what I assumed is happening.
> >>>
> >> ...
> >>>
> >>> If this does not explain the obvious issue, I have not idea how explain
> >>> it further. My English is limited. This is a HID driver with data gathering
> >>> functions running in the context of the USB-HID context. Callbacks from the
> >>> hwmon and the debugfs subsystem call these data gathering functions, and the
> >>> first function in that context, corsairpsu_request(), which can run several
> >>> instances in paralellel, needs the mutex.
> >>>
> >>
> >> You don't explain why the patches below are insufficient.
> >>
> >> I used guard() to keep the changes simple, but hwmon_lock() / hwmon_unlock()
> >> would be similar. Please provide evidence that this does not work.
> >>
> >> Thanks,
> >> Guenter
> >> --
> >>  From aa3ec1484bdd619e8fa2ce569ec653d35fbf3615 Mon Sep 17 00:00:00 2001
> >> From: Guenter Roeck <linux@roeck-us.net>
> >> Date: Wed, 13 May 2026 07:14:33 -0700
> >> Subject: [PATCH 1/4] hwmon: Support guard() and scoped_guard for subsystem
> >>   locks
> >>
> >> Add support for guard() and scoped_guard() for the hwmon subsystem lock
> >> to simplify its use.
> >>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >>   Documentation/hwmon/hwmon-kernel-api.rst | 7 ++++---
> >>   include/linux/hwmon.h                    | 2 ++
> >>   2 files changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst
> >> b/Documentation/hwmon/hwmon-kernel-api.rst index 1d7f1397a827..9fcde32a140d 100644
> >> --- a/Documentation/hwmon/hwmon-kernel-api.rst
> >> +++ b/Documentation/hwmon/hwmon-kernel-api.rst
> >> @@ -85,9 +85,10 @@ removal.
> >>   When using ``[devm_]hwmon_device_register_with_info()`` to register the
> >>   hardware monitoring device, accesses using the associated access functions
> >>   are serialised by the hardware monitoring core. If a driver needs locking
> >> -for other functions such as interrupt handlers or for attributes which are
> >> -fully implemented in the driver, hwmon_lock() and hwmon_unlock() can be used
> >> -to ensure that calls to those functions are serialized.
> >> +for other functions such as interrupt handlers, attributes which are fully
> >> +implemented in the driver, or debugfs functions, hwmon_lock() and hwmon_unlock()
> >> +can be used to ensure that calls to those functions are serialized. Those
> >> +functions also support guard() and scoped_guard() variants.
> >>   
> >>   Using devm_hwmon_device_register_with_info()
> >>   --------------------------------------------
> >> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> >> index 301a83afbd66..04959e044fd0 100644
> >> --- a/include/linux/hwmon.h
> >> +++ b/include/linux/hwmon.h
> >> @@ -495,6 +495,8 @@ char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
> >>   void hwmon_lock(struct device *dev);
> >>   void hwmon_unlock(struct device *dev);
> >>   
> >> +DEFINE_GUARD(hwmon_lock, struct device *, hwmon_lock(_T), hwmon_unlock(_T))
> >> +
> >>   /**
> >>    * hwmon_is_bad_char - Is the char invalid in a hwmon name
> >>    * @ch: the char to be considered
> > 
> > Now I am completely confused. What is that guard() and scoped_guard() patch?
> 
> Why do you think I attached it ? Why would I do that if it was already upstream ?
> I wrote it this morning, that is why. You'd find it on the mailing list if
> you looked.
> 
> Ok, fine, I'll send another patch without it if you don't want to apply it
> even for testing.

No no, don't get me wrong. I just was confused about what you wanted.
And what me really confused, was the change in include/linux/hwmon.h. I
just couldn't imagine, that you proposed to change a subsystem header.
I really try to avoid to touch code, that may affect other drivers
outside of mine. Though, I will try it, but I still need to read into
this and understand it completely. I also may need to rethink how to
better deal with the with the RAW HID interface, which currently shares
the cmd_buffer and the completion with the normal HID operation.

greetings,
Wilken

Re: [PATCH] hwmon: corsair-psu: fix and readd locking of command buffer

[Guenter Roeck]

On Wed, May 13, 2026 at 05:50:30PM +0000, Wilken Gottwalt wrote:
> On Wed, 13 May 2026 09:42:08 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> Now I am completely confused. What is that guard() and scoped_guard() patch?

Again, with subsystem locks instead of guard().

Guenter
---
From bafbb2e301621335381209e395fa281a395ffc53 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 13 May 2026 09:25:46 -0700
Subject: [PATCH] hwmon: (corsair-psu) Protect debugfs accesses with subsystem
 lock

Debugfs accesses need to be mutext protected. Acquire hwmon subsystem
lock where needed to avoid race conditions against hwmon sysfs accesses.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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 76f3e1da68d0..866e798d553c 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -664,7 +664,9 @@ static void print_uptime(struct seq_file *seqf, u8 cmd)
 	long val;
 	int ret;
 
+	hwmon_lock(priv->hwmon_dev);
 	ret = corsairpsu_get_value(priv, cmd, 0, &val);
+	hwmon_unlock(priv->hwmon_dev);
 	if (ret < 0) {
 		seq_puts(seqf, "N/A\n");
 		return;
@@ -730,7 +732,9 @@ 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".
 	 */
+	hwmon_lock(priv->hwmon_dev);
 	ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
+	hwmon_unlock(priv->hwmon_dev);
 	if (ret < 0)
 		seq_puts(seqf, "N/A\n");
 	else
-- 
2.45.2