[PATCH] hwmon: (corsair-psu) Close HID device on probe errors

[PATCH] hwmon: (corsair-psu) Close HID device on probe errors

[Myeonghun Pak]

corsairpsu_probe() opens the HID device before sending the device init
and firmware-info commands. If either command fails, the error path jumps
directly to fail_and_stop and skips hid_hw_close().

Use the existing fail_and_close label for those post-open failures so the
open count and low-level close callback are balanced before hid_hw_stop().

Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/hwmon/corsair-psu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index dddbd2463f..76f3e1da68 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -796,13 +796,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
 	ret = corsairpsu_init(priv);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
-		goto fail_and_stop;
+		goto fail_and_close;
 	}
 
 	ret = corsairpsu_fwinfo(priv);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
-		goto fail_and_stop;
+		goto fail_and_close;
 	}
 
 	corsairpsu_get_criticals(priv);
-- 
2.50.1

Re: [PATCH] hwmon: (corsair-psu) Close HID device on probe errors

[sashiko-bot]

Sashiko AI review found 1 potential issue(s):
- [High] Missing required locking for debugfs attributes and shared command buffer.
--

commit f516eb8cbe20556b89a6e098a982bbc4318175c6
Author: Myeonghun Pak <mhun512@gmail.com>

hwmon: (corsair-psu) Close HID device on probe errors

This commit modifies the error handling path in corsairpsu_probe() to
properly balance the open count. It adds a new fail_and_close label so
the HID hardware is closed before stopping when initialization or firmware
queries fail.

> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index dddbd2463f8da7..76f3e1da68d09e 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -796,13 +796,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
>  	ret = corsairpsu_init(priv);

This isn't a bug introduced by this patch, but does the driver lack
required locking for the shared priv->cmd_buffer and priv->wait_completion
when accessed concurrently?

The hwmon guidelines state that drivers must implement locking for attributes
registered outside the core (like debugfs) and should use hwmon_lock() and
hwmon_unlock().

Since corsairpsu_debugfs_init() registers attributes that eventually call
corsairpsu_usb_cmd(), these debugfs reads can execute concurrently with
standard sysfs reads. Inside corsairpsu_usb_cmd(), the driver uses a single
shared buffer:

Thread A:
    memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
    reinit_completion(&priv->wait_completion);
    hid_hw_output_report(...);

Thread B (before Thread A's reply arrives):
    memset(priv->cmd_buffer, 0, CMD_BUFFER_SIZE);
    reinit_completion(&priv->wait_completion);

When the device reply arrives, corsairpsu_raw_event() overwrites
cmd_buffer and calls complete(), which would wake only one waiter.

Can this concurrent access lead to corrupted device communication, missed
wakeups, or returning values for the wrong command?

>  	if (ret < 0) {
>  		dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
> -		goto fail_and_stop;
> +		goto fail_and_close;
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260424135107.13720-1-mhun512@gmail.com?part=1

Re: [PATCH] hwmon: (corsair-psu) Close HID device on probe errors

[Wilken Gottwalt]

On Fri, 24 Apr 2026 22:50:51 +0900
Myeonghun Pak <mhun512@gmail.com> wrote:

> corsairpsu_probe() opens the HID device before sending the device init
> and firmware-info commands. If either command fails, the error path jumps
> directly to fail_and_stop and skips hid_hw_close().
> 
> Use the existing fail_and_close label for those post-open failures so the
> open count and low-level close callback are balanced before hid_hw_stop().
> 
> Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  drivers/hwmon/corsair-psu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index dddbd2463f..76f3e1da68 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -796,13 +796,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id ret = corsairpsu_init(priv);
>  	if (ret < 0) {
>  		dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
> -		goto fail_and_stop;
> +		goto fail_and_close;
>  	}
>  
>  	ret = corsairpsu_fwinfo(priv);
>  	if (ret < 0) {
>  		dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
> -		goto fail_and_stop;
> +		goto fail_and_close;
>  	}
>  
>  	corsairpsu_get_criticals(priv);

I really had to go back to the initial commit to check, if I introduced that
issue later. But nope, I had it that way from the start. Good catch, I totally
missed that. Thank you.

greetings,
Wilken

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

Re: [PATCH] hwmon: (corsair-psu) Close HID device on probe errors

[Guenter Roeck]

On Fri, Apr 24, 2026 at 10:50:51PM +0900, Myeonghun Pak wrote:
> corsairpsu_probe() opens the HID device before sending the device init
> and firmware-info commands. If either command fails, the error path jumps
> directly to fail_and_stop and skips hid_hw_close().
> 
> Use the existing fail_and_close label for those post-open failures so the
> open count and low-level close callback are balanced before hid_hw_stop().
> 
> Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> Reviewed-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>

Applied.

Thanks,
Guenter