Re: [RFC v8 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 4 potential issue(s) to consider:
- [High] leds: memory corruption due to unaligned DMA buffer tx sharing cacheline
- [Low] leds: build failure due to undefined macro kzalloc_obj
- [Low] leds: build failure due to missing argument in blk_rq_map_kern()
- [Low] leds: build failure due to missing size argument in strscpy()
--

commit 823a081d978e63fdae64dbf79945582f7cc88fe2
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 driver for ASUS Aura RGB LEDs present on ROG external
NVMe enclosures. It operates by registering a SCSI device handler to intercept
and issue vendor-specific SCSI commands to control the LEDs.

> diff --git a/drivers/leds/rgb/leds-asus-aura-scsi.c b/drivers/leds/rgb/leds-asus-aura-scsi.c
> index 0000000000000..eaba968598dbf 100644
> --- /dev/null
> +++ b/drivers/leds/rgb/leds-asus-aura-scsi.c
[ ... ]
> +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];
> +};

[Severity: High]
Can this corrupt memory on non-cache-coherent architectures? The tx buffer is
a 3-byte array sharing a cacheline with the lock and work members. If mapped
for DMA, a cacheline invalidation during the DMA operation could clobber
concurrent CPU writes to the lock (e.g., from asus_aura_set()), leading to
lock corruption and crashes. Should this buffer be isolated to its own
cacheline using ____cacheline_aligned?

[ ... ]
> +static int ene_write_reg(struct asus_aura *aura, u16 reg,
> +			 const u8 *tx, u8 len)
> +{
[ ... ]
> +	/*
> +	 * 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);

[Severity: Low]
Does this code compile? The blk_rq_map_kern() function requires five
arguments, including the request queue (struct request_queue *q) as the
first parameter, but only four are provided here.

[ ... ]
> +static int asus_aura_register_led(struct asus_aura *aura, int index)
> +{
[ ... ]
> +	/*
> +	 * 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));

[Severity: Low]
Does this build? The strscpy() function takes three arguments: destination,
source, and the size of the destination buffer. The size argument is missing
here.

[ ... ]
> +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);

[Severity: Low]
Is kzalloc_obj() a defined macro in this context? It seems its use here
could prevent the driver from compiling. Should this use kzalloc() with
sizeof(*aura) instead?

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