Re: [PATCH] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4253 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> 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@linaro.org>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]