Re: [PATCH 3/4] HID: lg-g15: report Super-key-lock state on G15 v2's keyboard interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] HID: lg-g15: missing HID_QUIRK_INPUT_PER_APP in
  lg_g15_v2_kbd_probe()
- [Medium] HID: lg-g15: initial lock state read after LED registration
  in lg_g15_v2_kbd_probe()
- [Low] HID: lg-g15: race updating super_lock between lg_g15_resume()
  and raw_event
--

commit 429a4d981b469fb0f361faf62d0245c19b75257d
Author: Torstein Eide <torsteine+linux@gmail.com>

HID: lg-g15: report Super-key-lock state on G15 v2's keyboard interface

This commit binds the Logitech G15 v2 keyboard USB device (046D:C226) to
hid-lg-g15 and exposes the Super-key lock state from report 3 as a
read-only g15::game_mode LED while keeping default hid-input handling.

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 35d183787383a..592c9747738e2 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
[ ... ]
> @@ -981,11 +995,138 @@ static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
[ ... ]
> +static void lg_g15_v2_kbd_set_super_lock(struct lg_g15_kbd_data *kbd, bool super_lock)
> +{
> +	if (super_lock == kbd->super_lock)
> +		return;
> +
> +	kbd->super_lock = super_lock;
> +	led_classdev_notify_brightness_hw_changed(&kbd->led, super_lock);
> +}
> +
> +/* Fetches the current lock state, which may already be on at probe/resume. */
> +static void lg_g15_v2_kbd_read_super_lock(struct hid_device *hdev,
> +					  struct lg_g15_kbd_data *kbd)
> +{
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kzalloc(2, GFP_KERNEL); /* DMA-able, not on the stack */
> +	if (!buf)
> +		return;
> +
> +	ret = hid_hw_raw_request(hdev, LG_G15_V2_KBD_GAME_MODE_REPORT, buf, 2,
> +				 HID_INPUT_REPORT, HID_REQ_GET_REPORT);
> +	if (ret == 2 && buf[0] == LG_G15_V2_KBD_GAME_MODE_REPORT)
> +		lg_g15_v2_kbd_set_super_lock(kbd, buf[1] & 0x01);
> +	else
> +		hid_dbg(hdev, "Error reading Super-key-lock state: %d\n", ret);
> +
> +	kfree(buf);
> +}
[ ... ]
> +	kbd->led.name = "g15::game_mode";
> +	kbd->led.max_brightness = 1;
> +	kbd->led.brightness_get = lg_g15_v2_kbd_super_lock_get;
> +	kbd->led.flags = LED_BRIGHT_HW_CHANGED; /* Enables the hw_changed notify below */
> +
> +	ret = devm_led_classdev_register(&hdev->dev, &kbd->led);
> +	if (ret)
> +		return 0; /* Non-fatal, keyboard input still works without the LED */
> +
> +	/*
> +	 * Only set drvdata once the LED is registered: raw_event may run as
> +	 * soon as hid_hw_start() returns, and notifying a brightness change
> +	 * on an unregistered LED triggers a WARN.
> +	 */
> +	hid_set_drvdata(hdev, kbd);
> +	lg_g15_v2_kbd_read_super_lock(hdev, kbd);

[Severity: Medium]
Does registering kbd->led before reading the initial hardware lock state
publish a stale brightness of 0 and emit a spurious brightness_hw_changed
notification when the Super-key lock switch is already on at probe?

Inside devm_led_classdev_register(), led_classdev_register_ext() calls
kbd->led.brightness_get while kbd->super_lock is still false, caches
kbd->led.brightness = 0, initializes kbd->led.brightness_hw_changed to -1
(-ENODATA), and emits KOBJ_ADD.  Userspace readers reacting to the uevent
can observe 0 while the GET_REPORT transfer is in flight, and
lg_g15_v2_kbd_read_super_lock() -> lg_g15_v2_kbd_set_super_lock() then fires
led_classdev_notify_brightness_hw_changed() and sets brightness_hw_changed
to 1 even though no hardware change occurred after registration.

Reading the initial report into kbd->super_lock before calling
devm_led_classdev_register() (similar to lg_g15_get_initial_led_brightness()
in lg_g15_probe()) would register the LED with the actual hardware state.

[ ... ]

> @@ -1492,6 +1638,12 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	struct hid_report *rep;
>  	int ret, i, gkeys = 0;
>  
> +	if (lg_g15_is_v2_kbd(hdev)) /* Not the GamePanel LCD interface */
> +		return lg_g15_v2_kbd_probe(hdev);
> +
> +	if (id->driver_data == LG_G15_V2_KBD) /* e.g. a mismatching new_id */
> +		return -ENODEV;
> +
>  	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;

[Severity: Medium]
Should lg_g15_v2_kbd_probe() also set HID_QUIRK_INPUT_PER_APP on
hdev->quirks before calling hid_parse() and hid_hw_start()?

Previously 046D:C226 was bound by hid-generic, where hid_generic_probe()
sets HID_QUIRK_INPUT_PER_APP:

drivers/hid/hid-generic.c:hid_generic_probe() {
	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;

	ret = hid_parse(hdev);
	if (ret)
		return ret;

	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
}

Because lg_g15_probe() dispatches to lg_g15_v2_kbd_probe() before setting
HID_QUIRK_INPUT_PER_APP, any interface on 046D:C226 with multiple
application collections (such as the consumer-control/extra-report
interface) has all application collections merged into a single input_dev
and loses the per-application input device name suffix, which is a
regression relative to hid-generic.

[ ... ]

> @@ -1772,9 +1947,18 @@ static void lg_g15_remove(struct hid_device *hdev)
>   */
>  static int lg_g15_resume(struct hid_device *hdev)
>  {
> -	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +	struct lg_g15_data *g15;
>  	struct lg_g15_led *contrast;
>  
> +	if (lg_g15_is_v2_kbd(hdev)) {
> +		struct lg_g15_kbd_data *kbd = hid_get_drvdata(hdev);
> +
> +		if (kbd)
> +			lg_g15_v2_kbd_read_super_lock(hdev, kbd);
> +		return 0;
> +	}

[Severity: Low]
Can lg_g15_resume() race with lg_g15_v2_kbd_raw_event() when updating
kbd->super_lock and kbd->led.brightness_hw_changed?

In usbhid, hid_resume_common() and hid_post_reset() call hid_restart_io() to
restart the interrupt IN URB before invoking the HID driver's resume or
reset_resume callback.  If the Super-key lock switch is toggled during
resume, the GET_REPORT control transfer and an interrupt IN report can run
concurrently:

CPU0 (resume)                     CPU1 (URB completion)
lg_g15_resume()
  lg_g15_v2_kbd_read_super_lock()
    hid_hw_raw_request()
      -> reads old state (0)
                                  lg_g15_raw_event()
                                    lg_g15_v2_kbd_raw_event()
                                      lg_g15_v2_kbd_set_super_lock(kbd, 1)
                                        kbd->super_lock = 1;
                                        ..._hw_changed(&kbd->led, 1);
    lg_g15_v2_kbd_set_super_lock(kbd, 0)
      kbd->super_lock = 0;  <-- overwrites newer interrupt state

The two paths can also interleave between assigning kbd->super_lock and
calling led_classdev_notify_brightness_hw_changed(), leaving kbd->super_lock
and kbd->led.brightness_hw_changed with opposite values.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094426.1432465-1-torsteine+linux@gmail.com?part=3