Re: [PATCH v3 2/3] usb: dwc3: gadget: Stall and restart EP0 if host is unresponsive

[Greg KH]

On Mon, Apr 10, 2023 at 04:19:53PM -0700, Wesley Cheng wrote:
> It was observed that there are hosts that may complete pending SETUP
> transactions before the stop active transfers and controller halt occurs,
> leading to lingering endxfer commands on DEPs on subsequent pullup/gadget
> start iterations.
> 
>   dwc3_gadget_ep_disable   name=ep8in flags=0x3009  direction=1
>   dwc3_gadget_ep_disable   name=ep4in flags=1  direction=1
>   dwc3_gadget_ep_disable   name=ep3out flags=1  direction=0
>   usb_gadget_disconnect   deactivated=0  connected=0  ret=0
> 
> The sequence shows that the USB gadget disconnect (dwc3_gadget_pullup(0))
> routine completed successfully, allowing for the USB gadget to proceed with
> a USB gadget connect.  However, if this occurs the system runs into an
> issue where:
> 
>   BUG: spinlock already unlocked on CPU
>   spin_bug+0x0
>   dwc3_remove_requests+0x278
>   dwc3_ep0_out_start+0xb0
>   __dwc3_gadget_start+0x25c
> 
> This is due to the pending endxfers, leading to gadget start (w/o lock
> held) to execute the remove requests, which will unlock the dwc3
> spinlock as part of giveback.
> 
> To mitigate this, resolve the pending endxfers on the pullup disable
> path by re-locating the SETUP phase check after stop active transfers, since
> that is where the DWC3_EP_DELAY_STOP is potentially set.  This also allows
> for handling of a host that may be unresponsive by using the completion
> timeout to trigger the stall and restart for EP0.
> 
> Fixes: c96683798e27 ("usb: dwc3: ep0: Don't prepare beyond Setup stage")

I'm confused.  You have a Fixes: tag here, yet this patch depends on
patch 1/3, right?  This implies that you do not want or need this to be
backported to any stable kernels, right?

Or do you?  If so, put the bug fixes first, and properly add a cc:
stable tag, so that they will get backported correctly.

If not, then don't even put a fixes tag on it as obviously it isn't a
bugfix that is relevant to track anywhere, and then this is just a
normal new feature to be added to the driver.

Please resolve this and submit a new series based on your decision.

thanks,

greg k-h