Re: [PATCH] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] HID: nintendo-switch2-usb: unused schedule and error bug in
  switch2_bulk_in()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] HID: nintendo: AB-BA deadlock in switch2_usb_shutdown() [1]
--

commit 6f6e1e4dbbc575c0862f97bc1dde9a1e20a88deb
Author: Vicki Pfau <vi@endrift.com>

HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work

This commit moves the scheduling of message_in_work inside bulk_in_lock
in switch2_bulk_in() and adds a shutdown check in
switch2_usb_message_in_work().

Link: https://lore.kernel.org/linux-input/a2dc00e7-9cab-4315-87a4-795f256c1e03@endrift.com/T/

> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 623473f3d2161..5174a5c2dbeca 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c
> @@ -71,16 +71,14 @@ static void switch2_bulk_in(struct urb *urb)
>  	}
>  
>  	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> -	if (ns2_usb->shutdown)
> -		schedule = false;
> -
>  	for (i = 0; i < NS2_IN_URBS; i++) {
>  		int err;
>  		struct switch2_urb *ns2_urb;
>  
>  		if (ns2_usb->bulk_in[i].urb == urb) {
> -			if (schedule) {
> +			if (!ns2_usb->shutdown) {
>  				ns2_usb->bulk_in[i].state = NS2_URB_IN;
> +				schedule_work(&ns2_usb->message_in_work);
>  				continue;
>  			} else {
>  				ns2_usb->bulk_in[i].state = NS2_URB_FREE;
> @@ -115,9 +113,6 @@ static void switch2_bulk_in(struct urb *urb)
>  		}
>  	}
>  	spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> -
> -	if (schedule)
> -		schedule_work(&ns2_usb->message_in_work);
>  }

[Severity: High]
Does replacing the schedule check with !ns2_usb->shutdown cause failed
URBs to be processed as valid input and leave schedule unused?

Earlier in switch2_bulk_in(), schedule is only set to true when
urb->status is 0, while unhandled non-zero status codes such as -EPROTO
fall through the default branch with schedule set to false:

drivers/hid/hid-nintendo-switch2-usb.c:switch2_bulk_in() {
	bool schedule = false;
	unsigned long flags;

	switch (urb->status) {
	case 0:
		schedule = true;
		break;
	...
	default:
		dev_dbg(&ns2_usb->udev->dev, "unknown input urb status: %d\n", urb->status);
		break;
	}

Without checking schedule here, a failed URB hitting the default branch is
marked NS2_URB_IN and queued for switch2_usb_message_in_work(), passing
stale transfer_buffer data to switch2_receive_command() instead of marking
the URB NS2_URB_FREE and resubmitting it.  Removing all reads of schedule
also triggers a -Wunused-but-set-variable compiler warning.

[ ... ]

> @@ -294,6 +289,10 @@ static void switch2_usb_message_in_work(struct work_struct *work)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
> +	if (ns2_usb->shutdown) {
> +		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
> +		return;
> +	}
>  	for (i = 0; i < NS2_IN_URBS; i++) {
>  		urb = &ns2_usb->bulk_in[i];
>  		if (urb->state != NS2_URB_IN)

[Severity: High]
This problem wasn't introduced by this patch, but can
switch2_usb_message_in_work() and switch2_usb_shutdown() hit an ABBA
deadlock [1] when an inbound USB packet arrives during controller
disconnect?

CPU0                               kworker
----                               -------
mutex_lock(&ns2->lock)
                                   switch2_usb_message_in_work()
                                     spin_unlock_irqrestore(...)
                                     switch2_receive_command()
switch2_usb_shutdown()
  cancel_work_sync(...)
                                       mutex_lock(&ns2->lock)

 *** DEADLOCK ***

CPU0: switch2_controller_detach_cfg() -> switch2_usb_shutdown()
kworker: switch2_usb_message_in_work() -> switch2_receive_command()

switch2_controller_detach_cfg() (and the error path of
switch2_controller_attach_cfg()) calls cfg->shutdown() while holding
ns2->lock.  If switch2_usb_message_in_work() has already passed the
ns2_usb->shutdown check and dropped bulk_in_lock, it blocks on ns2->lock
in switch2_receive_command() while cancel_work_sync() waits for the work
to finish.

[1] https://sashiko.dev/#/bug/linux-6ed5071c-4448-42b6-82a2-03203abfe1da

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110043420.2433197-1-vi@endrift.com?part=1