Re: [PATCH v5] HID: ayaneo: Add AYANEO 3 detachable controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: ayaneo: jiffies underflow in eject_store() causes
  infinite sleep
- [Low] HID: ayaneo: spurious -ENODEV errors on LED unregister during
  USB disconnect
--

commit 6aae1d65587c9f83b7545615c19722d11a6ced70
Author: Matías Martínez <hello@matias.me>

HID: ayaneo: Add AYANEO 3 detachable controller driver

This commit adds a HID driver for the AYANEO 3 detachable controller,
supporting module identification, software eject, configuration reset,
rumble intensity selection, and multicolor LED control on the vendor
interface while starting the other interfaces as generic HID devices.

> diff --git a/drivers/hid/hid-ayaneo.c b/drivers/hid/hid-ayaneo.c
> new file mode 100644
> index 0000000000000..a6c2dee793902
> --- /dev/null
> +++ b/drivers/hid/hid-ayaneo.c

[ ... ]

> +static ssize_t eject_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{

[ ... ]

> +		/*
> +		 * The firmware wants a minimum time between the eject
> +		 * command and the power cut that follows this write.
> +		 */
> +		if (!ret && time_before(jiffies, deadline)) {
> +			if (msleep_interruptible(jiffies_to_msecs(deadline -
> +								  jiffies)))
> +				ret = -EINTR;
> +		}
> +	}
> +	return ret ? ret : count;
> +}

[Severity: Medium]
Can deadline - jiffies underflow if jiffies advances past deadline between
the two volatile reads of jiffies?

Because the polling loop completes right around the 3000 ms deadline, a
timer tick or preemption between time_before(jiffies, deadline) and
deadline - jiffies can wrap the subtraction to a value near ULONG_MAX.
jiffies_to_msecs() then returns an unsigned int with the sign bit set,
which __msecs_to_jiffies() converts to MAX_JIFFY_OFFSET, hanging the caller
in msleep_interruptible() with aya->lock held until interrupted by a
signal.  Snapshotting jiffies into a local variable once before
time_before() avoids the underflow.

[ ... ]

> +static int ayaneo_led_set(struct led_classdev *cdev, enum led_brightness value)
> +{
> +	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
> +	struct ayaneo *aya = container_of(mc, struct ayaneo, mcled);
> +	int ret = 0, i;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) {
> +		led_mc_calc_color_components(mc, value);
> +		aya->brightness = min_t(unsigned int, value, 255);
> +		for (i = 0; i < 3; i++)
> +			aya->rgb[i] = min_t(unsigned int,
> +					    aya->subleds[i].brightness, 255);
> +
> +		ret = ayaneo_send_config(aya, 0);
> +		if (ret)
> +			hid_err(aya->hdev,
> +				"failed to update RGB config: %d\n", ret);
> +	}
> +	return ret;
> +}

[ ... ]

> +static int ayaneo_register_led(struct ayaneo *aya)
> +{
> +	struct led_classdev *cdev = &aya->mcled.led_cdev;
> +	int ret, i;

[ ... ]

> +	cdev->color = LED_COLOR_ID_RGB;
> +	cdev->brightness = 0;
> +	cdev->max_brightness = 255;
> +	cdev->brightness_set_blocking = ayaneo_led_set;
> +
> +	/*
> +	 * Not devm: the LED must be unregistered before hid_hw_stop(), or a
> +	 * racing brightness write could reach a torn-down transport.
> +	 */
> +	ret = led_classdev_multicolor_register(&aya->hdev->dev, &aya->mcled);

[Severity: Low]
Does unregistering the LED during USB disconnect or controller power-off
always log two spurious -ENODEV errors?

When the controller is unplugged or powered off via ayaneo-ec's
controller_power attribute, usb_disconnect() marks the USB device
USB_STATE_NOTATTACHED before ayaneo_remove() runs.
led_classdev_multicolor_unregister() then issues an LED_OFF write through
ayaneo_led_set(), where usb_submit_urb() fails with -ENODEV.  Because
ayaneo_led_set() logs hid_err() unconditionally and cdev->flags does not
include LED_HW_PLUGGABLE, both ayaneo_led_set() and
set_brightness_delayed_set_brightness() log an error on every disconnect:

drivers/leds/led-core.c:set_brightness_delayed_set_brightness() {
	...
	/* LED HW might have been unplugged, therefore don't warn */
	if (ret == -ENODEV && led_cdev->flags & LED_UNREGISTERING &&
	    led_cdev->flags & LED_HW_PLUGGABLE)
		return;

	if (ret < 0)
		dev_err(led_cdev->dev,
			"Setting an LED's brightness failed (%d)\n", ret);
}

In addition, when aya->configured is false and value is 0 (such as on
driver unbind or if sysfs_create_group() fails in ayaneo_probe()), the
LED_OFF write on unregister still sends a config command to the controller
despite the driver's policy of sending no traffic unless userspace asks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113212729.15925-1-hello@matias.me?part=1