| Message ID | 20230410231954.437-3-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Avoid having pending end transfers on soft disconnect | expand |
On Mon, Apr 10, 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") > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > drivers/usb/dwc3/gadget.c | 42 +++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 320e30476c88..91768f1bdbaf 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2546,29 +2546,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 > @@ -2581,6 +2569,26 @@ 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) { > + dev_warn(dwc->dev, "wait for SETUP phase timed out\n"); > + spin_lock_irqsave(&dwc->lock, flags); > + dwc3_ep0_reset_state(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@synopsys.com> Thanks, Thinh
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
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 320e30476c88..91768f1bdbaf 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2546,29 +2546,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 @@ -2581,6 +2569,26 @@ 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) { + dev_warn(dwc->dev, "wait for SETUP phase timed out\n"); + spin_lock_irqsave(&dwc->lock, flags); + dwc3_ep0_reset_state(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.
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") Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> --- drivers/usb/dwc3/gadget.c | 42 +++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 17 deletions(-)