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

[Denis Benato <denis.benato@linux.dev>]

On 6/15/26 19:00, sashiko-bot@kernel.org wrote:
> 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?
No: xg mobile device controllers are just that: not an input device: if it fails
there it's right to tear down the whole thing.
>> +		}
>> +	}
>> +
>>  	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?
Ditto. Won't resolve these as it would only make the code more confusing.
>> +		}
>> +	}
>> +
>>  	/* 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)) &&