Re: [PATCH RFC v4 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] leds: data race on subled_info array causes torn color updates
- [Low] leds: build failure due to undefined macro kzalloc_obj
- [Low] leds: build failure due to missing argument in blk_rq_map_kern()
--

commit e6ce9b3b67cc2aa4897215acc2c62600810790a8
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 that
exposes 4 independently addressable LEDs via the LED multicolor class. It
uses a SCSI device handler matched by vendor INQUIRY to send custom ENE
sequences over passthrough SCSI commands.

> diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000000000..089cc3dcee105
> --- /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 cause a build failure? The kernel API blk_rq_map_kern() requires 5
arguments, where the first parameter should be the struct request_queue *q,
but it is called here with only 4 arguments.

> +		if (ret)
> +			goto out;
> +	}
[ ... ]
> +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_zone *zone = led->zone;
> +	unsigned long flags;
> +
> +	led_mc_calc_color_components(mc, brightness);
> +
> +	spin_lock_irqsave(&zone->lock, flags);

[Severity: Medium]
Could this lead to a data race on the subled_info array? The LED core allows
concurrent brightness_set callbacks for the same LED (for instance, from
sysfs and a hardware trigger). The helper led_mc_calc_color_components()
modifies the shared mc->subled_info array locklessly before the driver
acquires its spinlock. If multiple threads update the brightness
concurrently, they might interleave their writes to the array, causing the
winning thread to read a mix of old and new color components.

> +	/* 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;
> +	zone->dirty |= BIT(led->index);
> +	spin_unlock_irqrestore(&zone->lock, flags);
> +
> +	schedule_work(&zone->work);
> +}
[ ... ]
> +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 code fail to compile? It looks like it invokes an undefined macro
kzalloc_obj() instead of the standard kzalloc() function to allocate memory.

> +	if (!zone)
> +		return SCSI_DH_NOMEM;
> +	zone->sdev = sdev;

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