Re: [PATCH v6 03/12] Input: xbox_gip - Add controllable LED support
From: sashiko-bot@kernel.org
Date: 2026-09-08 03:39:16
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [High] Registering a managed LED classdev against an input device creates a circular dependency, leading to memory leaks, list corruption, and kernel crashes on device reconnect. Pre-existing issues: - [High] Repeatedly calling `devm_input_allocate_device()` against the USB interface without unbinding leaks `input_dev` structures on every device reconnect. -- commit 170b2e9725cebf9b1d26ed7fb1bf1a2b52d452c6 Author: Vicki Pfau [off-list ref] Input: xbox_gip - Add controllable LED support This commit adds controllable LED support for Xbox One controllers. It exposes both monochrome white and RGBW addressable LEDs as led class devices depending on the controller capabilities.
quoted hunk ↗ jump to hunk
diff --git a/drivers/input/joystick/gip/gip.h b/drivers/input/joystick/gip/gip.h index 605149834269f..69eef001d91ff 100644 --- a/drivers/input/joystick/gip/gip.h +++ b/drivers/input/joystick/gip/gip.h@@ -234,6 +235,10 @@ struct gip_attachment { uint8_t seq_vendor; uint8_t device_state; + union { + struct led_classdev standard; + struct led_classdev_mc color; + } guide_led;
[Severity: High] Does embedding the led_classdev directly inside the long-lived gip_attachment structure risk list corruption? If the input device is reset and re-probed (for example, via a reset message), the driver recreates the device and could call devm_led_classdev_register() again on the exact same already-registered led_classdev struct. This would corrupt the global leds_list and cause a kernel crash.
quoted hunk ↗ jump to hunk
diff --git a/drivers/input/joystick/gip/gip-core.c b/drivers/input/joystick/gip/gip-core.c index 0e23811a6ecc1..a5ed5f3bf3fed 100644 --- a/drivers/input/joystick/gip/gip-core.c +++ b/drivers/input/joystick/gip/gip-core.c
[ ... ]
+static int gip_guide_led_probe(struct gip_attachment *attachment, struct device *dev)
+{[ ... ]
+ if (!rc) + rc = devm_led_classdev_multicolor_register(dev, + mc_cdev);
[Severity: High] Can this create a circular dependency? By using devm_led_classdev_multicolor_register() in gip_guide_led_probe() with the input_dev as the parent device, the LED child device takes a reference to the input_dev. Since input_dev devres actions only execute when its reference count drops to zero, the LED device is never automatically unregistered, creating a circular reference and leaking memory. [ ... ]
+ } else {
+ struct led_classdev *cdev = &attachment->guide_led.standard;[ ... ]
+ rc = devm_led_classdev_register(dev, cdev);
[Severity: High] Similarly, does registering the standard LED classdev against the input device here create the same circular dependency preventing automatic unregistration? [ ... ]
quoted hunk ↗ jump to hunk
@@ -1157,6 +1268,10 @@ static int gip_setup_input_device(struct gip_attachment *attachment) if (rc) goto err_free_device; + rc = gip_guide_led_probe(attachment, &input->dev); + if (rc) + gip_err(attachment, "Failed to register LEDs: %d\n", rc); + return 0;
[Severity: High] This isn't a bug introduced by this patch, but does the allocation of the input device earlier in gip_setup_input_device() leak input_dev structures on every reconnect? The code uses devm_input_allocate_device() with the long-lived USB interface as the parent. When the controller resets (by sending a reset packet), the driver correctly calls input_unregister_device(), but the devm free action remains attached to the USB interface's devres list. A new input_dev is then allocated for the re-initialization, leaking an entire input_dev structure each time until the USB dongle is physically unplugged. [Severity: High] Also, if a controller reset causes gip_setup_input_device() to run again, does calling gip_guide_led_probe() register the LEDs a second time without ensuring the previous LED instances were unregistered? Combined with embedding the led_classdev inside the attachment, this seems like it would trigger list corruption. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260908032145.2118234-1-vi@endrift.com?part=3