Re: [PATCH v4 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:
- [High] Propagating cosmetic LED restore errors during system resume causes the entire device resume to fail.
- [High] Failing to initialize the auxiliary XG Mobile LED aborts the entire HID device probe.
- [High] Manual call to `hid_hw_stop()` while using `devm_led_classdev_register()` leads to a use-after-teardown bug during device removal and probe error paths.
--

commit 26f82fa14c4bebc817bb5d79a1cdcdc34bb503d3
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 6896730efafca..e68a6d93369d8 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
[ ... ]
> @@ -1343,9 +1371,52 @@ static int asus_start_multitouch(struct hid_device *hdev)
>  	return 0;
>  }
>  
> +static int asus_xgm_init(struct hid_device *hdev, struct asus_drvdata *drvdata)
> +{
[ ... ]
> +	drvdata->xgm_led->cdev.brightness_set_blocking = asus_xgm_led_set;
> +
> +	/* LED state is arbitrary on boot, set a default */
> +	ret = asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
> +	if (ret) {
> +		hid_err(hdev, "Asus failed to set xgm led: %d\n", ret);
> +		goto asus_xgm_init_err;
> +	}
> +
> +	ret = devm_led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);

[Severity: High]
Because this LED is registered using devm_led_classdev_register(), its
unregistration happens during the devres cleanup phase. This phase runs after
the driver's remove callback (or probe error path) finishes.

If the probe error path or asus_remove() manually calls hid_hw_stop(hdev),
the HID transport is shut down. During the subsequent devres cleanup,
led_classdev_unregister() is called and sets the LED to LED_OFF. This invokes
asus_xgm_led_set(), which calls hid_hw_raw_request() on a stopped interface.

Could this lead to a use-after-teardown bug?

> +	if (ret) {
> +		hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
> +		goto asus_xgm_init_err;
> +	}
> +
> +	return 0;
> +asus_xgm_init_err:
> +	drvdata->xgm_led = NULL;
> +	return ret;
> +}
> +
>  static int __maybe_unused asus_resume(struct hid_device *hdev)
>  {
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +	int ret;
>  
>  	/*
>  	 * If we have a backlight listener registered, restore the previous state,
> @@ -1355,7 +1426,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;

[Severity: High]
If asus_xgm_led_set() fails to restore the LED state (e.g., due to an I/O
error or timeout), this returns the error code directly to the PM core.
A failed resume callback causes the PM subsystem to treat the device resume as
failed, potentially leaving primary features like the keyboard and touchpad
unresponsive.

Would it be better for the failure of a cosmetic LED restore to log a warning
instead of aborting the entire device resume?

> +		}
> +	}
> +
>  	return 0;
> +asus_resume_err:
> +	return ret;
>  }
>  
>  static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
[ ... ]
> @@ -1484,6 +1565,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, the code jumps to err_stop_hw, failing the entire
device probe.

Does this make a non-critical aesthetic feature a hard dependency for the
entire device? Might it be safer to log the failure rather than preventing
the keyboard and touchpad from working?

> +		}
> +	}
> +
>  	/* Laptops keyboard backlight is always at 0x5a */
>  	if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
>  	    (asus_has_report_id(hdev, FEATURE_KBD_REPORT_ID)) &&

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