Re: [PATCH RFC v2 1/1] leds: asus-aura-scsi: Add ASUS Aura RGB LED driver for ROG NVMe enclosures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The workqueue function unconditionally writes to flash even when no LEDs are dirty.
- [Medium] Hardcoded LED class device names will cause a sysfs collision if multiple enclosures are connected.
- [Medium] Missing memory barriers and locking for led->rgb cause data races and lost color updates.
- [Low] Missing 'struct request_queue *' argument in blk_rq_map_kern() call.
- [Low] Use of non-existent kzalloc_obj macro prevents compilation.
--

commit 12eb38bf536e935469d4f61d68a4b96acd78d4b1
Author: Liang Haowen <nbg2974@gmail.com>

leds: asus-aura-scsi: Add ASUS Aura RGB LED driver for ROG NVMe enclosures

This commit adds a new out-of-tree driver for ASUS ROG external NVMe
enclosures. It registers a SCSI device handler matched by INQUIRY and
exposes the 4 addressable Aura RGB LEDs as multicolor LED class devices.
A single per-zone work item handles the ENE protocol sequences and
serializes updates.

> diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000000000..4dd559a5d8ec1
> --- /dev/null
> +++ b/drivers/leds/leds-asus-aura-scsi.c
[ ... ]
> +static int ene_write(struct scsi_device *sdev, u16 reg,
> +		     const void *data, u8 arg_count)
> +{
> +	struct request *rq;
> +	struct scsi_cmnd *scmd;
> +	u8 cdb[ENE_CDB_LEN];
> +	int ret;
> +
> +	ene_build_cdb(cdb, reg, arg_count);
> +
> +	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_OUT, 0);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
> +
> +	if (arg_count) {
> +		ret = blk_rq_map_kern(rq, (void *)data, arg_count, GFP_NOIO);

[Severity: Low]
Does this missing argument in ene_write() in
drivers/leds/leds-asus-aura-scsi.c cause a build failure? The
blk_rq_map_kern() function takes 5 arguments (requiring a
struct request_queue *q as the first parameter), but only 4 are provided
here.

> +		if (ret)
> +			goto out;
> +	}
[ ... ]
> +static void asus_aura_zone_work(struct work_struct *work)
> +{
> +	struct asus_aura_zone *zone =
> +		container_of(work, struct asus_aura_zone, work);
> +	struct scsi_device *sdev = zone->sdev;
> +	u8 apply = ENE_APPLY;
> +	u8 save = ENE_SAVE;
> +	u8 mode = ENE_MODE_STATIC;
> +	int i, ret;
> +
> +	if (!scsi_device_online(sdev))
> +		return;
> +
> +	/* Mode first: without it the device ignores the whole sequence. */
> +	ret = ene_write(sdev, ENE_REG_MODE, &mode, 1);
> +	if (ret)
> +		goto err;
> +
> +	for (i = 0; i < ARION_NUM_LEDS; i++) {
> +		struct asus_aura_led *led = &zone->leds[i];
> +
> +		if (!test_and_clear_bit(i, &zone->dirty))
> +			continue;
> +
> +		ret = ene_write(sdev, ENE_REG_COLORS + i * ENE_RGB_LEN,
> +				led->rgb, ENE_RGB_LEN);
> +		if (ret)
> +			goto err;
> +
> +		/*
> +		 * Cover the DIRECT colour set too; some firmware revisions
> +		 * pull from 0x8100 instead of 0x8160.
> +		 */
> +		ret = ene_write(sdev, ENE_REG_COLORS_DIRECT + i * ENE_RGB_LEN,
> +				led->rgb, ENE_RGB_LEN);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = ene_write(sdev, ENE_REG_APPLY, &apply, 1);
> +	if (ret)
> +		goto err;
> +
> +	/*
> +	 * The change only takes effect after SAVE (0xaa). NOTE: saving on
> +	 * every brightness change writes flash each time; revisit for wear
> +	 * once confirmed.
> +	 */
> +	ret = ene_write(sdev, ENE_REG_APPLY, &save, 1);

[Severity: High]
Does asus_aura_zone_work() in drivers/leds/leds-asus-aura-scsi.c
unconditionally write to the device flash even when no LEDs are dirty?

If schedule_work() is called while asus_aura_zone_work() is already
executing, the work item gets re-queued. If the first execution already
processed the newly set LED bit, the second execution will observe a fully
cleared zone->dirty bitmap, but still send the MODE, APPLY, and SAVE
commands to the hardware.

Since SAVE writes to the device flash, these spurious empty runs could cause
unnecessary device flash wear.

> +	if (ret)
> +		goto err;
> +
> +	return;
> +err:
> +	dev_err(&sdev->sdev_gendev,
> +		"asus_aura: colour update failed: %d\n", ret);
> +}
> +
> +/* Non-blocking LED callback (LED core fast path). Cache colour, defer SCSI. */
> +static void asus_aura_set(struct led_classdev *cdev,
> +			  enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct asus_aura_led *led =
> +		container_of(mc, struct asus_aura_led, mc_cdev);
> +
> +	led_mc_calc_color_components(mc, brightness);
> +	/* ENE colour register byte order is R, B, G. */
> +	led->rgb[0] = led->subled[0].brightness;
> +	led->rgb[1] = led->subled[2].brightness;
> +	led->rgb[2] = led->subled[1].brightness;
> +
> +	set_bit(led->index, &led->zone->dirty);
> +	schedule_work(&led->zone->work);

[Severity: Medium]
Can this cause data races and lost color updates?

In asus_aura_set() in drivers/leds/leds-asus-aura-scsi.c, led->rgb is updated
before marking the LED as dirty via set_bit(). Because set_bit() lacks a full
memory barrier, the CPU or compiler can reorder the led->rgb writes to occur
after the dirty bit is set.

A concurrently executing asus_aura_zone_work() on another CPU could observe
the dirty bit via test_and_clear_bit(), read stale led->rgb values, and
clear the bit, causing the new color to be permanently lost.

> +}
> +
> +static int asus_aura_register_led(struct asus_aura_zone *zone, int index)
> +{
> +	struct asus_aura_led *led = &zone->leds[index];
> +	struct led_classdev *cdev = &led->mc_cdev.led_cdev;
> +	int ret;
> +
> +	led->zone = zone;
> +	led->index = index;
> +
> +	led->subled[0].color_index = LED_COLOR_ID_RED;
> +	led->subled[1].color_index = LED_COLOR_ID_GREEN;
> +	led->subled[2].color_index = LED_COLOR_ID_BLUE;
> +	led->mc_cdev.num_colors = 3;
> +	led->mc_cdev.subled_info = led->subled;
> +
> +	cdev->name = kasprintf(GFP_KERNEL, "asus-arion:led%d", index);

[Severity: Medium]
Will this hardcoded name in asus_aura_register_led() in
drivers/leds/leds-asus-aura-scsi.c cause a sysfs collision if multiple
enclosures are connected?

If a user connects two ASUS ROG external NVMe enclosures, the second enclosure
will attempt to register identical sysfs names (e.g.,
/sys/class/leds/asus-arion:led0), resulting in an -EEXIST error and
preventing the use of multiple devices simultaneously.

> +	if (!cdev->name)
> +		return -ENOMEM;
[ ... ]
> +static int asus_aura_attach(struct scsi_device *sdev)
> +{
> +	struct asus_aura_zone *zone;
> +	int i, ret;
> +
> +	if (strncmp(sdev->vendor, ARION_INQ_VENDOR, strlen(ARION_INQ_VENDOR)) ||
> +	    strncmp(sdev->model, ARION_INQ_MODEL, strlen(ARION_INQ_MODEL)))
> +		return SCSI_DH_DEV_UNSUPP;
> +
> +	zone = kzalloc_obj(*zone, GFP_KERNEL);

[Severity: Low]
Does this prevent the driver from compiling?

In asus_aura_attach() in drivers/leds/leds-asus-aura-scsi.c, the macro
kzalloc_obj does not exist in the Linux kernel headers, leading to an
implicit declaration or undefined reference error.

> +	if (!zone)
> +		return SCSI_DH_NOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/202609032000.RFCv2-1.lhw@gmail.com?part=1