| Message ID | bd0ae98e5ae821fdebec6821a8eb73291cee3319.1473405255.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Felipe, Sorry for late reply due to my holiday. On 9 September 2016 at 18:47, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Baolin Wang <baolin.wang@linaro.org> writes: >> When system has stpped the gadget, we should avoid queuing any requests >> which will cause tranfer failed. Thus adding some disconnect checking to >> avoid this situation. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > do you mind if we discuss this for a little longer? Sure. > >> --- >> Changes since v1: >> - Split into 2 separate ptaches. >> - Choose complete mechanism instead of polling. >> --- >> drivers/usb/dwc3/ep0.c | 8 ++++++++ >> drivers/usb/dwc3/gadget.c | 12 +++++++++--- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >> index fe79d77..632e5a4 100644 >> --- a/drivers/usb/dwc3/ep0.c >> +++ b/drivers/usb/dwc3/ep0.c >> @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request, >> int ret; >> >> spin_lock_irqsave(&dwc->lock, flags); >> + if (!dwc->pullups_connected) { >> + dwc3_trace(trace_dwc3_ep0, >> + "queuing request %p to %s when gadget is disconnected", >> + request, dep->name); >> + ret = -ESHUTDOWN; >> + goto out; >> + } > > I have been thinking about this branch here. It's not a problem to queue > a request with pullups disconnected. It's only a problem to issue > START_TRANSFER without RUN_STOP bit set. > > So maybe this check should be done in dwc3_send_gadget_ep_cmd(). By > doing that we also make sure to do the check in one place and one place > only because all endpoints rely dwc3_send_gadget_ep_cmd(). OK, that makes sense to me. > >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 1783406..1a33308 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) >> struct dwc3 *dwc = dep->dwc; >> int ret; >> >> + if (!dwc->pullups_connected) { >> + dwc3_trace(trace_dwc3_gadget, >> + "queuing request %p to %s when gadget is disconnected", >> + &req->request, dep->endpoint.name); >> + return -ESHUTDOWN; >> + } >> + >> if (!dep->endpoint.desc) { >> dwc3_trace(trace_dwc3_gadget, >> "trying to queue request %p to disabled %s", >> @@ -1984,13 +1991,12 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, >> if (ret) >> break; >> } >> - > > trailing change. Ah, sorry. Will remove it. > >> /* >> * Our endpoint might get disabled by another thread during >> * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 >> * early on so DWC3_EP_BUSY flag gets cleared >> */ >> - if (!dep->endpoint.desc) >> + if (!dep->endpoint.desc || !dwc->pullups_connected) > > I'm still considering this as well. Sure, we kill pullups before the > descriptor is set to NULL, but that shouldn't be a problem. What will > happen is: > > usb_gadget_disconnect(); > udc->driver->disconnect(); > for_each_ep() { > for_each_request() { > usb_ep_dequeue(); > } > usb_ep_disable(); > dep->endpoint.desc = NULL; > } > udc->driver->unbind(); > usb_gadget_udc_stop(); > > I don't see a problem here. Did you manage to trigger any failure when > you didn't have this check? Care to show some logs? We might have a bug > elsewhere which we don't want to mask by adding this check here. If we did not add this 'pullups_connected' checking here, it maybe will kick one transfer to cause a problem of TART_TRANSFER, but it also can be removed if we check in dwc3_send_gadget_ep_cmd() as you suggested. Thanks for your comments and I will send out the new patch. -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c index fe79d77..632e5a4 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request, int ret; spin_lock_irqsave(&dwc->lock, flags); + if (!dwc->pullups_connected) { + dwc3_trace(trace_dwc3_ep0, + "queuing request %p to %s when gadget is disconnected", + request, dep->name); + ret = -ESHUTDOWN; + goto out; + } + if (!dep->endpoint.desc) { dwc3_trace(trace_dwc3_ep0, "trying to queue request %p to disabled %s", diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 1783406..1a33308 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) struct dwc3 *dwc = dep->dwc; int ret; + if (!dwc->pullups_connected) { + dwc3_trace(trace_dwc3_gadget, + "queuing request %p to %s when gadget is disconnected", + &req->request, dep->endpoint.name); + return -ESHUTDOWN; + } + if (!dep->endpoint.desc) { dwc3_trace(trace_dwc3_gadget, "trying to queue request %p to disabled %s", @@ -1984,13 +1991,12 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep, if (ret) break; } - /* * Our endpoint might get disabled by another thread during * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 * early on so DWC3_EP_BUSY flag gets cleared */ - if (!dep->endpoint.desc) + if (!dep->endpoint.desc || !dwc->pullups_connected) return 1; if (usb_endpoint_xfer_isoc(dep->endpoint.desc) && @@ -2064,7 +2070,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc, * dwc3_gadget_giveback(). If that happens, we're just gonna return 1 * early on so DWC3_EP_BUSY flag gets cleared */ - if (!dep->endpoint.desc) + if (!dep->endpoint.desc || !dwc->pullups_connected) return; if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
When system has stpped the gadget, we should avoid queuing any requests which will cause tranfer failed. Thus adding some disconnect checking to avoid this situation. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes since v1: - Split into 2 separate ptaches. - Choose complete mechanism instead of polling. --- drivers/usb/dwc3/ep0.c | 8 ++++++++ drivers/usb/dwc3/gadget.c | 12 +++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html