Re: [PATCH v9] xpad: Overhaul device data for wireless devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] input: data race on x360w pad metadata between URB completion and workqueue
--

commit 623a5a67bf1fb99bf182903ceaa0fbfb964bfc7b
Author: Sanjay Govind <sanjay.govind9@gmail.com>

xpad: Overhaul device data for wireless devices
    
This commit updates Xbox 360 wireless controllers to extract and use vendor
IDs, subtypes, and product IDs from link and capability reports. It delays
creating the input device until 500ms after the pad is reported present to
ensure this information is available.

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

[ ... ]

> @@ -995,18 +1035,98 @@ static void xpad_presence_work(struct work_struct *work)
> -static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> +static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data, u32 len)
>  {
>  	struct input_dev *dev;
>  	bool present;
> +	u16 parsed_vid;
>  
>  	/* Presence change */
>  	if (data[0] & 0x08) {
>  		present = (data[1] & 0x80) != 0;
>  
> -		if (xpad->pad_present != present) {
> +		if (xpad->pad_present && !present) {
> +			/* Forget everything we learned about the old pad */
> +			WRITE_ONCE(xpad->pad_gen, xpad->pad_gen + 1);
> +			xpad->sub_type = 0;
> +			xpad->flags = 0;
> +			xpad->wireless_vid = 0;
> +			xpad->wireless_pid = 0;
> +			xpad->wireless_version = 0;

[Severity: Low]
Can these pad metadata fields race with xpad_init_input() during device
initialization?

The URB completion handler xpad360w_process_packet() runs asynchronously in
softirq context and modifies these fields without holding any synchronization
primitive.

[ ... ]

> @@ -1943,8 +2088,60 @@ static int xpad_init_input(struct usb_xpad *xpad)
>  	usb_to_input_id(xpad->udev, &input_dev->id);
>  
>  	if (xpad->xtype == XTYPE_XBOX360W) {
> -		/* x360w controllers and the receiver have different ids */
> -		input_dev->id.product = 0x02a1;
> +		if (xpad->wireless_vid)
> +			input_dev->id.vendor = xpad->wireless_vid;
> +		if (xpad->wireless_pid)
> +			input_dev->id.product = xpad->wireless_pid;

[Severity: Low]
Since the delayed xpad_presence_work can execute xpad_init_input() on another
CPU concurrently with a disconnect packet or late capabilities report, could
this read torn, zeroed, or partially updated device identification data?

The 500ms delay defers the work, but does not serialize it against subsequent
URB completions. This might cause the input device to be momentarily
registered with an incorrect ID (e.g., 0000:0000) or name before the
subsequent work iteration unregisters it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015220126.102766-3-sanjay.govind9@gmail.com?part=1