| Message ID | 2fcf3b5d90068d549589a57a27a79f76c6769b04.1650593829.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f66eef8fb8989a7193cafc3870f7c7b2b97f16cb |
| Headers | show |
| Series | usb: dwc3: gadget: Rework pullup | expand |
Hi Thinh, On 4/21/2022 7:23 PM, Thinh Nguyen wrote: > If the controller hasn't DMA'ed the Setup data from its fifo, it won't > process the End Transfer command. Polling for the command completion may > block the driver from servicing the Setup phase and cause a timeout. > Previously we only check and delay issuing End Transfer in the case of > endpoint dequeue. Let's do that for all End Transfer scenarios. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/usb/dwc3/gadget.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 7c4d5f671687..f0fd7c32828b 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2056,16 +2056,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > if (r == req) { > struct dwc3_request *t; > > - /* > - * If a Setup packet is received but yet to DMA out, the controller will > - * not process the End Transfer command of any endpoint. Polling of its > - * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a > - * timeout. Delay issuing the End Transfer command until the Setup TRB is > - * prepared. > - */ > - if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) > - dep->flags |= DWC3_EP_DELAY_STOP; > - > /* wait until it is processed */ > dwc3_stop_active_transfer(dep, true, true); > > @@ -3657,6 +3647,18 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, > (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) > return; > > + /* > + * If a Setup packet is received but yet to DMA out, the controller will > + * not process the End Transfer command of any endpoint. Polling of its > + * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a > + * timeout. Delay issuing the End Transfer command until the Setup TRB is > + * prepared. > + */ > + if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) { > + dep->flags |= DWC3_EP_DELAY_STOP; > + return; > + } > + Since we could be calling dwc3_stop_active_transfer() as part of the dwc3_gadget_pullup(0) case (due to dwc3_stop_active_transfers()), how do we ensure that all active EPs are stopped before calling run/stop clear? static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) { ... if (dwc->ep0state != EP0_SETUP_PHASE) { int ret; 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"); } ---> We know that ep0state will be in SETUP phase now, but host can send the SETUP data shortly after here. This may cause some endpoints in the below dwc3_stop_active_transfers() call to mark DWC3_EP_DELAY_STOP. (ep0state could advance as we call gadget giveback in between servicing each dep, and lock is released briefly) /* * In the Synopsys DesignWare Cores USB3 Databook Rev. 3.30a * Section 4.1.8 Table 4-7, it states that for a device-initiated * disconnect, the SW needs to ensure that it sends "a DEPENDXFER * command for any active transfers" before clearing the RunStop * bit. */ dwc3_stop_active_transfers(dwc); ---> Do we need to add some synchronization here to make sure that all EPs that had the DWC3_EP_DELAY_STOP had been serviced by the status phase complete handler? Otherwise, we will continue to try to halt the controller, which will fail since there could still be EPs which are active. __dwc3_gadget_stop(dwc); spin_unlock_irqrestore(&dwc->lock, flags); return dwc3_gadget_run_stop(dwc, false, false); } I haven't run into this particular scenario yet, but thought I'd ask to see if there was some flow that I missed. Thanks Wesley Cheng
Hi Wesley, Wesley Cheng wrote: > Hi Thinh, > > On 4/21/2022 7:23 PM, Thinh Nguyen wrote: >> If the controller hasn't DMA'ed the Setup data from its fifo, it won't >> process the End Transfer command. Polling for the command completion may >> block the driver from servicing the Setup phase and cause a timeout. >> Previously we only check and delay issuing End Transfer in the case of >> endpoint dequeue. Let's do that for all End Transfer scenarios. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/usb/dwc3/gadget.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 7c4d5f671687..f0fd7c32828b 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2056,16 +2056,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep >> *ep, >> if (r == req) { >> struct dwc3_request *t; >> - /* >> - * If a Setup packet is received but yet to DMA out, the >> controller will >> - * not process the End Transfer command of any endpoint. >> Polling of its >> - * DEPCMD.CmdAct may block setting up TRB for Setup >> packet, causing a >> - * timeout. Delay issuing the End Transfer command until >> the Setup TRB is >> - * prepared. >> - */ >> - if (dwc->ep0state != EP0_SETUP_PHASE && >> !dwc->delayed_status) >> - dep->flags |= DWC3_EP_DELAY_STOP; >> - >> /* wait until it is processed */ >> dwc3_stop_active_transfer(dep, true, true); >> @@ -3657,6 +3647,18 @@ void dwc3_stop_active_transfer(struct dwc3_ep >> *dep, bool force, >> (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) >> return; >> + /* >> + * If a Setup packet is received but yet to DMA out, the >> controller will >> + * not process the End Transfer command of any endpoint. Polling >> of its >> + * DEPCMD.CmdAct may block setting up TRB for Setup packet, >> causing a >> + * timeout. Delay issuing the End Transfer command until the >> Setup TRB is >> + * prepared. >> + */ >> + if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) { >> + dep->flags |= DWC3_EP_DELAY_STOP; >> + return; >> + } >> + > > Since we could be calling dwc3_stop_active_transfer() as part of the > dwc3_gadget_pullup(0) case (due to dwc3_stop_active_transfers()), how do > we ensure that all active EPs are stopped before calling run/stop clear? It should be fine even the End Transfer completes after the run_stop bit is clear. Clearing the run_stop would stop activity because the host is disconnected. But the controller can still assert interrupt if there are pending events even after the run_stop bit is cleared. > > static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) { > ... > if (dwc->ep0state != EP0_SETUP_PHASE) { > int ret; > > 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"); > } > > ---> We know that ep0state will be in SETUP phase now, but host can send > the SETUP data shortly after here. This may cause some endpoints in the > below dwc3_stop_active_transfers() call to mark DWC3_EP_DELAY_STOP. > (ep0state could advance as we call gadget giveback in between servicing > each dep, and lock is released briefly) No, it shouldn't advance. The patch "[PATCH 4/6] usb: dwc3: ep0: Don't prepare beyond Setup stage" will cause the controller to respond a STALL to any new control transfer and put it back and prepare a new TRB for a new SETUP packet. > > /* > * In the Synopsys DesignWare Cores USB3 Databook Rev. 3.30a > * Section 4.1.8 Table 4-7, it states that for a device-initiated > * disconnect, the SW needs to ensure that it sends "a DEPENDXFER > * command for any active transfers" before clearing the RunStop > * bit. > */ > dwc3_stop_active_transfers(dwc); > > ---> Do we need to add some synchronization here to make sure that all > EPs that had the DWC3_EP_DELAY_STOP had been serviced by the status > phase complete handler? Otherwise, we will continue to try to halt the > controller, which will fail since there could still be EPs which are > active. Because we make sure ep0state won't advance beyond EP0_SETUP_PHASE, we don't have to worry about DWC3_EP_DELAY_STOP. > > __dwc3_gadget_stop(dwc); > spin_unlock_irqrestore(&dwc->lock, flags); > > return dwc3_gadget_run_stop(dwc, false, false); > } > > I haven't run into this particular scenario yet, but thought I'd ask to > see if there was some flow that I missed. > Thanks for testing! BR, Thinh
On Thu, Apr 21, 2022 at 07:23:03PM -0700, Thinh Nguyen wrote: > If the controller hasn't DMA'ed the Setup data from its fifo, it won't > process the End Transfer command. Polling for the command completion may > block the driver from servicing the Setup phase and cause a timeout. > Previously we only check and delay issuing End Transfer in the case of > endpoint dequeue. Let's do that for all End Transfer scenarios. > > Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > --- > drivers/usb/dwc3/gadget.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 7c4d5f671687..f0fd7c32828b 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2056,16 +2056,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > if (r == req) { > struct dwc3_request *t; > > - /* > - * If a Setup packet is received but yet to DMA out, the controller will > - * not process the End Transfer command of any endpoint. Polling of its > - * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a > - * timeout. Delay issuing the End Transfer command until the Setup TRB is > - * prepared. > - */ > - if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) > - dep->flags |= DWC3_EP_DELAY_STOP; > - > /* wait until it is processed */ > dwc3_stop_active_transfer(dep, true, true); > > @@ -3657,6 +3647,18 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, > (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) > return; > > + /* > + * If a Setup packet is received but yet to DMA out, the controller will > + * not process the End Transfer command of any endpoint. Polling of its > + * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a > + * timeout. Delay issuing the End Transfer command until the Setup TRB is > + * prepared. > + */ > + if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) { > + dep->flags |= DWC3_EP_DELAY_STOP; > + return; > + } > + dwc3_remove_requests() calls dwc3_stop_active_transfer() but does not check any flags before retiring all the requests. should we add some assert/WARN_ON to make sure that we are not retiring the requests before stopping the active transfers? Thanks, Pavan
Pavan Kondeti wrote: > On Thu, Apr 21, 2022 at 07:23:03PM -0700, Thinh Nguyen wrote: >> If the controller hasn't DMA'ed the Setup data from its fifo, it won't >> process the End Transfer command. Polling for the command completion may >> block the driver from servicing the Setup phase and cause a timeout. >> Previously we only check and delay issuing End Transfer in the case of >> endpoint dequeue. Let's do that for all End Transfer scenarios. >> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> --- >> drivers/usb/dwc3/gadget.c | 22 ++++++++++++---------- >> 1 file changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 7c4d5f671687..f0fd7c32828b 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2056,16 +2056,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, >> if (r == req) { >> struct dwc3_request *t; >> >> - /* >> - * If a Setup packet is received but yet to DMA out, the controller will >> - * not process the End Transfer command of any endpoint. Polling of its >> - * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a >> - * timeout. Delay issuing the End Transfer command until the Setup TRB is >> - * prepared. >> - */ >> - if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) >> - dep->flags |= DWC3_EP_DELAY_STOP; >> - >> /* wait until it is processed */ >> dwc3_stop_active_transfer(dep, true, true); >> >> @@ -3657,6 +3647,18 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, >> (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) >> return; >> >> + /* >> + * If a Setup packet is received but yet to DMA out, the controller will >> + * not process the End Transfer command of any endpoint. Polling of its >> + * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a >> + * timeout. Delay issuing the End Transfer command until the Setup TRB is >> + * prepared. >> + */ >> + if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) { >> + dep->flags |= DWC3_EP_DELAY_STOP; >> + return; >> + } >> + > > dwc3_remove_requests() calls dwc3_stop_active_transfer() but does not check > any flags before retiring all the requests. should we add some assert/WARN_ON > to make sure that we are not retiring the requests before stopping the active > transfers? > Why? Do you see a problem with it? Ideally we should wait for the controller to release the TRBs before returning the requests to the upperlayer, and only force retire the requests on timeout. However, in the case of dwc3_remove_requests(), it should only be called when there's a disconnect or de-initialization of the controller. The dwc3 driver reports -ESHUTDOWN error in the requests to the function driver. The function driver shouldn't be using/reusing the request buffer as it should be doing driver tear down/cleanup at this point. I haven't seen a problem handling it this way yet. From what I recall, previously, the reason we had it this way is because the events generated from End Transfer completion prevents the controller from halting. However, with this rework of pullup(), it should be able to handle that case now. So we can create a patch to make sure that we handle all End Transfer completion cases. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 7c4d5f671687..f0fd7c32828b 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2056,16 +2056,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, if (r == req) { struct dwc3_request *t; - /* - * If a Setup packet is received but yet to DMA out, the controller will - * not process the End Transfer command of any endpoint. Polling of its - * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a - * timeout. Delay issuing the End Transfer command until the Setup TRB is - * prepared. - */ - if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) - dep->flags |= DWC3_EP_DELAY_STOP; - /* wait until it is processed */ dwc3_stop_active_transfer(dep, true, true); @@ -3657,6 +3647,18 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, (dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; + /* + * If a Setup packet is received but yet to DMA out, the controller will + * not process the End Transfer command of any endpoint. Polling of its + * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a + * timeout. Delay issuing the End Transfer command until the Setup TRB is + * prepared. + */ + if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) { + dep->flags |= DWC3_EP_DELAY_STOP; + return; + } + /* * NOTICE: We are violating what the Databook says about the * EndTransfer command. Ideally we would _always_ wait for the
If the controller hasn't DMA'ed the Setup data from its fifo, it won't process the End Transfer command. Polling for the command completion may block the driver from servicing the Setup phase and cause a timeout. Previously we only check and delay issuing End Transfer in the case of endpoint dequeue. Let's do that for all End Transfer scenarios. Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- drivers/usb/dwc3/gadget.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)