| Message ID | 4f9b6acbff2cd0be417fd4a943c1975bf41f8fda.1583443184.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: gadget: Misc transfer cancellation fixes | expand |
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) > else > dep->flags |= DWC3_EP_STALL; > } else { > + /* > + * Don't issue CLEAR_STALL command to control endpoints. The > + * controller automatically clears the STALL when it receives > + * the SETUP token. > + */ we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was this triggered?
Hi, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) >> else >> dep->flags |= DWC3_EP_STALL; >> } else { >> + /* >> + * Don't issue CLEAR_STALL command to control endpoints. The >> + * controller automatically clears the STALL when it receives >> + * the SETUP token. >> + */ > we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was > this triggered? > I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() has the similar name as __dwc3_gadget_ep_set_halt(). However, that function only calls dwc3_ep0_stall_and_restart() and not handled through SET/CLEAR_FEATURE(halt) request. If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control endpoints, it still goes through __dwc3_gadget_ep_set_halt(). BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > Felipe Balbi wrote: >> Hi, >> >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) >>> else >>> dep->flags |= DWC3_EP_STALL; >>> } else { >>> + /* >>> + * Don't issue CLEAR_STALL command to control endpoints. The >>> + * controller automatically clears the STALL when it receives >>> + * the SETUP token. >>> + */ >> we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was >> this triggered? >> > > I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() > has the similar name as __dwc3_gadget_ep_set_halt(). However, that > function only calls dwc3_ep0_stall_and_restart() and not handled through > SET/CLEAR_FEATURE(halt) request. > > If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control > endpoints, it still goes through __dwc3_gadget_ep_set_halt(). Perhaps that should be fixed, then?
Hi, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> Felipe Balbi wrote: >>> Hi, >>> >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) >>>> else >>>> dep->flags |= DWC3_EP_STALL; >>>> } else { >>>> + /* >>>> + * Don't issue CLEAR_STALL command to control endpoints. The >>>> + * controller automatically clears the STALL when it receives >>>> + * the SETUP token. >>>> + */ >>> we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was >>> this triggered? >>> >> I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() >> has the similar name as __dwc3_gadget_ep_set_halt(). However, that >> function only calls dwc3_ep0_stall_and_restart() and not handled through >> SET/CLEAR_FEATURE(halt) request. >> >> If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control >> endpoints, it still goes through __dwc3_gadget_ep_set_halt(). > Perhaps that should be fixed, then? > If we want to add a patch to make this clear, we can add a separate patch to rename dwc3_gadget_ep0_set_halt() to something along the line of dwc3_ep0_stall_and_restart(). Everything else looks fine to me. Let me know if you have any other concern. Thanks, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index b87bdec84210..21b10364b888 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1501,6 +1501,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r { int i; + /* If req->trb is not set, then the request has not started */ + if (!req->trb) + return; + /* * If request was already started, this means we had to * stop the transfer. With that we also need to ignore @@ -1591,6 +1595,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) { struct dwc3_gadget_ep_cmd_params params; struct dwc3 *dwc = dep->dwc; + struct dwc3_request *req; + struct dwc3_request *tmp; int ret; if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol) else dep->flags |= DWC3_EP_STALL; } else { + /* + * Don't issue CLEAR_STALL command to control endpoints. The + * controller automatically clears the STALL when it receives + * the SETUP token. + */ + if (dep->number <= 1) { + dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE); + return 0; + } ret = dwc3_send_clear_stall_ep_cmd(dep); - if (ret) + if (ret) { dev_err(dwc->dev, "failed to clear STALL on %s\n", dep->name); - else - dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE); + return ret; + } + + dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE); + + dwc3_stop_active_transfer(dep, true, true); + + list_for_each_entry_safe(req, tmp, &dep->started_list, list) + dwc3_gadget_move_cancelled_request(req); + + list_for_each_entry_safe(req, tmp, &dep->pending_list, list) + dwc3_gadget_move_cancelled_request(req); + + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) { + dep->flags &= ~DWC3_EP_DELAY_START; + dwc3_gadget_ep_cleanup_cancelled_requests(dep); + } } return ret;
DWC3 must not issue CLEAR_STALL command to control endpoints. The controller automatically clears the STALL when it receives the SETUP token. Also, when the driver receives ClearFeature(halt_ep), DWC3 must stop any active transfer from the endpoint and give back all the requests to the function drivers. Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-)