Re: [PATCH] HID: asus: add support for xgm led

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

On 6/12/26 00:21, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The `asus_xgm_led_set` function is incorrectly assigned to the non-blocking `brightness_set` LED callback, but it performs sleeping operations.
> - [Medium] The XGM LED class device name is hardcoded, which causes device probe failures due to sysfs naming collisions.
> --
>
> commit 55bc647f99f9d2c64c1e99f0ee43e81efa5a6298
> 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 3f5e96900b67a..8a5f364b7535f 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
> [ ... ]
>
>> @@ -1303,6 +1327,32 @@ 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))) {
>> +		drvdata->xgm_led = devm_kzalloc(&hdev->dev, sizeof(*drvdata->xgm_led), GFP_KERNEL);
>> +		if (drvdata->xgm_led == NULL) {
>> +			ret = -ENOMEM;
>> +			goto err_stop_hw;
>> +		}
>> +		drvdata->xgm_led->hdev = hdev;
>> +		drvdata->xgm_led->cdev.name = "asus:xgm:led";
> [Severity: Medium]
> If multiple XG Mobile docks are connected, or if an ungraceful disconnect
> leaves a stale sysfs entry behind, will this hardcoded name cause a sysfs
> naming collision? A duplicate name error here would cause
> led_classdev_register() to fail and abort the entire HID device probe.
ASUS has no models that can support two XGM devices: the connector
is a mixed usb-c+proprietary pci-e and no model has more than one:
there is no way one device can have more than one of these and I think
hardcoding the name is absolutely acceptable here.
> Should this use a dynamic identifier, perhaps incorporating dev_name(),
> to ensure uniqueness?
>
>> +		drvdata->xgm_led->cdev.brightness = 1;
>> +		drvdata->xgm_led->cdev.max_brightness = 1;
>> +		drvdata->xgm_led->cdev.brightness_set = asus_xgm_led_set;
> [Severity: High]
> Can this cause a sleep-in-atomic bug if the LED is triggered by a software
> blink (such as a timer or heartbeat sysfs trigger)?
>
> When software blinking is used, the LED core invokes the brightness_set
> callback directly from softirq context. However, asus_xgm_led_set() calls
> asus_kbd_set_report(), which performs a GFP_KERNEL allocation and a blocking
> USB transaction via hid_hw_raw_request().
>
> Should this callback be assigned to brightness_set_blocking instead,
> so the LED core knows to defer the sleeping work to a workqueue?
Will test this and produce a v2 regardless of the outcome.
>> +		ret = led_classdev_register(&hdev->dev, &drvdata->xgm_led->cdev);
>> +		if (ret) {
>> +			hid_err(hdev, "Asus failed to register xgm led: %d\n", ret);
>> +			goto err_stop_hw;
>> +		}
>> +
>> +		/*
>> +		 * Sometimes after a reboot LEDs are ON, sometimes are OFF: set them to
>> +		 * what the default brightness resets when doing a cold boot.
>> +		 */
>> +		asus_xgm_led_set(&drvdata->xgm_led->cdev, drvdata->xgm_led->cdev.brightness);
>> +	}
>> +