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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Sanjay,

On Fri, Oct 16, 2026 at 11:01:28AM +1300, Sanjay Govind wrote:
> Xbox 360 wireless controllers expose information in the link and
> capabilities reports.
>
> Extract and use the vendor id for wireless controllers, and use
> the subtype to build a nicer device name and product id.
>
> Some xbox 360 controllers put a vid and pid into the stick capability
> data, so check if this was done, and pull the vid, pid and revision from
> there.
>
> To make sure this information is available when the input device is
> created, delay creating it until 500ms after the pad is reported as
> present. Some devices never send a capabilities report, so the
> timeout ensures they are still initialized.
...
> @@ -746,6 +768,8 @@ struct usb_xpad {
>  
>  	bool pad_present;
>  	bool input_created;
> +	unsigned int pad_gen;		/* bumped every time an x360w pad goes away */
> +	unsigned int input_gen;		/* pad_gen the input device was created for */
...
> +	u8 sub_type;
> +	u16 flags;
> +	u16 wireless_vid;
> +	u16 wireless_pid;
> +	u16 wireless_version;

I saw your reply to Sashiko about adding a spinlock and snapshotting
the wireless state for v10. Stepping back and looking at the driver as
a whole, grouping the wire-level discovery state into a dedicated
struct protected by a spinlock will also help clean up a few other
races and edge cases in the current state machine:

	struct xpad360w_pad_info {
		bool present;
		unsigned int gen;
		u8 sub_type;
		u8 flags;
		u16 vid;
		u16 pid;
		u16 version;
	};

> @@ -958,19 +988,16 @@ static void xpad360_process_packet(struct usb_xpad *xpad, struct input_dev *dev,
>  
>  static void xpad_presence_work(struct work_struct *work)
>  {
> -	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> +	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work.work);
> +	unsigned int gen;
>  	int error;
>  
> -	if (xpad->pad_present) {
> -		error = xpad_init_input(xpad);
> -		if (error) {
> -			/* complain only, not much else we can do here */
> -			dev_err(&xpad->dev->dev,
> -				"unable to init device: %d\n", error);
> -		} else {
> -			rcu_assign_pointer(xpad->x360w_dev, xpad->dev);
> -		}
> -	} else {
> +	/*
> +	 * Tear down the input device if the pad went away, or if it was
> +	 * replaced by a (possibly different) pad since the device was created.
> +	 */
> +	if (xpad->input_created &&
> +	    (!xpad->pad_present || xpad->input_gen != READ_ONCE(xpad->pad_gen))) {
>  		RCU_INIT_POINTER(xpad->x360w_dev, NULL);
>  		synchronize_rcu();
>  		/*
> @@ -979,6 +1006,19 @@ static void xpad_presence_work(struct work_struct *work)
>  		 */
>  		xpad_deinit_input(xpad);
>  	}
> +
> +	if (xpad->pad_present && !xpad->input_created) {
> +		gen = READ_ONCE(xpad->pad_gen);
> +		error = xpad_init_input(xpad);
> +		if (error) {
> +			/* complain only, not much else we can do here */
> +			dev_err(&xpad->intf->dev,
> +				"unable to init device: %d\n", error);
> +		} else {
> +			xpad->input_gen = gen;
> +			rcu_assign_pointer(xpad->x360w_dev, xpad->dev);
> +		}
> +	}
>  }

There are two issues here with how pad_gen and xpad_init_input()
interact:

1. In xpad360w_process_packet(), pad_gen is incremented on disconnect
before pad_present is set to false, whereas xpad_presence_work() checks
xpad->pad_present before reading pad_gen. If xpad_presence_work() races
with a disconnect packet, it can see pad_present == true, read the newly
incremented pad_gen (along with the zeroed metadata), and set
input_gen = pad_gen. When a new controller connects later, pad_gen is
not incremented on connect, so input_gen == pad_gen and the new
controller will never replace the bogus input device.

Bumping gen when a pad connects (or on both connect and disconnect) and
snapshotting the entire struct xpad360w_pad_info atomically under the
spinlock at the start of xpad_presence_work() avoids this completely.

2. xpad_init_input() sleeps while registering the input and LED devices.
If the pad disconnects (or is replaced by another pad, which pushes the
delayed work timer out by 500ms) while xpad_init_input() is executing,
xpad_presence_work() will still publish xpad->x360w_dev upon return.
After xpad_init_input() succeeds, please re-check under the spinlock
that pad_info.present is still true and pad_info.gen == info.gen before
setting xpad->input_gen and assigning xpad->x360w_dev; otherwise tear
the newly created device down via xpad_deinit_input(xpad).

> @@ -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, 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;
> +		}
> +
> +		/*
> +		 * Also reschedule if the input device does not match the
> +		 * presence state, e.g. because pending work was cancelled
> +		 * on suspend.
> +		 */
> +		if (xpad->pad_present != present ||
> +		    xpad->input_created != present) {
>  			xpad->pad_present = present;
> -			schedule_work(&xpad->work);
> +			if (present) {
> +				/*
> +				 * Delay marking device as present, so we can make sure
> +				 * we have received all the information from the capabilities
> +				 * report. Some devices don't send one, so the delay
> +				 * guarantees that these devices are still initialized.
> +				 */
> +				mod_delayed_work(system_percpu_wq,
> +						 &xpad->work, msecs_to_jiffies(500));
> +			} else {
> +				mod_delayed_work(system_percpu_wq, &xpad->work, 0);
> +			}
> +		}
>  	}

Checking xpad->input_created != present here causes a problem during the
500ms discovery window: while waiting for the timer to expire (or while
xpad_init_input() is running), pad_present is true and input_created is
still false. Any status packet (data[0] & 0x08) arriving during that
window with present == true (for example, headset status bit 01.6
changing) will see input_created != present and push the 500ms timer
back again.

The only reason pad_present and input_created can get out of sync across
suspend is that xpad_suspend() calls xpad360w_stop_input(), which cancels
any pending delayed work via cancel_delayed_work_sync(&xpad->work).
Instead of checking input_created in the IRQ handler, you can simply
reconcile the state in xpad_suspend() after stopping input and sending
the optional poweroff command:

	if (xpad->xtype == XTYPE_XBOX360W) {
		xpad360w_stop_input(xpad);

		if (auto_poweroff && xpad->pad_info.present)
			xpad360w_poweroff_controller(xpad);

		xpad->pad_info.present = xpad->input_created;
	}

Also, when a pad disconnects (xpad->pad_info.present && !present), it
would be good to do RCU_INIT_POINTER(xpad->x360w_dev, NULL) immediately
in xpad360w_process_packet() so no further input reports are routed to
the old input_dev while teardown work is pending (especially if another
pad connects right away and reschedules the delayed work for +500ms).

> +	/* Capabilities report */
> +	if (len >= 21 && data[0] == 0x00 && data[1] == 0x05 && data[5] == 0x12) {
> +		xpad->flags |= data[20];
> +		/*
> +		 * A bunch of vendors started putting vids and pids
> +		 * into capabilities data because they can't be
> +		 * retrieved by xinput easily.
> +		 * Not all of them do though, so check the vids match
> +		 * before extracting that info. A vid of 0 means the link
> +		 * report did not provide one, so there is nothing to match.
> +		 */
> +		if (xpad->wireless_vid &&
> +		    get_unaligned_le16(data + 10) == xpad->wireless_vid &&
> +		    get_unaligned_le16(data + 12)) {
> +			xpad->wireless_pid = get_unaligned_le16(data + 12);
> +			xpad->wireless_version = get_unaligned_le16(data + 14);
>  		}
>  	}

Once the capabilities report arrives, we have all the identification
data the controller will ever send. If the presence work is currently
waiting for the 500ms fallback timeout, we can expedite it:

	if (delayed_work_pending(&xpad->work))
		mod_delayed_work(system_percpu_wq, &xpad->work, 0);

That way controllers that respond to the capabilities inquiry are
registered (and have their LEDs initialized) immediately without waiting
the full 500ms, while controllers that do not respond still fall back to
the 500ms timeout.

> +static int xpad_inquiry_pad_capabilities(struct usb_xpad *xpad)
> +{
...
> +	return xpad_try_sending_next_out_packet(xpad);
> +}

Since the caller ignores the return value, please make
xpad_inquiry_pad_capabilities() return void (matching
xpadone_ack_mode_report() and xpad360w_poweroff_controller()).

> @@ -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;
...
> +		case SUBTYPE_PRO_GUITAR:
> +			input_dev->name = "Xbox 360 Wireless Rock Band Pro Guitar";
> +			break;
> +		}
>  	}

Please factor this Xbox 360 wireless ID/name setup out into a small
helper function (taking input_dev and the snapshotted
const struct xpad360w_pad_info *info) so xpad_init_input() stays
concise.

Thanks.

-- 
Dmitry