Re: [PATCH v5 4/5] HID: asus: add support for xgm led

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] A failure to initialize the cosmetic XG Mobile LED aborts the entire HID device probe.
- [High] Failing to restore the XGM LED brightness causes the entire HID device resume process to fail.

Pre-existing issues:
- [High] The devm-managed battery is not explicitly unregistered before stopping the hardware, leading to use-after-stop during teardown.
--

commit 7aa41eaa0bd3e715bef4c850703c7d47445cad14
Author: Denis Benato <denis.benato@linux.dev>

HID: asus: add support for xgm led
    
XG mobile stations have very bright leds behind the fan that can be
turned either ON or OFF: add a cled interface to allow controlling the
brightness of those red leds.

> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 0fb8cd6437b77..06e4888f090ca 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

[ ... ]

> @@ -1361,7 +1432,17 @@ static int __maybe_unused asus_resume(struct hid_device *hdev)
>  	if (drvdata->listener.brightness_set)
>  		asus_kbd_backlight_set(&drvdata->listener, drvdata->kbd_backlight_brightness);
>  
> +	if (drvdata->xgm_led) {
> +		ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +		if (ret) {
> +			hid_err(hdev, "Asus failed to restore xgm brightness: %d\n", ret);
> +			goto asus_resume_err;
> +		}
> +	}
> +
>  	return 0;
> +asus_resume_err:
> +	return ret;

[Severity: High]
In asus_resume(), if asus_xgm_led_set() fails to restore the LED state,
returning an error causes the entire HID device resume process to fail.

Will this leave the device in an inconsistent state and break input
functionality?

As documented in the comments for the backlight, non-fatal LED state
restorations shouldn't fail the system resume. Could this just ignore the error
here as well?

[ ... ]

> @@ -1491,6 +1572,16 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		}
>  	}
>  
> +	if (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID) &&
> +	    ((hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2022) ||
> +	     (hdev->product == USB_DEVICE_ID_ASUSTEK_XGM_2023))) {
> +		ret = asus_xgm_init(hdev, drvdata);
> +		if (ret) {
> +			hid_err(hdev, "Failed to initialize xg mobile: %d\n", ret);
> +			goto err_stop_hw;
> +		}
> +	}
> +

[Severity: High]
If asus_xgm_init() fails due to a transient USB error during the initial
asus_xgm_led_set() call, will this completely abort the HID device probe and
disable the keyboard?

Since similar features like the keyboard backlight only emit a warning on
failure, should a non-fatal cosmetic LED initialization failure also just warn
and allow the driver to continue normal operation?

[ ... ]

> @@ -1540,6 +1631,9 @@ static void asus_remove(struct hid_device *hdev)
>  	if (drvdata->listener.brightness_set)
>  		asus_hid_unregister_listener(&drvdata->listener);
>  
> +	if (drvdata->xgm_led)
> +		devm_led_classdev_unregister(&hdev->dev, &drvdata->xgm_led->cdev);
> +
>  	asus_worker_stop(drvdata->worker);
>  	hid_hw_stop(hdev);
>  }

[Severity: High]
This is a pre-existing issue, but the devm-managed battery is not explicitly
unregistered before stopping the hardware.

Because the battery is registered via devm_power_supply_register() before
hid_hw_start(), and hid_hw_stop() is called here before the devres cleanup,
could this lead to a use-after-stop?

If a concurrent read of the battery sysfs properties happens during teardown, it
triggers asus_battery_get_property() which calls hid_hw_raw_request() on a
stopped HID device. Can this cause bus hangs or undefined behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619001103.1189200-1-denis.benato@linux.dev?part=4