On Thu, Apr 13, 2023, 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")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> ---
> drivers/usb/dwc3/gadget.c | 49 +++++++++++++++++++++++++--------------
> 1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..be84c133f0d7 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2528,29 +2528,17 @@ static int __dwc3_gadget_start(struct dwc3 *dwc);
> static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
> {
> unsigned long flags;
> + int ret;
>
> spin_lock_irqsave(&dwc->lock, flags);
> dwc->connected = false;
>
> /*
> - * Per databook, when we want to stop the gadget, if a control transfer
> - * is still in process, complete it and get the core into setup phase.
> + * Attempt to end pending SETUP status phase, and not wait for the
> + * function to do so.
> */
> - if (dwc->ep0state != EP0_SETUP_PHASE) {
> - int ret;
> -
> - if (dwc->delayed_status)
> - dwc3_ep0_send_delayed_status(dwc);
> -
> - reinit_completion(&dwc->ep0_in_setup);
> -
> - spin_unlock_irqrestore(&dwc->lock, flags);
> - ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> - msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
> - spin_lock_irqsave(&dwc->lock, flags);
> - if (ret == 0)
> - dev_warn(dwc->dev, "timed out waiting for SETUP phase\n");
> - }
> + if (dwc->delayed_status)
> + dwc3_ep0_send_delayed_status(dwc);
>
> /*
> * In the Synopsys DesignWare Cores USB3 Databook Rev. 3.30a
> @@ -2563,6 +2551,33 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
> __dwc3_gadget_stop(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> + /*
> + * Per databook, when we want to stop the gadget, if a control transfer
> + * is still in process, complete it and get the core into setup phase.
> + * In case the host is unresponsive to a SETUP transaction, forcefully
> + * stall the transfer, and move back to the SETUP phase, so that any
> + * pending endxfers can be executed.
> + */
> + if (dwc->ep0state != EP0_SETUP_PHASE) {
> + reinit_completion(&dwc->ep0_in_setup);
> +
> + ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> + msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
> + if (ret == 0) {
> + unsigned int dir;
> +
> + dev_warn(dwc->dev, "wait for SETUP phase timed out\n");
> + spin_lock_irqsave(&dwc->lock, flags);
> + dir = !!dwc->ep0_expect_in;
> + if (dwc->ep0state == EP0_DATA_PHASE)
> + dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
> + else
> + dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
> + dwc3_ep0_stall_and_restart(dwc);
> + spin_unlock_irqrestore(&dwc->lock, flags);
> + }
> + }
> +
> /*
> * Note: if the GEVNTCOUNT indicates events in the event buffer, the
> * driver needs to acknowledge them before the controller can halt.
Acked-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
Thanks,
Thinh
