Hi,
Baolin Wang <baolin.wang@xxxxxxxxxx> 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@xxxxxxxxxx>
do you mind if we discuss this for a little longer?
> ---
> 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().
> 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.
> /*
> * 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
