Re: [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

[Felipe Balbi <balbi@kernel.org>]

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


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?

> ---
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]