Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] hid: redundant client_opened counter due to core refcounting
--
commit 60750525b808a9b3b327f3fb2200f042e05ff888
Author: Vicki Pfau [off-list ref]
HID: hid-steam: Make client_opened a counter
This commit replaces the client_opened boolean flag with a counter to track
the number of open clients. It increments and decrements the counter in the
low-level open and close callbacks to ensure the driver does not incorrectly
assume there are no remaining clients when one of multiple clients closes.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 2f5e9d3e64991..4a0a13b0ae62f 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -634,7 +634,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
struct steam_device *steam = hdev->driver_data;
mutex_lock(&steam->mutex);
- steam->client_opened = true;
+ steam->client_opened++;
[Severity: Low]
This introduces a manual counter increment here, assuming the callback can
be invoked concurrently or multiple times for the same device.
However, the HID core strictly serializes low-level operations using
hdev->ll_open_lock and internally refcounts them via hdev->ll_open_count.
Looking at hid_hw_open() in hid-core.c:
if (!hdev->ll_open_count++) {
ret = hdev->ll_driver->open(hdev);
This guarantees that the driver's .open callback is invoked exactly once on
the 0->1 transition.
Is it possible for steam_client_ll_open() to be called multiple times in an
unbalanced manner, or does the core refcounting make this counter redundant?
quoted hunk ↗ jump to hunk
mutex_unlock(&steam->mutex);
steam_input_unregister(steam);
@@ -654,12 +654,12 @@ static void steam_client_ll_close(struct hid_device *hdev)
spin_unlock_irqrestore(&steam->lock, flags);
mutex_lock(&steam->mutex);
- steam->client_opened = false;
- if (connected)
+ steam->client_opened--;
[Severity: Low]
Similarly, the HID core prevents unbalanced low-level closes.
Looking at hid_hw_close() in hid-core.c:
if (!--hdev->ll_open_count)
hdev->ll_driver->close(hdev);
This ensures the .close callback is invoked exactly once on the 1->0
transition.
The manual decrement relies on the premise that client_opened could
legitimately be greater than 1. With the core refcounting in place, it appears
it cannot. Could this counter be adding unnecessary code complexity based on
the false premise of multiple concurrent opens?
quoted hunk ↗ jump to hunk
- if (connected)
+ if (connected && !steam->client_opened)
steam_set_lizard_mode(steam, lizard_mode);
mutex_unlock(&steam->mutex);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261013091204.529049-1-lee@kernel.org?part=2