Re: [PATCH RFC v5 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 3 potential issue(s) to consider:
- [Low] block: build failure due to missing request_queue argument in blk_rq_map_kern()
- [Low] leds: build failure due to undefined macro kzalloc_obj()
- [Low] leds: UAPI naming convention violation for LED class device
--

commit fe7900b55dc69157a3ef071f13e8783fac33da59
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 SCSI driver for ASUS Aura RGB LEDs found on ROG
NVMe enclosures. It exposes each LED as a multicolor LED class device by
routing vendor SCSI commands. Updates are batched and scheduled via a
workqueue to prevent concurrent writes.

> diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000000000..a1e31dc3f0101
> --- /dev/null
> +++ b/drivers/leds/leds-asus-aura-scsi.c
[ ... ]
> +	if (arg_count) {
> +		ret = blk_rq_map_kern(rq, (void *)data, arg_count, GFP_NOIO);

[Severity: Low]
Does this build against the current block layer?

The blk_rq_map_kern() function requires five arguments, including a
struct request_queue pointer. Only four arguments are provided here.

> +		if (ret)
> +			goto out;
> +	}
[ ... ]
> +	/*
> +	 * Include the sdev's H:C:T:L: every enclosure gets its own SCSI
> +	 * host, so the names stay unique when more than one is connected.
> +	 * With a static name the LED core would register the second
> +	 * enclosure's LEDs under renamed nodes (asus-arion:led0_1), which
> +	 * is the wrong device identity. The names are per-attachment, like
> +	 * sd X letters, and userspace is expected to enumerate.
> +	 *
> +	 * dev_name() renders the sdev as H:C:T:L; the extra colons would
> +	 * break the devicename:color:function scheme userspace parses LED
> +	 * class names with, so they are flattened to dashes and the name
> +	 * keeps exactly one separator.
> +	 */
> +	strscpy(hctl, dev_name(&zone->sdev->sdev_gendev), sizeof(hctl));
> +	strreplace(hctl, ':', '-');
> +	cdev->name = kasprintf(GFP_KERNEL, "asus-arion-%s:led%d", hctl, index);

[Severity: Low]
Does this naming scheme violate the UAPI LED naming convention?

The standard format requires two colons (devicename:color:function), as
documented in Documentation/leds/leds-class.rst. The comment above states
this follows the devicename:color:function convention, but the generated
name (e.g., asus-arion-xxxx:led0) only contains a single colon.

> +	if (!cdev->name)
> +		return -ENOMEM;
[ ... ]
> +	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 to be undefined, which will cause a build failure.
Should this just be kzalloc(sizeof(*zone), GFP_KERNEL)?

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

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