[PATCH RFC 0/1] leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[PATCH RFC 0/1] leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[Liang Haowen]

Hello,

this is the v1 I promised in the "Placement of ASUS Aura and
platform/x86 ASUS files relocation" thread: the LED side of ASUS Aura
RGB on ROG external NVMe enclosures, posted for review in drivers/leds
as Armin suggested, and in line with the shared ASUS Aura interface
Denis has been coordinating.

The hardware: ROG external NVMe enclosures (ROG STRIX Arion, USB
0b05:1932) are plain USB mass-storage devices. They expose two mass
storage interfaces (BOT and UAS) and no HID interface; the Aura LEDs
hang off an ENE controller reached through vendor SCSI commands on the
same LUN as the disk. The Arion has 4 independently addressable LEDs,
verified on hardware.

The interface: the driver registers a scsi_device_handler, matches by
INQUIRY strings (vendor "ROG", model "ESD-S1C"), does not claim the
sdev (sd keeps owning the disk), and exposes each LED as a multicolor
LED class device, /sys/class/leds/asus-arion:led0 through led3.

Protocol summary: a 16-byte vendor CDB (opcode 0xec, 'A' 'S'
signature, register index, argument count in cdb[13]). MODE 0x8021
(Static) must be written first in every sequence or the device ignores
the whole sequence; colours are written to 0x8160 + 3 * led and
0x8100 + 3 * led (3 bytes each, order R, B, G; both tables are written
because firmware revisions pull from one or the other); APPLY 0x80a0
takes 0x01 to apply and 0xaa to save, and only the save makes a change
stick.

A transport gotcha that seems worth wider visibility: the CDB cannot
go through scsi_execute_cmd(), because it derives the command length
from scsi_command_size(opcode), which maps vendor opcode 0xec to
10 bytes. The ENE protocol is a 16-byte CDB with the data length in
cdb[13], so scsi_execute_cmd() drops that byte and the device silently
ignores the write (GOOD status, no error). The driver builds the block
request by hand and forces cmd_len = 16, which is what SG_IO does from
userspace. Any kernel driver sending a vendor CDB whose real length
does not match scsi_command_size(opcode) is going to hit the same
thing.

Placement: per Armin's suggestion in the thread, the LED side belongs
in drivers/leds, since the enclosure is not a platform device and the
user-facing interface is the multicolor LED sysfs. What is posted here
is the driver as verified on hardware, still monolithic. The agreed
shape going forward, with Denis, is a SCSI transport helper in
drivers/scsi feeding an Aura LED driver in drivers/leds behind a
shared ASUS Aura interface; the Kconfig, Makefile and MAINTAINERS
wiring lands with that split. So the main questions for this round are
the LED interface, the protocol handling and the placement.

Known caveats, stated up front:

- the handler attaches manually until a notifier lands
  (echo asus_aura > /sys/block/sdX/device/dh_state);
- SAVE (0xaa) is issued on every colour change, which writes the
  enclosure flash each time; wear has not been characterized yet;
- the LEDs are registered with a NULL parent device, because
  parenting them to the sdev creates a reference cycle that blocks
  the sdev's final release on unplug and leaks the LED nodes and the
  module refcount.

Comments on the interface shape and on folding this into the shared
Aura work are very welcome.

Signed-off-by: Liang Haowen <nbg2974@gmail.com>

Liang Haowen (1):
  leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

 drivers/leds/leds-asus-aura-scsi.c | 302 +++++++++++++++++++++++++++++++++++++
 1 file changed, 302 insertions(+)

-- 
2.55.0

[PATCH RFC 1/1] leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[Liang Haowen]

ASUS ROG external NVMe enclosures (ROG STRIX Arion, USB 0b05:1932) are
plain USB mass-storage devices with no HID interface: the Aura LEDs
hang off an ENE controller driven by vendor SCSI commands on the same
LUN as the disk. The enclosure has 4 independently addressable LEDs,
verified on hardware.

Register a scsi_device_handler matched by INQUIRY (vendor "ROG",
model "ESD-S1C"); it does not claim the sdev (sd keeps owning the
disk) and exposes each LED as a multicolor LED class device,
/sys/class/leds/asus-arion:led0 through led3.

Protocol: a 16-byte vendor CDB (opcode 0xec, 'A' 'S' signature,
register index, argument count in cdb[13]). MODE 0x8021 (Static) must
be written first in every sequence or the device ignores it; colours
go to 0x8160 + 3 * led and 0x8100 + 3 * led (3 bytes, order R, B, G;
both tables are written because firmware revisions pull from one or
the other); APPLY 0x80a0 takes 0x01 to apply and 0xaa to save.

The CDB cannot go through scsi_execute_cmd(): it sizes the command
via scsi_command_size(opcode), which maps vendor opcode 0xec to
10 bytes, so cdb[13] is dropped and the device silently ignores the
write (GOOD status, no error). Build the block request by hand and
force cmd_len = 16, mirroring what SG_IO does from userspace.

This is the monolithic out-of-tree version as verified on hardware;
the Kconfig/Makefile/MAINTAINERS wiring lands with the agreed split
into a SCSI transport helper and a shared ASUS Aura LED interface.

Signed-off-by: Liang Haowen <nbg2974@gmail.com>

---
------------------------------------------------------------------------
 drivers/leds/leds-asus-aura-scsi.c | 302 ++++++++++
 1 file changed, 302 insertions(+)
------------------------------------------------------------------------

diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
new file mode 100644
index 000000000000..111111111111
--- /dev/null
+++ b/drivers/leds/leds-asus-aura-scsi.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ASUS Aura RGB over SCSI for ROG external NVMe enclosures
+ * (e.g. ROG STRIX Arion, USB 0b05:1932).
+ *
+ * USB mass-storage device, no HID; the ENE LED controller is driven via
+ * vendor SCSI commands. Matched by INQUIRY (vendor "ROG", model "ESD-S1C"),
+ * does NOT claim the sdev (sd keeps owning the disk).
+ *
+ * The Arion exposes 4 independently addressable LEDs (verified on hardware):
+ * each is a multicolor LED class device (asus-arion:led0..led3). A colour
+ * change writes that LED's slot only: EFFECT 0x8160 + 3*led, DIRECT
+ * 0x8100 + 3*led (3 bytes, byte order R, B, G), then APPLY (0x01) and
+ * SAVE (0xaa). MODE (0x8021 = Static) is written first in every sequence;
+ * skipping it makes the device ignore the whole sequence.
+ *
+ * Uses brightness_set (non-blocking LED core fast path) + a work_struct
+ * for the sleeping block-request vendor CDB send.
+ *
+ * CDB length: scsi_execute_cmd() sizes the CDB via scsi_command_size(opcode),
+ * which maps vendor opcode 0xec to 10 bytes. The ENE protocol uses a 16-byte
+ * CDB with the data length in cdb[13], so scsi_execute_cmd() drops cdb[13]
+ * and the device silently ignores the write. We build the request by hand
+ * and force cmd_len = 16 (what SG_IO does from userspace).
+ *
+ * Attach manually until a notifier lands:
+ * echo asus_aura > /sys/block/sdX/device/dh_state
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/blk_types.h>
+#include <linux/blkdev.h>
+#include <linux/blk-mq.h>
+#include <linux/workqueue.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_dh.h>
+
+#define ARION_INQ_VENDOR	"ROG"
+#define ARION_INQ_MODEL		"ESD-S1C"
+
+#define ENE_OPCODE		0xec
+#define ENE_REG_MODE		0x8021	/* AuraMode value: Static=1, Breathe=2, ... */
+#define ENE_REG_APPLY		0x80a0
+#define ENE_REG_COLORS		0x8160	/* + 3*led, 3 bytes per LED, order R,B,G */
+#define ENE_REG_COLORS_DIRECT	0x8100	/* + 3*led, same layout */
+#define ENE_APPLY		0x01
+#define ENE_SAVE		0xaa
+#define ENE_MODE_STATIC		1
+#define ENE_CDB_LEN		16
+#define ENE_RGB_LEN		3
+
+/*
+ * Verified on hardware: the enclosure has 4 independently settable LEDs.
+ * (The colour table reserves 16 slots; only the first 4 drive anything.)
+ */
+#define ARION_NUM_LEDS		4
+
+struct asus_aura_led {
+	struct asus_aura_zone	*zone;
+	int			index;
+	struct led_classdev_mc	mc_cdev;
+	struct mc_subled	subled[3];
+	u8			rgb[ENE_RGB_LEN];
+	struct work_struct	work;
+};
+
+struct asus_aura_zone {
+	struct scsi_device	*sdev;
+	struct asus_aura_led	leds[ARION_NUM_LEDS];
+};
+
+static void ene_build_cdb(u8 *cdb, u16 reg, u8 arg_count)
+{
+	memset(cdb, 0, ENE_CDB_LEN);
+	cdb[0] = ENE_OPCODE;
+	cdb[1] = 'A';
+	cdb[2] = 'S';
+	cdb[3] = (reg >> 8) & 0xff;
+	cdb[4] = reg & 0xff;
+	cdb[13] = arg_count;
+}
+
+/* Raw block request so we can force cmd_len=16 (see file header). */
+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 = blk_mq_alloc_request(sdev->request_queue, REQ_OP_DRV_OUT, 0);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	scmd = blk_mq_rq_to_pdu(rq);
+	scmd->cmd_len = ENE_CDB_LEN;
+	memcpy(scmd->cmnd, cdb, ENE_CDB_LEN);
+
+	if (arg_count) {
+		ret = blk_rq_map_kern(rq,
+				      (void *)data, arg_count, GFP_KERNEL);
+		if (ret)
+			goto out;
+	}
+
+	blk_execute_rq(rq, true);
+	ret = scmd->result;
+out:
+	blk_mq_free_request(rq);
+	return ret;
+}
+
+/* Sleepable: runs on the system workqueue. Writes one LED's slot. */
+static void asus_aura_led_work(struct work_struct *work)
+{
+	struct asus_aura_led *led =
+		container_of(work, struct asus_aura_led, work);
+	struct scsi_device *sdev = led->zone->sdev;
+	u8 apply = ENE_APPLY;
+	u8 save = ENE_SAVE;
+	u8 mode = ENE_MODE_STATIC;
+	int 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;
+
+	ret = ene_write(sdev, ENE_REG_COLORS + led->index * 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 + led->index * 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);
+	if (ret)
+		goto err;
+
+	return;
+err:
+	dev_err(&sdev->sdev_gendev,
+		"asus_aura: led%d write failed: %d\n", led->index, 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;
+
+	schedule_work(&led->work);
+}
+
+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);
+	if (!cdev->name)
+		return -ENOMEM;
+	cdev->max_brightness = 255;
+	cdev->brightness_set = asus_aura_set;
+
+	INIT_WORK(&led->work, asus_aura_led_work);
+	led_mc_calc_color_components(&led->mc_cdev, cdev->brightness);
+
+	/*
+	 * Register with NULL parent: parenting the LED to the sdev takes a
+	 * device reference, which blocks the sdev's final release on unplug,
+	 * which is what calls scsi_dh_release_device() -> our .detach() that
+	 * unregisters the LEDs. That reference cycle leaked the LED nodes and
+	 * the module refcount on every hot-unplug.
+	 */
+	ret = led_classdev_multicolor_register(NULL, &led->mc_cdev);
+	if (ret)
+		kfree(cdev->name);
+	return ret;
+}
+
+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);
+	if (!zone)
+		return SCSI_DH_NOMEM;
+	zone->sdev = sdev;
+
+	for (i = 0; i < ARION_NUM_LEDS; i++) {
+		ret = asus_aura_register_led(zone, i);
+		if (ret)
+			goto err_free;
+	}
+
+	sdev->handler_data = zone;
+	sdev_printk(KERN_INFO, sdev,
+		    "asus_aura: %d per-LED multicolor LEDs registered\n",
+		    ARION_NUM_LEDS);
+	return SCSI_DH_OK;
+
+err_free:
+	while (i--) {
+		cancel_work_sync(&zone->leds[i].work);
+		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);
+		kfree(zone->leds[i].mc_cdev.led_cdev.name);
+	}
+	kfree(zone);
+	return SCSI_DH_NOMEM;
+}
+
+static void asus_aura_detach(struct scsi_device *sdev)
+{
+	struct asus_aura_zone *zone = sdev->handler_data;
+	int i;
+
+	if (!zone)
+		return;
+	for (i = 0; i < ARION_NUM_LEDS; i++) {
+		cancel_work_sync(&zone->leds[i].work);
+		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);
+		kfree(zone->leds[i].mc_cdev.led_cdev.name);
+	}
+	kfree(zone);
+	sdev->handler_data = NULL;
+}
+
+static struct scsi_device_handler asus_aura_dh = {
+	.name	= "asus_aura",
+	.module	= THIS_MODULE,
+	.attach	= asus_aura_attach,
+	.detach	= asus_aura_detach,
+};
+
+static int __init asus_aura_init(void)
+{
+	return scsi_register_device_handler(&asus_aura_dh);
+}
+
+static void __exit asus_aura_exit(void)
+{
+	scsi_unregister_device_handler(&asus_aura_dh);
+}
+
+module_init(asus_aura_init);
+module_exit(asus_aura_exit);
+
+MODULE_DESCRIPTION("ASUS Aura RGB over SCSI for ROG NVMe enclosures (per-LED)");
+MODULE_AUTHOR("Liang Haowen");
+MODULE_LICENSE("GPL");

Re: [PATCH RFC 1/1] leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Incorrect teardown order in device detach and error paths leads to Use-After-Free via the workqueue.
- [High] Bypassing `scsi_alloc_request` leaves critical `scsi_cmnd` fields uninitialized.
- [High] Missing serialization for the multi-step ENE hardware command sequence allows concurrent workers to interleave and corrupt hardware state.
- [Low] Undefined macro `kzalloc_obj` is used for memory allocation.
- [Low] `blk_rq_map_kern` is called with insufficient arguments.
- [Low] The driver prints a success log message on probe/attach.
- [Low] The commit message violates the LED subsystem capitalization and prefix rules.
--

commit 241d70bc8fb77ca0865693a679d9c3bdc51ad240
Author: Liang Haowen <nbg2974@gmail.com>

leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

This commit adds a SCSI driver for ASUS ROG external NVMe enclosures to
support Aura LEDs. The driver exposes each LED as a multicolor LED class
device and communicates with the ENE controller using vendor SCSI commands.

> leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[Severity: Low]
Does this commit message subject follow the LED subsystem guidelines?
The guidelines specify that the description after the subsystem prefix should
be capitalized and include the driver name, such as:
leds: asus-aura-scsi: Add ASUS Aura SCSI driver for ROG NVMe enclosures

> diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000000000..c913501ba96a2
> --- /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 = blk_mq_alloc_request(sdev->request_queue, REQ_OP_DRV_OUT, 0);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);

[Severity: High]
Does bypassing scsi_alloc_request() here leave critical scsi_cmnd fields
uninitialized? Using blk_mq_alloc_request() directly skips
scsi_initialize_rq(), which means PDU fields like jiffies_at_alloc, rcu,
and retries remain uninitialized. When blk_execute_rq() runs, this might
trigger false timeouts or undefined behavior in the SCSI midlayer.

> +
> +	scmd = blk_mq_rq_to_pdu(rq);
> +	scmd->cmd_len = ENE_CDB_LEN;
> +	memcpy(scmd->cmnd, cdb, ENE_CDB_LEN);
> +
> +	if (arg_count) {
> +		ret = blk_rq_map_kern(rq,
> +				      (void *)data, arg_count, GFP_KERNEL);

[Severity: Low]
Can this blk_rq_map_kern() call compile? It seems to be missing the required
first parameter for the request queue (struct request_queue *q), passing
only 4 arguments instead of the mandatory 5.

> +		if (ret)
> +			goto out;
> +	}
[ ... ]
> +static void asus_aura_led_work(struct work_struct *work)
> +{
> +	struct asus_aura_led *led =
> +		container_of(work, struct asus_aura_led, work);
> +	struct scsi_device *sdev = led->zone->sdev;
> +	u8 apply = ENE_APPLY;
> +	u8 save = ENE_SAVE;
> +	u8 mode = ENE_MODE_STATIC;
> +	int 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;
> +
> +	ret = ene_write(sdev, ENE_REG_COLORS + led->index * ENE_RGB_LEN,
> +			led->rgb, ENE_RGB_LEN);

[Severity: High]
Could concurrent sysfs writes to different LEDs on the same device corrupt
the hardware state here? Because there are 4 LEDs on the same device,
multiple workers could interleave their ene_write() block requests to the
same endpoint without a shared mutex, potentially breaking the strict
multi-step sequence required by the ENE controller.

> +	if (ret)
> +		goto err;
[ ... ]
> +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]
Is kzalloc_obj() a valid macro in the kernel API? It appears this will cause
a build failure, and should likely be standard kzalloc(sizeof(*zone),
GFP_KERNEL) instead.

> +	if (!zone)
> +		return SCSI_DH_NOMEM;
> +	zone->sdev = sdev;
> +
> +	for (i = 0; i < ARION_NUM_LEDS; i++) {
> +		ret = asus_aura_register_led(zone, i);
> +		if (ret)
> +			goto err_free;
> +	}
> +
> +	sdev->handler_data = zone;
> +	sdev_printk(KERN_INFO, sdev,
> +		    "asus_aura: %d per-LED multicolor LEDs registered\n",
> +		    ARION_NUM_LEDS);

[Severity: Low]
Is it okay to print this success log message on probe? The LED subsystem
guidelines specify that drivers should avoid logging success messages, and
only log errors or warnings.

> +	return SCSI_DH_OK;
> +
> +err_free:
> +	while (i--) {
> +		cancel_work_sync(&zone->leds[i].work);
> +		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);

[Severity: High]
Could this teardown order introduce a use-after-free risk? Unregistering the
LED sets its brightness to 0, which invokes asus_aura_set() and queues a
new work item onto the system workqueue. Since cancel_work_sync() has
already been called, a dangling work item might remain when kfree(zone)
is executed shortly after.

> +		kfree(zone->leds[i].mc_cdev.led_cdev.name);
> +	}
> +	kfree(zone);
> +	return SCSI_DH_NOMEM;
> +}
> +
> +static void asus_aura_detach(struct scsi_device *sdev)
> +{
> +	struct asus_aura_zone *zone = sdev->handler_data;
> +	int i;
> +
> +	if (!zone)
> +		return;
> +	for (i = 0; i < ARION_NUM_LEDS; i++) {
> +		cancel_work_sync(&zone->leds[i].work);
> +		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);

[Severity: High]
Similar to the error path in asus_aura_attach(), does unregistering the LED
after cancel_work_sync() leave a dangling work item? The unregister call
might queue new work via asus_aura_set(), leading to a use-after-free when
the memory is subsequently freed.

> +		kfree(zone->leds[i].mc_cdev.led_cdev.name);
> +	}
> +	kfree(zone);

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

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

[Liang Haowen]

ASUS ROG external NVMe enclosures (ROG STRIX Arion, USB 0b05:1932) are
plain USB mass-storage devices with no HID interface: the Aura LEDs
hang off an ENE controller driven by vendor SCSI commands on the same
LUN as the disk. The enclosure has 4 independently addressable LEDs,
verified on hardware.

Register a scsi_device_handler matched by INQUIRY (vendor "ROG",
model "ESD-S1C"); it does not claim the sdev (sd keeps owning the
disk) and exposes each LED as a multicolor LED class device,
/sys/class/leds/asus-arion:led0 through led3.

Protocol: a 16-byte vendor CDB (opcode 0xec, 'A' 'S' signature,
register index, argument count in cdb[13]). MODE 0x8021 (Static) must
be written first in every sequence or the device ignores it; colours
go to 0x8160 + 3 * led and 0x8100 + 3 * led (3 bytes, order R, B, G;
both tables are written because firmware revisions pull from one or
the other); APPLY 0x80a0 takes 0x01 to apply and 0xaa to save.

The CDB cannot go through scsi_execute_cmd(): it sizes the command
via scsi_command_size(opcode), which maps vendor opcode 0xec to
10 bytes, so cdb[13] is dropped and the device silently ignores the
write (GOOD status, no error). The request is built with
scsi_alloc_request() instead, which fully initializes the scsi_cmnd,
with cmd_len forced to 16, mirroring what SG_IO does from userspace.

brightness_set only caches the colour and marks the LED in a per-zone
dirty bitmap; a single work item per zone then runs one ENE sequence
for all pending LEDs (MODE, colour slots, APPLY, SAVE). Funneling
every update through one work item keeps the sequences from
interleaving between concurrent LED updates and batches multi-LED
updates into a single APPLY/SAVE.

This is the monolithic out-of-tree version as verified on hardware;
the Kconfig/Makefile/MAINTAINERS wiring lands with the agreed split
into a SCSI transport helper and a shared ASUS Aura LED interface.

Signed-off-by: Liang Haowen <nbg2974@gmail.com>
---
 drivers/leds/leds-asus-aura-scsi.c | 332 +++++++++++++++++++++++++++++
 1 file changed, 332 insertions(+)
 create mode 100644 drivers/leds/leds-asus-aura-scsi.c

diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
new file mode 100644
index 0000000..4dd559a
--- /dev/null
+++ b/drivers/leds/leds-asus-aura-scsi.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ASUS Aura RGB over SCSI for ROG external NVMe enclosures
+ * (e.g. ROG STRIX Arion, USB 0b05:1932).
+ *
+ * USB mass-storage device, no HID; the ENE LED controller is driven via
+ * vendor SCSI commands. Matched by INQUIRY (vendor "ROG", model "ESD-S1C"),
+ * does NOT claim the sdev (sd keeps owning the disk).
+ *
+ * The Arion exposes 4 independently addressable LEDs (verified on hardware):
+ * each is a multicolor LED class device (asus-arion:led0..led3). A colour
+ * change writes that LED's slot only: EFFECT 0x8160 + 3*led, DIRECT
+ * 0x8100 + 3*led (3 bytes, byte order R, B, G), then APPLY (0x01) and
+ * SAVE (0xaa). MODE (0x8021 = Static) is written first in every sequence;
+ * skipping it makes the device ignore the whole sequence.
+ *
+ * Scheduling: brightness_set (LED core fast path) caches the colour and
+ * marks the LED in a per-zone dirty bitmap; a single work item per zone
+ * runs one ENE sequence for all pending LEDs (MODE once, colour slots,
+ * APPLY, SAVE). Funneling every update through that one work item also
+ * serializes the sequences: the MODE/colour/APPLY/SAVE chain must never
+ * interleave between concurrent LED updates.
+ *
+ * CDB length: scsi_execute_cmd() sizes the CDB via COMMAND_SIZE(opcode),
+ * which maps vendor opcode 0xec to 10 bytes. The ENE protocol uses a 16-byte
+ * CDB with the data length in cdb[13], so scsi_execute_cmd() drops cdb[13]
+ * and the device silently ignores the write. ene_write() therefore mirrors
+ * scsi_execute_cmd() on top of scsi_alloc_request() and forces cmd_len = 16
+ * (what SG_IO does from userspace).
+ *
+ * Attach manually until a notifier lands:
+ * echo asus_aura > /sys/block/sdX/device/dh_state
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/blk_types.h>
+#include <linux/blkdev.h>
+#include <linux/blk-mq.h>
+#include <linux/workqueue.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_dh.h>
+
+#define ARION_INQ_VENDOR	"ROG"
+#define ARION_INQ_MODEL		"ESD-S1C"
+
+#define ENE_OPCODE		0xec
+#define ENE_REG_MODE		0x8021	/* AuraMode value: Static=1, Breathe=2, ... */
+#define ENE_REG_APPLY		0x80a0
+#define ENE_REG_COLORS		0x8160	/* + 3*led, 3 bytes per LED, order R,B,G */
+#define ENE_REG_COLORS_DIRECT	0x8100	/* + 3*led, same layout */
+#define ENE_APPLY		0x01
+#define ENE_SAVE		0xaa
+#define ENE_MODE_STATIC		1
+#define ENE_CDB_LEN		16
+#define ENE_RGB_LEN		3
+#define ENE_TIMEOUT		(10 * HZ)
+
+/*
+ * Verified on hardware: the enclosure has 4 independently settable LEDs.
+ * (The colour table reserves 16 slots; only the first 4 drive anything.)
+ */
+#define ARION_NUM_LEDS		4
+
+struct asus_aura_led {
+	struct asus_aura_zone	*zone;
+	int			index;
+	struct led_classdev_mc	mc_cdev;
+	struct mc_subled	subled[3];
+	u8			rgb[ENE_RGB_LEN];
+};
+
+struct asus_aura_zone {
+	struct scsi_device	*sdev;
+	struct asus_aura_led	leds[ARION_NUM_LEDS];
+	unsigned long		dirty;	/* bit i: led i needs a colour write */
+	struct work_struct	work;
+};
+
+static void ene_build_cdb(u8 *cdb, u16 reg, u8 arg_count)
+{
+	memset(cdb, 0, ENE_CDB_LEN);
+	cdb[0] = ENE_OPCODE;
+	cdb[1] = 'A';
+	cdb[2] = 'S';
+	cdb[3] = (reg >> 8) & 0xff;
+	cdb[4] = reg & 0xff;
+	cdb[13] = arg_count;
+}
+
+/*
+ * scsi_execute_cmd() with cmd_len forced to 16. scsi_alloc_request()
+ * initializes the scsi_cmnd fully (scsi_initialize_rq(): zeroed cmnd, sense
+ * and rcu head, retries), which a raw blk_mq_alloc_request() does not.
+ */
+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);
+		if (ret)
+			goto out;
+	}
+
+	scmd = blk_mq_rq_to_pdu(rq);
+	scmd->cmd_len = ENE_CDB_LEN;
+	memcpy(scmd->cmnd, cdb, ENE_CDB_LEN);
+	scmd->allowed = 1;
+	rq->timeout = ENE_TIMEOUT;
+	rq->rq_flags |= RQF_QUIET;
+
+	blk_execute_rq(rq, true);
+	ret = scmd->result;
+out:
+	blk_mq_free_request(rq);
+	return ret;
+}
+
+/*
+ * Sleepable: runs on the system workqueue. One ENE sequence for every LED
+ * marked in the dirty bitmap. test_and_clear_bit() pairs with the set_bit()
+ * in asus_aura_set(): a colour cached while this runs requeues the work and
+ * is picked up by the next sequence.
+ */
+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);
+	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);
+}
+
+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);
+	if (!cdev->name)
+		return -ENOMEM;
+	cdev->max_brightness = 255;
+	cdev->brightness_set = asus_aura_set;
+
+	led_mc_calc_color_components(&led->mc_cdev, cdev->brightness);
+
+	/*
+	 * Register with NULL parent: parenting the LED to the sdev takes a
+	 * device reference, which blocks the sdev's final release on unplug,
+	 * which is what calls scsi_dh_release_device() -> our .detach() that
+	 * unregisters the LEDs. That reference cycle leaked the LED nodes and
+	 * the module refcount on every hot-unplug.
+	 */
+	ret = led_classdev_multicolor_register(NULL, &led->mc_cdev);
+	if (ret)
+		kfree(cdev->name);
+	return ret;
+}
+
+/*
+ * Unregister the LED devices before cancelling the work: unregistering
+ * removes the sysfs attributes, so no new brightness_set can schedule the
+ * zone work afterwards, and it waits for in-flight sysfs callbacks.
+ * Cancelling first would leave a window where a brightness write requeues
+ * the work after cancel_work_sync() returned, and the work would then run
+ * on freed memory.
+ */
+static void asus_aura_release(struct asus_aura_zone *zone, int num_leds)
+{
+	int i;
+
+	for (i = 0; i < num_leds; i++)
+		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);
+	cancel_work_sync(&zone->work);
+	for (i = 0; i < num_leds; i++)
+		kfree(zone->leds[i].mc_cdev.led_cdev.name);
+	kfree(zone);
+}
+
+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);
+	if (!zone)
+		return SCSI_DH_NOMEM;
+	zone->sdev = sdev;
+	INIT_WORK(&zone->work, asus_aura_zone_work);
+
+	for (i = 0; i < ARION_NUM_LEDS; i++) {
+		ret = asus_aura_register_led(zone, i);
+		if (ret) {
+			asus_aura_release(zone, i);
+			return SCSI_DH_NOMEM;
+		}
+	}
+
+	sdev->handler_data = zone;
+	return SCSI_DH_OK;
+}
+
+static void asus_aura_detach(struct scsi_device *sdev)
+{
+	struct asus_aura_zone *zone = sdev->handler_data;
+
+	if (!zone)
+		return;
+	asus_aura_release(zone, ARION_NUM_LEDS);
+	sdev->handler_data = NULL;
+}
+
+static struct scsi_device_handler asus_aura_dh = {
+	.name	= "asus_aura",
+	.module	= THIS_MODULE,
+	.attach	= asus_aura_attach,
+	.detach	= asus_aura_detach,
+};
+
+static int __init asus_aura_init(void)
+{
+	return scsi_register_device_handler(&asus_aura_dh);
+}
+
+static void __exit asus_aura_exit(void)
+{
+	scsi_unregister_device_handler(&asus_aura_dh);
+}
+
+module_init(asus_aura_init);
+module_exit(asus_aura_exit);
+
+MODULE_DESCRIPTION("ASUS Aura RGB over SCSI for ROG NVMe enclosures (per-LED)");
+MODULE_AUTHOR("Liang Haowen");
+MODULE_LICENSE("GPL");
-- 
2.55.0

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

[sashiko-bot]

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

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

[Lee Jones]

On Thu, 03 Sep 2026, sashiko-bot@kernel.org wrote:

> 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.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> --
> 
> 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

-- 
Lee Jones

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

[Liang Haowen]

Hello,

v2 of the LED driver for ASUS Aura RGB on ROG external NVMe
enclosures. It addresses all seven points from the review v1 received
(from the sashiko AI bot; there has been no human review on v1 yet).

Changes since v1:

- Teardown order: .detach() and the attach error path now unregister
  the LED class devices first, then cancel_work_sync(), then free.
  The v1 order (cancel first) left a window where a brightness write
  could requeue the work after cancel_work_sync() returned, so the
  work would run on freed memory.

- Request allocation: ene_write() now builds the request with
  scsi_alloc_request() instead of a raw blk_mq_alloc_request().
  scsi_initialize_rq() zeroes cmnd, initializes the rcu head, sense
  length and retries; skipping that left those fields uninitialized.
  An explicit timeout and RQF_QUIET are set, matching what
  scsi_execute_cmd() does.

- Serialization of the ENE sequence: brightness_set() now only caches
  the colour and marks the LED in a per-zone dirty bitmap; a single
  work item per zone runs one sequence (MODE, colour slots, APPLY,
  SAVE) for all pending LEDs. The v1 per-LED works could interleave
  their sequences between concurrent updates. As a side effect,
  multi-LED updates now batch into one APPLY/SAVE.

- Subject line: switched to the leds subsystem prefix and
  capitalization.

- The attach success log message is gone.

Two of the reported items did not hold up against the kernel this
driver is built against (7.2.2):

- kzalloc_obj() is not an undefined macro; it lives in
  include/linux/slab.h (since v6.17).

- blk_rq_map_kern() with four arguments is the current signature
  (rq, buf, len, gfp); drivers/scsi/scsi_lib.c calls it that way from
  scsi_execute_cmd().

Everything else is unchanged from v1: the hardware (ROG external NVMe
enclosures, e.g. ROG STRIX Arion, USB 0b05:1932, no HID, ENE LED
controller behind vendor SCSI commands on the disk's LUN, 4
independently addressable LEDs), the scsi_device_handler that does
not claim the sdev, the multicolor LED interface, and the
protocol handling. v2 was re-verified on hardware.

Known caveats, unchanged:

- the handler attaches manually until a notifier lands
  (echo asus_aura > /sys/block/sdX/device/dh_state);

- SAVE (0xaa) is issued with every colour update, which writes the
  enclosure flash each time; wear has not been characterized yet;

- the LEDs are registered with a NULL parent device, because
  parenting them to the sdev creates a reference cycle that blocks
  the sdev's final release on unplug.

Comments on the interface shape and on folding this into the shared
Aura work with Denis remain very welcome.

Signed-off-by: Liang Haowen <nbg2974@gmail.com>

Liang Haowen (1):
  leds: asus-aura-scsi: Add ASUS Aura RGB LED driver for ROG NVMe enclosures

 drivers/leds/leds-asus-aura-scsi.c | 332 +++++++++++++++++++++++++++++
 1 file changed, 332 insertions(+)

--
2.55.0