Re: [PATCH RFC v3 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:
- [Medium] LED class device name violates UAPI naming standards and breaks userspace parsing.
- [Low] Compilation failure due to undefined macro `kzalloc_obj`.
- [Low] Compilation failure due to missing argument in `blk_rq_map_kern()` call.
--

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

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

This commit introduces a driver for ASUS ROG external NVMe enclosures. It
exposes the Aura LEDs connected to an ENE controller via vendor SCSI
commands, implementing multicolor LED class devices for each LED.

> diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000000000..3ff33615a7934
> --- /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]
Will this compile successfully? Looking at blk_rq_map_kern() in
block/blk-map.c, the function expects 5 arguments with the request queue as
the first parameter, but this call only provides 4. Should this pass
sdev->request_queue as the first argument?

[ ... ]
> +static int asus_aura_register_led(struct asus_aura_zone *zone, int index)
> +{
[ ... ]
> +	/*
> +	 * 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.
> +	 */
> +	cdev->name = kasprintf(GFP_KERNEL, "asus-arion-%s:led%d",
> +			       dev_name(&zone->sdev->sdev_gendev), index);

[Severity: Medium]
Does this format string comply with the standard LED class device naming
conventions? Since the SCSI dev_name() format typically introduces multiple
colons (like 0:0:0:0), and standard LED userspace tools expect the naming
scheme devicename:color:function, will the extra colons break userspace
parsing or udev rules?

[ ... ]
> +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]
Will this cause a build failure? It appears kzalloc_obj is not a defined
kernel allocation macro. Should this be using kzalloc() with sizeof(*zone)
instead?

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