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

[sashiko-bot@kernel.org]

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