Re: [PATCH v2 1/1] Input: xpad - add support for Azeron devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The newly added Azeron device IDs are not sorted by vendor ID, violating the explicit comments in the driver.

Pre-existing issues:
- [High] Azeron devices will stall (-EPIPE) on FF_RUMBLE packets, triggering a pre-existing infinite URB retry loop.
--

commit 32ac579407e3b61d52176dbd3f7d52733eb47d44
Author: Roberts Kursitis <roberts.kursitis@azeron.eu>

Input: xpad - add support for Azeron devices

Add USB Vendor and Product IDs for the Azeron devices (Cyro,
Cyborg, Classic/Compact, Cyro Lefty, Cyborg II and Keyzen). Adds
a QUIRK_NO_LED flag to skip LED initialization since these devices
reject the Xbox 360 LED magic packet with a stall.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..4efcd16c3713c 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c

[ ... ]

> @@ -289,6 +290,12 @@ static const struct xpad_device {
>  	{ 0x15e4, 0x3f0a, "Xbox Airflo wired controller", 0, XTYPE_XBOX360 },
>  	{ 0x15e4, 0x3f10, "Batarang Xbox 360 controller", 0, XTYPE_XBOX360 },
>  	{ 0x162e, 0xbeef, "Joytech Neo-Se Take2", 0, XTYPE_XBOX360 },
> +	{ 0x16d0, 0x1103, "Azeron Cyro", QUIRK_NO_LED, XTYPE_XBOX360 },

[Severity: High]
This is a pre-existing issue, but mapping these devices as XTYPE_XBOX360 will
unconditionally enable the FF_RUMBLE capability in xpad_init_ff(). 

Since you noted in the commit message that these devices stall (-EPIPE) on LED
packets, will they also stall if userspace sends a rumble effect packet?

If they do stall on rumble, this will trigger a pre-existing bug in
xpad_irq_out(), which handles the -EPIPE status by falling through to the
default case:

drivers/input/joystick/xpad.c:xpad_irq_out() {
    switch (status) {
    ...
    default:
        dev_dbg(dev, "%s - nonzero urb status received: %d\n", __func__, status);
        break;
    }
    
    if (xpad->irq_out_active) {
        usb_anchor_urb(urb, &xpad->irq_out_anchor);
        error = usb_submit_urb(urb, GFP_ATOMIC);
    ...
}

Because xpad->irq_out_active is not cleared on -EPIPE, the driver will
infinitely resubmit the stalled OUT URB, creating an interrupt storm.

Should a QUIRK_NO_RUMBLE or similar be added to prevent rumble packets
from being sent to these devices?

[Severity: Low]
this isn't a bug, but the new entries with vendor ID 0x16d0 are placed before
the 0x1689 entries. Should these be moved down to maintain the numerical
sorting of the xpad_device array?

> +	{ 0x16d0, 0x113c, "Azeron Cyborg", QUIRK_NO_LED, XTYPE_XBOX360 },
> +	{ 0x16d0, 0x1192, "Azeron Classic/Compact", QUIRK_NO_LED, XTYPE_XBOX360 },
> +	{ 0x16d0, 0x1212, "Azeron Cyro Lefty", QUIRK_NO_LED, XTYPE_XBOX360 },
> +	{ 0x16d0, 0x12f7, "Azeron Cyborg II", QUIRK_NO_LED, XTYPE_XBOX360 },
> +	{ 0x16d0, 0x13ea, "Azeron Keyzen", QUIRK_NO_LED, XTYPE_XBOX360 },
>  	{ 0x1689, 0xfd00, "Razer Onza Tournament Edition", 0, XTYPE_XBOX360 },
>  	{ 0x1689, 0xfd01, "Razer Onza Classic Edition", 0, XTYPE_XBOX360 },

[ ... ]

> @@ -532,6 +539,7 @@ static const struct usb_device_id xpad_table[] = {
>  	XPAD_XBOXONE_VENDOR(0x1532),		/* Razer Wildcat */
>  	XPAD_XBOX360_VENDOR(0x15e4),		/* Numark Xbox 360 controllers */
>  	XPAD_XBOX360_VENDOR(0x162e),		/* Joytech Xbox 360 controllers */
> +	XPAD_XBOX360_VENDOR(0x16d0),		/* Azeron controllers */

[Severity: Low]
this isn't a bug, but similarly here, should the 0x16d0 vendor block be
placed after 0x1689 to keep the list sorted by vendor ID as requested by the
comment at the top of xpad_table?

>  	XPAD_XBOX360_VENDOR(0x1689),		/* Razer Onza */
>  	XPAD_XBOX360_VENDOR(0x17ef),		/* Lenovo */
>  	XPAD_XBOX360_VENDOR(0x1949),		/* Amazon controllers */

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619132821.18764-1-roberts.kursitis@azeron.eu?part=1