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

Message ID bd0ae98e5ae821fdebec6821a8eb73291cee3319.1473405255.git.baolin.wang@linaro.org
State New
Headers show

Commit Message

Baolin Wang Sept. 9, 2016, 7:16 a.m.
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

Comments

Baolin Wang Sept. 18, 2016, 4:48 a.m. | #1
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

Patch hide | download patch | download mbox

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)) {