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

[sashiko-bot@kernel.org]

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.

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?

> +		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);
> +	}
> +

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