Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1479699 > unrolled thread
| Started by | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| First post | 2016-09-09 09:20 +0200 |
| Last post | 2016-09-18 06:50 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-09-09 09:20 +0200
Re: [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Felipe Balbi <balbi@kernel.org> - 2016-09-09 12:50 +0200
Re: [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically Baolin Wang <baolin.wang@linaro.org> - 2016-09-18 06:50 +0200
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-09-09 09:20 +0200 |
| Subject | [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <sfogp-8jT-13@gated-at.bofh.it> |
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(-)
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)) {
--
1.7.9.5
[toc] | [next] | [standalone]
| From | Felipe Balbi <balbi@kernel.org> |
|---|---|
| Date | 2016-09-09 12:50 +0200 |
| Message-ID | <sfrxE-1Je-1@gated-at.bofh.it> |
| In reply to | #1479699 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Baolin Wang <baolin.wang@linaro.org> |
|---|---|
| Date | 2016-09-18 06:50 +0200 |
| Subject | Re: [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically |
| Message-ID | <siCdb-3uq-5@gated-at.bofh.it> |
| In reply to | #1479864 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web