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

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

[Liang Haowen]

Hello,

v8, as its own thread. It addresses every comment from Lee's v6
review and Ilpo's line review; the notes below say what was done per
comment and where I think a comment rests on a misunderstanding, I
say so instead of silently keeping the code.

The submission format is git send-email from v7 on, the driver lives
in drivers/leds/rgb/, and it still has no Kconfig wiring: it is the
verified out-of-tree module, the wiring lands with the agreed SCSI
split. On the SCSI device handler itself: it is an out-of-tree
expedient, not a design statement. The enclosure exposes a single
mass-storage interface that usb-storage owns, so a USB interface
driver cannot claim it without taking the disk down, and hooking the
SCSI bus from a module needs the unexported scsi_bus_type. The
device handler is the one hook that observes the sdev without
claiming it. For in-tree, the attachment should move under
drivers/scsi in whichever form the SCSI side prefers, notifier or an
exported attach helper, and I will rework it accordingly.

Changes against the review comments:

- Includes sorted alphabetically, linux/err.h added (Ilpo).

- The vendor CDB is now struct ene_cdb, __packed, with named fields
  and a __be16 register set with cpu_to_be16() (Lee, Ilpo), plus a
  static_assert on the 16-byte size.

- The ENE register defines carry plain comments; the R,B,G wire
  order has named indices (ENE_WIRE_R/B/G) so the reordering against
  the R,G,B subleds no longer relies on magic numbers (Lee, Ilpo).

- struct asus_aura_zone became struct asus_aura, the per-enclosure
  context; the dirty bitmask is gone, each LED carries its own
  pending flag (Lee: LED-level attribute, faux indexing removed).

- The work item returns before touching the device when nothing is
  pending: an unlocked scan first, since a colour arriving after the
  scan re-queues the work (Lee). The locked snapshot follows.

- The write payload is copied into a DMA-safe per-device buffer
  before blk_rq_map_kern(); no stack memory is ever mapped for DMA
  (Lee, VMAP_STACK).

- ene_write() became ene_write_reg() with a tx buffer argument, the
  arg_count branch is gone (never false), and it returns 0/-EIO
  instead of the raw SAM status; the rationale for not using
  scsi_execute_cmd() moved to the commit message (Lee).

- The 10 s timeout dropped to 5 s, the development-history comments
  are gone, declarations are unwrapped, loop variables are declared
  in the loop, lines are reflowed to 100 columns, the code is grouped
  with blank lines, and
  asus_aura_register_led() frees its own name on failure (Lee).

- dev_err_ratelimited() instead of dev_err() on the work error path
  (Ilpo); _once() would hide a persistently failing device, and
  there is no caller to report to, the work runs detached from the
  LED core (Lee).

- strscpy() uses the 2-arg form (Ilpo).

Two comments I did not act on:

- .module = THIS_MODULE in the device handler: the in-tree handlers
  (scsi_dh_alua, scsi_dh_emc) set it explicitly as well, so I kept
  it, but I am happy to drop it if the subsystem prefers.

- kzalloc_obj() stays: checkpatch on 7.2 prefers it over kzalloc()
  with sizeof, and the sashiko report against it is based on an
  older tree (it has been in include/linux/slab.h since v7.0).

v8 was verified on hardware again: the four LEDs, per-LED colours,
60 sequential updates, unplug under load (zero splats, zero leaked
nodes, clean rmmod).

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

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

-- 
2.55.0

[RFC v8 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-0-0-0-0::led-0 through led-3. The
H:C:T:L part of the sdev name keeps the names unique when more than
one enclosure is connected, with its colons flattened to dashes. The
color section stays empty, since multicolor LEDs enumerate their
palette via multi_intensity, and the four identical zones use the
function name with a "-N" ordinal, as the naming section in
Documentation/leds/leds-class.rst asks for.

Protocol: a 16-byte vendor CDB (opcode 0xec, 'A' 'S' signature,
BE16 register index, argument count in byte 13), modelled by
struct ene_cdb. 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, wire 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 byte 13 is dropped and the device silently ignores the
write (GOOD status, no error). scsi_execute_cmd() also offers no way
to override the derived length. The request is therefore built with
scsi_alloc_request(), which initializes the scsi_cmnd parts a
passthrough needs, with cmd_len set to the CDB size, mirroring what
SG_IO does from userspace. A 5 s timeout and a single retry-less
quiet submission cover the register writes.

The write payload is copied into a per-device buffer before it is
mapped with blk_rq_map_kern(), so no stack memory is ever mapped for
DMA (VMAP_STACK).

brightness_set only caches the colour and marks the LED pending
under a spinlock; led_mc_calc_color_components() runs under that
lock too, because it writes the shared subled_info array and
trigger events call led_set_brightness() without the led_access lock
that serializes sysfs stores. A single work item per enclosure then
snapshots the pending LEDs and runs one ENE sequence for all of
them. 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. A re-queued run
with nothing pending returns before touching the device, so it
cannot wear the flash with a pointless SAVE, and the lock keeps a
colour write from being reordered after its pending flag on weakly
ordered architectures.

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/rgb/leds-asus-aura-scsi.c | 388 +++++++++++++++++++++++++
 1 file changed, 388 insertions(+)
 create mode 100644 drivers/leds/rgb/leds-asus-aura-scsi.c

diff --git a/drivers/leds/rgb/leds-asus-aura-scsi.c b/drivers/leds/rgb/leds-asus-aura-scsi.c
new file mode 100644
index 0000000..eaba968
--- /dev/null
+++ b/drivers/leds/rgb/leds-asus-aura-scsi.c
@@ -0,0 +1,388 @@
+// 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 on the disk's LUN. Matched by INQUIRY (vendor
+ * "ROG", model "ESD-S1C"); the sdev is not claimed, sd keeps owning the
+ * disk. Attach manually until the SCSI side agrees on an in-tree hook:
+ * echo asus_aura > /sys/block/sdX/device/dh_state
+ *
+ * Each of the 4 LEDs is a multicolor LED class device,
+ * /sys/class/leds/asus-arion-<H-C-T-L>::led-0..led-3. A colour change
+ * writes only that LED's slots in the ENE colour tables, then applies
+ * and saves; MODE must precede every sequence or the device ignores
+ * it. The register layout is described by struct ene_cdb.
+ *
+ * brightness_set (LED core fast path) only caches the colour and marks
+ * the LED pending under a spinlock; a single work item per enclosure
+ * snapshots all pending LEDs and runs one ENE sequence for them. One
+ * work item also serializes the sequences against each other: the
+ * MODE/colour/APPLY/SAVE chain must never interleave between
+ * concurrent LED updates.
+ */
+
+#include <linux/bits.h>
+#include <linux/blk-mq.h>
+#include <linux/blk_types.h>
+#include <linux/blkdev.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/string.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"
+
+/* The colour table has 16 slots; 4 of them drive LEDs. */
+#define ARION_NUM_LEDS		4
+
+/* ENE register map (BE16 on the wire). */
+#define ENE_REG_MODE		0x8021	/* 1 = Static selects the colour tables */
+#define ENE_REG_APPLY		0x80a0	/* 0x01 = apply, 0xaa = apply and save */
+#define ENE_REG_COLORS_EFFECT	0x8160	/* colour table, 3 bytes per LED */
+#define ENE_REG_COLORS_DIRECT	0x8100	/* second table some firmware uses */
+
+#define ENE_MODE_STATIC		1
+#define ENE_APPLY		0x01
+#define ENE_SAVE		0xaa
+
+/* The colour tables are R, B, G on the wire; the subleds are R, G, B. */
+#define ENE_WIRE_R		0
+#define ENE_WIRE_B		1
+#define ENE_WIRE_G		2
+#define ENE_RGB_LEN		3
+
+/*
+ * Vendor CDB: opcode, 'A''S' signature, register, argument count in
+ * byte 13. scsi_execute_cmd() cannot be used for it, see the commit
+ * message.
+ */
+struct ene_cdb {
+	u8		opcode;
+	u8		sig[2];
+	__be16		reg;
+	u8		reserved[8];
+	u8		arg_count;
+	u8		pad[2];
+} __packed;
+
+static_assert(sizeof(struct ene_cdb) == 16);
+
+struct asus_aura_led {
+	struct led_classdev_mc	mc_cdev;
+	struct mc_subled	subled[ENE_RGB_LEN];
+
+	/* Cached colour, consumed by the work item. */
+	u8			rgb[ENE_RGB_LEN];
+	bool			pending;
+
+	struct asus_aura	*aura;
+};
+
+struct asus_aura {
+	struct scsi_device	*sdev;
+	struct asus_aura_led	leds[ARION_NUM_LEDS];
+
+	/* Serializes the cached colours and pending flags. */
+	spinlock_t		lock;
+	struct work_struct	work;
+
+	/* DMA-safe staging buffer, the only memory ever mapped for DMA. */
+	u8			tx[ENE_RGB_LEN];
+};
+
+static int ene_write_reg(struct asus_aura *aura, u16 reg,
+			 const u8 *tx, u8 len)
+{
+	struct scsi_device *sdev = aura->sdev;
+	struct request *rq;
+	struct scsi_cmnd *scmd;
+	struct ene_cdb cdb = {
+		.opcode		= 0xec,
+		.sig		= { 'A', 'S' },
+		.reg		= cpu_to_be16(reg),
+		.arg_count	= len,
+	};
+	int ret;
+
+	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_OUT, 0);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	/*
+	 * Stack memory is not DMA-safe (VMAP_STACK), so the payload is
+	 * copied into the per-device buffer first.
+	 */
+	memcpy(aura->tx, tx, len);
+	ret = blk_rq_map_kern(rq, aura->tx, len, GFP_NOIO);
+	if (ret)
+		goto out;
+
+	scmd = blk_mq_rq_to_pdu(rq);
+	scmd->cmd_len = sizeof(cdb);
+	memcpy(scmd->cmnd, &cdb, sizeof(cdb));
+	scmd->allowed = 1;
+	rq->timeout = 5 * HZ;
+	rq->rq_flags |= RQF_QUIET;
+
+	blk_execute_rq(rq, true);
+
+	/* GOOD status is zero in the fields we care about. */
+	ret = scmd->result ? -EIO : 0;
+out:
+	blk_mq_free_request(rq);
+	return ret;
+}
+
+static void asus_aura_work(struct work_struct *work)
+{
+	struct asus_aura *aura =
+		container_of(work, struct asus_aura, work);
+	u8 rgb[ARION_NUM_LEDS][ENE_RGB_LEN];
+	bool pending[ARION_NUM_LEDS];
+	bool any = false;
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * Fast path without the lock: if nothing is pending, return
+	 * before touching the device. A colour arriving right after
+	 * the check re-queues this work, so nothing is lost.
+	 */
+	for (int i = 0; i < ARION_NUM_LEDS; i++) {
+		if (READ_ONCE(aura->leds[i].pending)) {
+			any = true;
+			break;
+		}
+	}
+	if (!any)
+		return;
+
+	/* Snapshot the cached colours and clear the pending flags. */
+	spin_lock_irqsave(&aura->lock, flags);
+	for (int i = 0; i < ARION_NUM_LEDS; i++) {
+		pending[i] = aura->leds[i].pending;
+		aura->leds[i].pending = false;
+		memcpy(rgb[i], aura->leds[i].rgb, ENE_RGB_LEN);
+	}
+	spin_unlock_irqrestore(&aura->lock, flags);
+
+	if (!scsi_device_online(aura->sdev))
+		return;
+
+	/* Mode first: without it the device ignores the sequence. */
+	ret = ene_write_reg(aura, ENE_REG_MODE,
+			   &(u8){ ENE_MODE_STATIC }, 1);
+	if (ret)
+		goto err;
+
+	for (int i = 0; i < ARION_NUM_LEDS; i++) {
+		if (!pending[i])
+			continue;
+
+		/*
+		 * Both colour tables are written: firmware revisions
+		 * pull from either of them.
+		 */
+		ret = ene_write_reg(aura, ENE_REG_COLORS_EFFECT + i * ENE_RGB_LEN,
+				    rgb[i], ENE_RGB_LEN);
+		if (ret)
+			goto err;
+
+		ret = ene_write_reg(aura, ENE_REG_COLORS_DIRECT + i * ENE_RGB_LEN,
+				    rgb[i], ENE_RGB_LEN);
+		if (ret)
+			goto err;
+	}
+
+	ret = ene_write_reg(aura, ENE_REG_APPLY, &(u8){ ENE_APPLY }, 1);
+	if (ret)
+		goto err;
+
+	/* The change only takes effect after a save. */
+	ret = ene_write_reg(aura, ENE_REG_APPLY, &(u8){ ENE_SAVE }, 1);
+	if (ret)
+		goto err;
+
+	return;
+
+err:
+	/*
+	 * This runs detached from the LED core, which has no error
+	 * channel for brightness_set. The write is dropped and logged
+	 * ratelimited; the LED keeps the last successfully applied
+	 * colour until the next user update.
+	 */
+	dev_err_ratelimited(&aura->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);
+	struct asus_aura *aura = led->aura;
+	unsigned long flags;
+
+	/*
+	 * led_mc_calc_color_components() writes the shared subled_info
+	 * array. Sysfs stores are serialized by the LED core, but
+	 * trigger events call led_set_brightness() without that lock,
+	 * so compute and cache under the aura lock.
+	 */
+	spin_lock_irqsave(&aura->lock, flags);
+
+	led_mc_calc_color_components(mc, brightness);
+
+	led->rgb[ENE_WIRE_R] = led->subled[0].brightness;
+	led->rgb[ENE_WIRE_B] = led->subled[2].brightness;
+	led->rgb[ENE_WIRE_G] = led->subled[1].brightness;
+	led->pending = true;
+
+	spin_unlock_irqrestore(&aura->lock, flags);
+
+	schedule_work(&aura->work);
+}
+
+static int asus_aura_register_led(struct asus_aura *aura, int index)
+{
+	struct asus_aura_led *led = &aura->leds[index];
+	struct led_classdev *cdev = &led->mc_cdev.led_cdev;
+	char hctl[32];
+	int ret;
+
+	led->aura = aura;
+
+	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 = ENE_RGB_LEN;
+	led->mc_cdev.subled_info = led->subled;
+
+	/*
+	 * The sdev's H:C:T:L keeps the names unique when more than one
+	 * enclosure is connected; with a static name the LED core would
+	 * register a second enclosure's LEDs under renamed nodes
+	 * (asus-arion::led-0_1), the wrong device identity. The colons
+	 * are flattened to dashes, the color section stays empty
+	 * (multicolor, palette via multi_intensity) and the four
+	 * identical zones take a "-N" ordinal, as
+	 * Documentation/leds/leds-class.rst asks for.
+	 */
+	strscpy(hctl, dev_name(&aura->sdev->sdev_gendev));
+	strreplace(hctl, ':', '-');
+	cdev->name = kasprintf(GFP_KERNEL, "asus-arion-%s::led-%d", hctl, 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);
+
+	/*
+	 * NULL parent: a device reference on the sdev would block its
+	 * final release on unplug, which is what calls detach() to
+	 * unregister the LEDs. The cycle leaked 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 LEDs before cancelling the work: unregistering removes
+ * the sysfs attributes, so no new brightness_set can schedule the work
+ * afterwards. Cancelling first would leave a window where a brightness
+ * write requeues the work after cancel_work_sync() returned.
+ */
+static void asus_aura_release(struct asus_aura *aura, int num_leds)
+{
+	for (int i = 0; i < num_leds; i++) {
+		led_classdev_multicolor_unregister(&aura->leds[i].mc_cdev);
+		kfree(aura->leds[i].mc_cdev.led_cdev.name);
+	}
+
+	cancel_work_sync(&aura->work);
+	kfree(aura);
+}
+
+static int asus_aura_attach(struct scsi_device *sdev)
+{
+	struct asus_aura *aura;
+	int 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;
+
+	aura = kzalloc_obj(*aura, GFP_KERNEL);
+	if (!aura)
+		return SCSI_DH_NOMEM;
+
+	aura->sdev = sdev;
+	spin_lock_init(&aura->lock);
+	INIT_WORK(&aura->work, asus_aura_work);
+
+	for (int i = 0; i < ARION_NUM_LEDS; i++) {
+		ret = asus_aura_register_led(aura, i);
+		if (ret) {
+			asus_aura_release(aura, i);
+			return SCSI_DH_NOMEM;
+		}
+	}
+
+	sdev->handler_data = aura;
+	return SCSI_DH_OK;
+}
+
+static void asus_aura_detach(struct scsi_device *sdev)
+{
+	struct asus_aura *aura = sdev->handler_data;
+
+	if (!aura)
+		return;
+
+	asus_aura_release(aura, 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");
+MODULE_AUTHOR("Liang Haowen");
+MODULE_LICENSE("GPL");
-- 
2.55.0

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

[Ilpo Järvinen]

On Wed, 23 Sep 2026, Liang Haowen wrote:

> 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-0-0-0-0::led-0 through led-3. The
> H:C:T:L part of the sdev name keeps the names unique when more than
> one enclosure is connected, with its colons flattened to dashes. The
> color section stays empty, since multicolor LEDs enumerate their
> palette via multi_intensity, and the four identical zones use the
> function name with a "-N" ordinal, as the naming section in
> Documentation/leds/leds-class.rst asks for.
> 
> Protocol: a 16-byte vendor CDB (opcode 0xec, 'A' 'S' signature,
> BE16 register index, argument count in byte 13), modelled by
> struct ene_cdb. 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, wire 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 byte 13 is dropped and the device silently ignores the
> write (GOOD status, no error). scsi_execute_cmd() also offers no way
> to override the derived length. The request is therefore built with
> scsi_alloc_request(), which initializes the scsi_cmnd parts a
> passthrough needs, with cmd_len set to the CDB size, mirroring what
> SG_IO does from userspace. A 5 s timeout and a single retry-less
> quiet submission cover the register writes.
> 
> The write payload is copied into a per-device buffer before it is
> mapped with blk_rq_map_kern(), so no stack memory is ever mapped for
> DMA (VMAP_STACK).
> 
> brightness_set only caches the colour and marks the LED pending
> under a spinlock; led_mc_calc_color_components() runs under that
> lock too, because it writes the shared subled_info array and
> trigger events call led_set_brightness() without the led_access lock
> that serializes sysfs stores. A single work item per enclosure then
> snapshots the pending LEDs and runs one ENE sequence for all of
> them. 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. A re-queued run
> with nothing pending returns before touching the device, so it
> cannot wear the flash with a pointless SAVE, and the lock keeps a
> colour write from being reordered after its pending flag on weakly
> ordered architectures.
> 
> 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/rgb/leds-asus-aura-scsi.c | 388 +++++++++++++++++++++++++
>  1 file changed, 388 insertions(+)
>  create mode 100644 drivers/leds/rgb/leds-asus-aura-scsi.c
> 
> diff --git a/drivers/leds/rgb/leds-asus-aura-scsi.c b/drivers/leds/rgb/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000..eaba968
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-asus-aura-scsi.c
> @@ -0,0 +1,388 @@
> +// 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 on the disk's LUN. Matched by INQUIRY (vendor
> + * "ROG", model "ESD-S1C"); the sdev is not claimed, sd keeps owning the
> + * disk. Attach manually until the SCSI side agrees on an in-tree hook:
> + * echo asus_aura > /sys/block/sdX/device/dh_state
> + *
> + * Each of the 4 LEDs is a multicolor LED class device,
> + * /sys/class/leds/asus-arion-<H-C-T-L>::led-0..led-3. A colour change
> + * writes only that LED's slots in the ENE colour tables, then applies
> + * and saves; MODE must precede every sequence or the device ignores
> + * it. The register layout is described by struct ene_cdb.
> + *
> + * brightness_set (LED core fast path) only caches the colour and marks
> + * the LED pending under a spinlock; a single work item per enclosure
> + * snapshots all pending LEDs and runs one ENE sequence for them. One
> + * work item also serializes the sequences against each other: the
> + * MODE/colour/APPLY/SAVE chain must never interleave between
> + * concurrent LED updates.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/blk-mq.h>
> +#include <linux/blk_types.h>
> +#include <linux/blkdev.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.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"
> +
> +/* The colour table has 16 slots; 4 of them drive LEDs. */
> +#define ARION_NUM_LEDS		4
> +
> +/* ENE register map (BE16 on the wire). */
> +#define ENE_REG_MODE		0x8021	/* 1 = Static selects the colour tables */
> +#define ENE_REG_APPLY		0x80a0	/* 0x01 = apply, 0xaa = apply and save */
> +#define ENE_REG_COLORS_EFFECT	0x8160	/* colour table, 3 bytes per LED */
> +#define ENE_REG_COLORS_DIRECT	0x8100	/* second table some firmware uses */
> +
> +#define ENE_MODE_STATIC		1
> +#define ENE_APPLY		0x01
> +#define ENE_SAVE		0xaa
> +
> +/* The colour tables are R, B, G on the wire; the subleds are R, G, B. */
> +#define ENE_WIRE_R		0
> +#define ENE_WIRE_B		1
> +#define ENE_WIRE_G		2
> +#define ENE_RGB_LEN		3
> +
> +/*
> + * Vendor CDB: opcode, 'A''S' signature, register, argument count in
> + * byte 13. scsi_execute_cmd() cannot be used for it, see the commit
> + * message.
> + */
> +struct ene_cdb {
> +	u8		opcode;
> +	u8		sig[2];
> +	__be16		reg;
> +	u8		reserved[8];
> +	u8		arg_count;
> +	u8		pad[2];
> +} __packed;
> +
> +static_assert(sizeof(struct ene_cdb) == 16);

When you add new non-local stuff, you should be concious if you need to 
add a header for it. Here you added __packed and static_assert() both 
require a new include to be added.

It would be good for you to do one pass over your own code to check if you 
still miss other includes.

> +struct asus_aura_led {
> +	struct led_classdev_mc	mc_cdev;
> +	struct mc_subled	subled[ENE_RGB_LEN];
> +
> +	/* Cached colour, consumed by the work item. */
> +	u8			rgb[ENE_RGB_LEN];
> +	bool			pending;
> +
> +	struct asus_aura	*aura;
> +};
> +
> +struct asus_aura {
> +	struct scsi_device	*sdev;
> +	struct asus_aura_led	leds[ARION_NUM_LEDS];
> +
> +	/* Serializes the cached colours and pending flags. */
> +	spinlock_t		lock;
> +	struct work_struct	work;
> +
> +	/* DMA-safe staging buffer, the only memory ever mapped for DMA. */
> +	u8			tx[ENE_RGB_LEN];
> +};
> +
> +static int ene_write_reg(struct asus_aura *aura, u16 reg,
> +			 const u8 *tx, u8 len)
> +{
> +	struct scsi_device *sdev = aura->sdev;
> +	struct request *rq;
> +	struct scsi_cmnd *scmd;
> +	struct ene_cdb cdb = {
> +		.opcode		= 0xec,
> +		.sig		= { 'A', 'S' },
> +		.reg		= cpu_to_be16(reg),

Include for cpu_to_be16().

> +		.arg_count	= len,
> +	};
> +	int ret;
> +
> +	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_OUT, 0);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
> +
> +	/*
> +	 * Stack memory is not DMA-safe (VMAP_STACK), so the payload is
> +	 * copied into the per-device buffer first.
> +	 */
> +	memcpy(aura->tx, tx, len);
> +	ret = blk_rq_map_kern(rq, aura->tx, len, GFP_NOIO);
> +	if (ret)
> +		goto out;
> +
> +	scmd = blk_mq_rq_to_pdu(rq);
> +	scmd->cmd_len = sizeof(cdb);
> +	memcpy(scmd->cmnd, &cdb, sizeof(cdb));
> +	scmd->allowed = 1;
> +	rq->timeout = 5 * HZ;

I don't think moving it here from define was an improvements.

> +	rq->rq_flags |= RQF_QUIET;
> +
> +	blk_execute_rq(rq, true);
> +
> +	/* GOOD status is zero in the fields we care about. */
> +	ret = scmd->result ? -EIO : 0;
> +out:
> +	blk_mq_free_request(rq);
> +	return ret;
> +}
> +
> +static void asus_aura_work(struct work_struct *work)
> +{
> +	struct asus_aura *aura =
> +		container_of(work, struct asus_aura, work);

Lee did ask you to put these on a single line.

Also add the include for container_of().

> +	u8 rgb[ARION_NUM_LEDS][ENE_RGB_LEN];
> +	bool pending[ARION_NUM_LEDS];
> +	bool any = false;
> +	unsigned long flags;
> +	int ret;
> +
> +	/*
> +	 * Fast path without the lock: if nothing is pending, return
> +	 * before touching the device. A colour arriving right after
> +	 * the check re-queues this work, so nothing is lost.
> +	 */
> +	for (int i = 0; i < ARION_NUM_LEDS; i++) {

For anything that iterates over an array of entries, use ARRAY_SIZE(). 
Don't forget to add include for it.

> +		if (READ_ONCE(aura->leds[i].pending)) {

Include for READ_ONCE()

> +			any = true;
> +			break;
> +		}
> +	}
> +	if (!any)
> +		return;
> +
> +	/* Snapshot the cached colours and clear the pending flags. */
> +	spin_lock_irqsave(&aura->lock, flags);
> +	for (int i = 0; i < ARION_NUM_LEDS; i++) {

ARRAY_SIZE()

> +		pending[i] = aura->leds[i].pending;
> +		aura->leds[i].pending = false;
> +		memcpy(rgb[i], aura->leds[i].rgb, ENE_RGB_LEN);
> +	}
> +	spin_unlock_irqrestore(&aura->lock, flags);
> +
> +	if (!scsi_device_online(aura->sdev))
> +		return;
> +
> +	/* Mode first: without it the device ignores the sequence. */
> +	ret = ene_write_reg(aura, ENE_REG_MODE,
> +			   &(u8){ ENE_MODE_STATIC }, 1);

To make it easier for those two read this code, I suggest you add a local 
u8 and then use sizeof() to bind the len to its size. It would also avoid
the complex looking initializer.

> +	if (ret)
> +		goto err;
> +
> +	for (int i = 0; i < ARION_NUM_LEDS; i++) {

ARRAY_SIZE().

> +		if (!pending[i])
> +			continue;
> +
> +		/*
> +		 * Both colour tables are written: firmware revisions
> +		 * pull from either of them.
> +		 */
> +		ret = ene_write_reg(aura, ENE_REG_COLORS_EFFECT + i * ENE_RGB_LEN,
> +				    rgb[i], ENE_RGB_LEN);
> +		if (ret)
> +			goto err;
> +
> +		ret = ene_write_reg(aura, ENE_REG_COLORS_DIRECT + i * ENE_RGB_LEN,
> +				    rgb[i], ENE_RGB_LEN);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = ene_write_reg(aura, ENE_REG_APPLY, &(u8){ ENE_APPLY }, 1);
> +	if (ret)
> +		goto err;
> +
> +	/* The change only takes effect after a save. */
> +	ret = ene_write_reg(aura, ENE_REG_APPLY, &(u8){ ENE_SAVE }, 1);
> +	if (ret)
> +		goto err;
> +
> +	return;
> +
> +err:
> +	/*
> +	 * This runs detached from the LED core, which has no error
> +	 * channel for brightness_set. The write is dropped and logged
> +	 * ratelimited; the LED keeps the last successfully applied
> +	 * colour until the next user update.
> +	 */
> +	dev_err_ratelimited(&aura->sdev->sdev_gendev,

Still missing the include for dev_*() printing.

> +			    "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);
> +	struct asus_aura *aura = led->aura;
> +	unsigned long flags;
> +
> +	/*
> +	 * led_mc_calc_color_components() writes the shared subled_info
> +	 * array. Sysfs stores are serialized by the LED core, but
> +	 * trigger events call led_set_brightness() without that lock,
> +	 * so compute and cache under the aura lock.
> +	 */
> +	spin_lock_irqsave(&aura->lock, flags);
> +
> +	led_mc_calc_color_components(mc, brightness);
> +
> +	led->rgb[ENE_WIRE_R] = led->subled[0].brightness;
> +	led->rgb[ENE_WIRE_B] = led->subled[2].brightness;
> +	led->rgb[ENE_WIRE_G] = led->subled[1].brightness;
> +	led->pending = true;
> +
> +	spin_unlock_irqrestore(&aura->lock, flags);
> +
> +	schedule_work(&aura->work);
> +}
> +
> +static int asus_aura_register_led(struct asus_aura *aura, int index)
> +{
> +	struct asus_aura_led *led = &aura->leds[index];
> +	struct led_classdev *cdev = &led->mc_cdev.led_cdev;
> +	char hctl[32];
> +	int ret;
> +
> +	led->aura = aura;
> +
> +	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 = ENE_RGB_LEN;
> +	led->mc_cdev.subled_info = led->subled;
> +
> +	/*
> +	 * The sdev's H:C:T:L keeps the names unique when more than one
> +	 * enclosure is connected; with a static name the LED core would
> +	 * register a second enclosure's LEDs under renamed nodes
> +	 * (asus-arion::led-0_1), the wrong device identity. The colons
> +	 * are flattened to dashes, the color section stays empty
> +	 * (multicolor, palette via multi_intensity) and the four
> +	 * identical zones take a "-N" ordinal, as
> +	 * Documentation/leds/leds-class.rst asks for.
> +	 */
> +	strscpy(hctl, dev_name(&aura->sdev->sdev_gendev));
> +	strreplace(hctl, ':', '-');
> +	cdev->name = kasprintf(GFP_KERNEL, "asus-arion-%s::led-%d", hctl, 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);
> +
> +	/*
> +	 * NULL parent: a device reference on the sdev would block its
> +	 * final release on unplug, which is what calls detach() to
> +	 * unregister the LEDs. The cycle leaked 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 LEDs before cancelling the work: unregistering removes
> + * the sysfs attributes, so no new brightness_set can schedule the work
> + * afterwards. Cancelling first would leave a window where a brightness
> + * write requeues the work after cancel_work_sync() returned.
> + */
> +static void asus_aura_release(struct asus_aura *aura, int num_leds)
> +{
> +	for (int i = 0; i < num_leds; i++) {
> +		led_classdev_multicolor_unregister(&aura->leds[i].mc_cdev);
> +		kfree(aura->leds[i].mc_cdev.led_cdev.name);
> +	}
> +
> +	cancel_work_sync(&aura->work);
> +	kfree(aura);
> +}
> +
> +static int asus_aura_attach(struct scsi_device *sdev)
> +{
> +	struct asus_aura *aura;
> +	int 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;
> +
> +	aura = kzalloc_obj(*aura, GFP_KERNEL);
> +	if (!aura)
> +		return SCSI_DH_NOMEM;
> +
> +	aura->sdev = sdev;
> +	spin_lock_init(&aura->lock);
> +	INIT_WORK(&aura->work, asus_aura_work);
> +
> +	for (int i = 0; i < ARION_NUM_LEDS; i++) {
> +		ret = asus_aura_register_led(aura, i);
> +		if (ret) {
> +			asus_aura_release(aura, i);
> +			return SCSI_DH_NOMEM;
> +		}
> +	}
> +
> +	sdev->handler_data = aura;
> +	return SCSI_DH_OK;
> +}
> +
> +static void asus_aura_detach(struct scsi_device *sdev)
> +{
> +	struct asus_aura *aura = sdev->handler_data;
> +
> +	if (!aura)
> +		return;
> +
> +	asus_aura_release(aura, 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");
> +MODULE_AUTHOR("Liang Haowen");
> +MODULE_LICENSE("GPL");
> 

-- 
 i.