Re: [PATCH v10] Input: xpad - overhaul device data for wireless devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Input: xpad - premature registration in xpad_presence_work() on reconnect
- [Medium] Input: xpad - stale pad_info state on false-to-true presence and suspend
--

commit 4f28e1c3111d72b61c99d3360763b2634a4795c8
Author: Sanjay Govind <sanjay.govind9@gmail.com>

Input: xpad - overhaul device data for wireless devices

This commit updates Xbox 360 wireless controller handling to defer input
device registration until link and capabilities discovery completes or a
500ms timeout expires, setting controller-specific vendor ID, product ID,
revision, and device name from the discovery reports. It also adds spinlock
protection, generation tracking, RCU pointer teardown on disconnect, and
suspend state reconciliation.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..41348e62fe19f 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -958,19 +995,20 @@ 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);
> +	struct xpad360w_pad_info info;
>  	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 {
> +	/*
> +	 * Take a consistent snapshot, as the reports are processed
> +	 * concurrently from the irq handler.
> +	 */
> +	scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +		info = xpad->pad_info;
> +	}
> +
> +	/* Remove the input device if its controller disconnected or changed. */
> +	if (xpad->input_created && (!info.present || xpad->input_gen != info.gen)) {
>  		RCU_INIT_POINTER(xpad->x360w_dev, NULL);
>  		synchronize_rcu();
>  		/*
> @@ -979,6 +1017,26 @@ static void xpad_presence_work(struct work_struct *work)
>  		 */
>  		xpad_deinit_input(xpad);
>  	}
> +
> +	if (info.present && !xpad->input_created) {
> +		error = xpad_init_input(xpad, &info);
> +		if (error) {
> +			/* complain only, not much else we can do here */
> +			dev_err(&xpad->intf->dev,
> +				"unable to init device: %d\n", error);
> +		} else {
> +			scoped_guard(spinlock_irqsave, &xpad->x360w_lock) {
> +				if (xpad->pad_info.present &&
> +				    xpad->pad_info.gen == info.gen) {
> +					xpad->input_gen = info.gen;
> +					rcu_assign_pointer(xpad->x360w_dev, xpad->dev);
> +					return;
> +				}
> +			}
> +
> +			xpad_deinit_input(xpad);
> +		}
> +	}
>  }
>  
>  /*
> @@ -995,19 +1053,83 @@ static void xpad_presence_work(struct work_struct *work)
>   * 01.1 - Pad state (Bytes 4+) valid
>   *
>   */
> -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 xpad360w_pad_info *info = &xpad->pad_info;
>  	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) {
> -			xpad->pad_present = present;
> -			schedule_work(&xpad->work);
> +		guard(spinlock_irqsave)(&xpad->x360w_lock);
> +
> +		if (info->present != present) {
> +			if (!present) {
> +				RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> +				/* Forget everything we learned about the old pad */
> +				*info = (struct xpad360w_pad_info) { .gen = info->gen };
> +			}
> +			info->present = present;
> +			if (present) {
> +				info->gen++;
> +				/*
> +				 * Allow discovery to complete; some pads never
> +				 * send capabilities.
> +				 */
> +				mod_delayed_work(system_percpu_wq,
> +						 &xpad->work, msecs_to_jiffies(500));
> +			} else {
> +				mod_delayed_work(system_percpu_wq, &xpad->work, 0);
> +			}
> +		}
> +	}
> +
> +	/* Link report */
> +	if (len >= 26 && data[0] == 0x00 && data[1] == 0x0F) {
[ ... ]
> +	/* Capabilities report */
> +	if (len >= 21 && data[0] == 0x00 && data[1] == 0x05 && data[5] == 0x12) {
> +		guard(spinlock_irqsave)(&xpad->x360w_lock);
> +
> +		info->flags |= data[20];
> +		/*
> +		 * Some vendors encode identification in the stick capabilities.
> +		 * Only trust it when the VID matches the link report.
> +		 */
> +		if (info->vid &&
> +		    get_unaligned_le16(data + 10) == info->vid &&
> +		    get_unaligned_le16(data + 12)) {
> +			info->pid = get_unaligned_le16(data + 12);
> +			info->version = get_unaligned_le16(data + 14);
> +		}
> +
> +		if (info->present && delayed_work_pending(&xpad->work))
> +			mod_delayed_work(system_percpu_wq, &xpad->work, 0);
>  	}

[Severity: Medium]
Can a disconnect followed quickly by a reconnect cause xpad_presence_work()
to register the new controller prematurely using the fallback receiver
identity before link and capabilities discovery has completed?

Both immediate disconnect teardown (delay 0) and 500ms deferred discovery
registration share the single xpad->work delayed_work item:

1. When a controller disconnects (data[0] & 0x08 with present == false),
   xpad360w_process_packet() clears xpad->pad_info, sets
   info->present = false, and queues xpad->work with delay 0.

2. Once a worker thread dequeues xpad->work to run xpad_presence_work(),
   a reconnect packet (present == true) can arrive before
   xpad_presence_work() acquires xpad->x360w_lock.

3. In xpad360w_process_packet(), the reconnect sets info->present = true,
   increments info->gen, and calls:

       mod_delayed_work(system_percpu_wq, &xpad->work,
                        msecs_to_jiffies(500));

   Because the running worker already cleared WORK_STRUCT_PENDING_BIT when
   dequeuing xpad->work, mod_delayed_work() only arms the 500ms timer for a
   subsequent run and does not delay the already-executing
   xpad_presence_work().

4. The executing xpad_presence_work() then snapshots xpad->pad_info while
   info.present is true and info.gen has the new generation, but before the
   link (00 0F) and capabilities (00 05) reports have arrived, so vid, pid,
   sub_type, version, and flags are still zero.

5. After tearing down the old input device in synchronize_rcu() and
   xpad_deinit_input(xpad), xpad->input_created is false. Because
   xpad_presence_work() does not check delayed_work_pending(&xpad->work) or
   re-snapshot xpad->pad_info after teardown, it immediately enters:

       if (info.present && !xpad->input_created) {
               error = xpad_init_input(xpad, &info);

   using the unpopulated pre-teardown snapshot.

6. While xpad_deinit_input() and xpad_init_input() run without holding
   xpad->x360w_lock, incoming link and capabilities reports update
   xpad->pad_info in place without changing xpad->pad_info.gen. In addition,
   if a capabilities report (00 05) arrives while info->present is true
   before a link report has populated info->sub_type,
   xpad360w_process_packet() expedites xpad->work to delay 0 without
   checking whether info->sub_type has been set.

7. The post-registration check in xpad_presence_work() only verifies
   xpad->pad_info.present && xpad->pad_info.gen == info.gen, which succeeds
   and publishes xpad->x360w_dev with the fallback receiver identity. When
   the 500ms timer later fires, xpad->input_created is already true and
   xpad->input_gen == info.gen, so the discovered controller identity is
   never applied. (This also appears to match the transient receiver-name
   fallback observed during battery-removal/reconnect testing.)

[ ... ]
> @@ -2263,8 +2471,20 @@ static int xpad_suspend(struct usb_interface *intf, pm_message_t message)
>  		 * Unless explicitly disabled, power them down
>  		 * so they don't just sit there flashing.
>  		 */
> -		if (auto_poweroff && xpad->pad_present)
> +		if (auto_poweroff && xpad->pad_info.present)
>  			xpad360w_poweroff_controller(xpad);
> +
> +		/* Finish teardown if suspend cancelled work for an old pad. */
> +		if (xpad->input_created &&
> +		    (!xpad->pad_info.present ||
> +		     xpad->input_gen != xpad->pad_info.gen)) {
> +			RCU_INIT_POINTER(xpad->x360w_dev, NULL);
> +			synchronize_rcu();
> +			xpad_deinit_input(xpad);
> +		}
> +
> +		/* A cancelled discovery must be restarted by the resume inquiry. */
> +		xpad->pad_info.present = xpad->input_created;
>  	} else {
>  		guard(mutex)(&input->mutex);
>  

[Severity: Medium]
Could xpad->pad_info retain stale state across presence transitions or
suspend/resume cycles?

There appear to be three paths where pad_info and presence state can get out
of sync in drivers/input/joystick/xpad.c:

1. In xpad360w_process_packet(), pad_info is only cleared on a true-to-false
   presence transition:

       if (info->present != present) {
               if (!present) {
                       RCU_INIT_POINTER(xpad->x360w_dev, NULL);
                       /* Forget everything we learned about the old pad */
                       *info = (struct xpad360w_pad_info) { .gen = info->gen };
               }

   However, the link report (00 0F) and capabilities report (00 05) handlers
   populate info->sub_type, info->vid, info->pid, info->version, and
   info->flags without checking info->present. If a late link or capabilities
   packet arrives after a disconnect packet (08 00) has already set
   info->present = false, those fields remain populated across the next
   false-to-true presence transition and would be used if the next
   controller falls back to the 500ms timeout.

2. In xpad_suspend(), if discovery work is cancelled by
   xpad360w_stop_input() before xpad->input_created becomes true (or after
   tearing down an old generation's input device in xpad_suspend()), setting:

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

   sets pad_info.present to false without clearing any vid, pid, sub_type,
   version, or flags already written into pad_info. A subsequent 08 00
   packet after resume sees info->present == false and does not clear
   pad_info, and the next 08 80 (false-to-true) transition preserves those
   stale fields.

3. Conversely, when xpad->input_created is true on entry to xpad_suspend(),
   xpad_suspend() stops input, powers off the controller via
   xpad360w_poweroff_controller() when auto_poweroff is set, and leaves
   xpad->pad_info.present = true. If xpad_resume() runs without a full reset
   (for instance, if a later interface fails during usb_suspend_both()
   rollback) and a different controller is present when
   xpad_inquiry_pad_presence() runs, the resulting 08 80 presence packet will
   see info->present == true and will not increment info->gen or schedule
   xpad->work to replace the old input_dev.

Would it make sense to clear the discovery fields on the false-to-true
presence transition (and require info->present when processing link and
capabilities reports), and how should xpad_suspend() handle pad_info when an
existing controller is powered off on suspend?

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