Hi,
Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
> When we change the USB function with configfs dynamically, we possibly met this
> situation: one core is doing the control transfer, another core is trying to
> unregister the USB gadget from userspace, we must wait for completing this
> control tranfer, or it will hang the controller to set the DEVCTRLHLT flag.
>
> Also adding some disconnect checking to avoid queuing any requests when the
> gadget is stopped.
>
> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
> ---
> drivers/usb/dwc3/ep0.c | 8 ++++++++
> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++++-----
> 2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index fe79d77..11519d7 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 == false) {
> + dwc3_trace(trace_dwc3_ep0,
> + "queuing request %p to %s when gadget is disconnected",
> + request, dep->name);
> + ret = -ESHUTDOWN;
> + goto out;
> + }
this could go on its own patch, however we can use if
(!dwc->pullups_connected) instead.
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1783406..bbac8f5 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 == false) {
> + dwc3_trace(trace_dwc3_gadget,
> + "queuing request %p to %s when gadget is disconnected",
> + &req->request, dep->endpoint.name);
> + return -ESHUTDOWN;
> + }
ditto
> @@ -1434,6 +1441,13 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> if (pm_runtime_suspended(dwc->dev))
> return 0;
>
> + /*
> + * 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.
> + */
Do you have the section reference for this? Which databook version are
you reading?
> + if (!is_on && dwc->ep0state != EP0_SETUP_PHASE)
> + return -EBUSY;
> +
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> if (is_on) {
> if (dwc->revision <= DWC3_REVISION_187A) {
> @@ -1481,12 +1495,20 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> struct dwc3 *dwc = gadget_to_dwc(g);
> unsigned long flags;
> int ret;
> + int timeout = 500;
>
> is_on = !!is_on;
>
> - spin_lock_irqsave(&dwc->lock, flags);
> - ret = dwc3_gadget_run_stop(dwc, is_on, false);
> - spin_unlock_irqrestore(&dwc->lock, flags);
> + do {
> + spin_lock_irqsave(&dwc->lock, flags);
> + ret = dwc3_gadget_run_stop(dwc, is_on, false);
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + if (ret != -EBUSY)
> + break;
> +
> + udelay(10);
> + } while (--timeout);
no, this is not a good idea at all. I'd rather see:
wait_event_timeout(dwc->wq, dwc->ep0state == EP0_SETUP_PHASE,
jiffies + msecs_to_jiffies(500));
or, perhaps, a wait_for_completion_timeout(). Then, ep0.c needs to call
complete() everytime Status Phase completes. That's probably a better
idea ;-)
> @@ -1990,7 +2012,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
> * 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 == false)
is this really necessary? Can we really get pullups_connect set to false
with dep->endpoint.desc still valid?
> @@ -2064,7 +2086,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 == false)
ditto
--
balbi
Attachment:
signature.asc
Description: PGP signature
