* [PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism
@ 2020-03-13 2:18 Thinh Nguyen
2020-03-13 2:18 ` [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer Thinh Nguyen
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-13 2:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Thinh Nguyen, linux-usb; +Cc: John Youn
Currently we use the "retry" method to issue START_TRANSFER command multiple
times, each with a next interval. There's been report that 5 number of retries
may not be enough. See https://lkml.org/lkml/2019/11/11/535
We could increase the number of retries. However, that also may not be enough
depending on different system latency. It is not often that the latency is
higher than 5 isoc intervals. If it goes beyond 5 intervals, let's restart the
whole process again.
Thinh Nguyen (3):
usb: dwc3: gadget: Properly handle failed kick_transfer
ute: dwc3: gadget: Store resource index of start cmd
usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer
drivers/usb/dwc3/gadget.c | 69 ++++++++++++++++++++++++++++++++++++++---------
1 file changed, 57 insertions(+), 12 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-13 2:18 [PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism Thinh Nguyen
@ 2020-03-13 2:18 ` Thinh Nguyen
2020-03-13 14:20 ` Felipe Balbi
2020-03-13 2:18 ` [PATCH 2/3] ute: dwc3: gadget: Store resource index of start cmd Thinh Nguyen
2020-03-13 2:18 ` [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer Thinh Nguyen
2 siblings, 1 reply; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-13 2:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Thinh Nguyen, linux-usb; +Cc: John Youn
If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
should properly end an active transfer and give back all the started
requests. However if it's for an isoc endpoint, the failure maybe due to
bus-expiry status. In this case, don't give back the requests and wait
for the next retry.
Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
drivers/usb/dwc3/gadget.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1b7d2f9cb673..4cb7f9329657 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1213,6 +1213,8 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
}
}
+static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep);
+
static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
{
struct dwc3_gadget_ep_cmd_params params;
@@ -1252,14 +1254,20 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
if (ret < 0) {
- /*
- * FIXME we need to iterate over the list of requests
- * here and stop, unmap, free and del each of the linked
- * requests instead of what we do now.
- */
- if (req->trb)
- memset(req->trb, 0, sizeof(struct dwc3_trb));
- dwc3_gadget_del_and_unmap_request(dep, req, ret);
+ struct dwc3_request *tmp;
+
+ if (ret == -EAGAIN)
+ return ret;
+
+ dwc3_stop_active_transfer(dep, true, true);
+
+ list_for_each_entry_safe(req, tmp, &dep->started_list, list)
+ dwc3_gadget_move_cancelled_request(req);
+
+ /* If ep isn't started, then there's no end transfer pending */
+ if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+ dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+
return ret;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] ute: dwc3: gadget: Store resource index of start cmd
2020-03-13 2:18 [PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism Thinh Nguyen
2020-03-13 2:18 ` [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer Thinh Nguyen
@ 2020-03-13 2:18 ` Thinh Nguyen
2020-03-13 2:18 ` [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer Thinh Nguyen
2 siblings, 0 replies; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-13 2:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Thinh Nguyen, linux-usb; +Cc: John Youn
As long as the START_TRANSFER command completes, it provides the
resource index of the endpoint. Use this when we need to issue
END_TRANSFER command to an isoc endpoint to retry with a new
XferNotReady event.
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
drivers/usb/dwc3/gadget.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4cb7f9329657..f1aae4615cf1 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -387,9 +387,12 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
trace_dwc3_gadget_ep_cmd(dep, cmd, params, cmd_status);
- if (ret == 0 && DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
- dep->flags |= DWC3_EP_TRANSFER_STARTED;
- dwc3_gadget_ep_get_transfer_index(dep);
+ if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
+ if (ret == 0)
+ dep->flags |= DWC3_EP_TRANSFER_STARTED;
+
+ if (ret != -ETIMEDOUT)
+ dwc3_gadget_ep_get_transfer_index(dep);
}
if (saved_config) {
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer
2020-03-13 2:18 [PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism Thinh Nguyen
2020-03-13 2:18 ` [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer Thinh Nguyen
2020-03-13 2:18 ` [PATCH 2/3] ute: dwc3: gadget: Store resource index of start cmd Thinh Nguyen
@ 2020-03-13 2:18 ` Thinh Nguyen
2020-03-13 14:38 ` Felipe Balbi
2 siblings, 1 reply; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-13 2:18 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Thinh Nguyen, linux-usb; +Cc: John Youn
After a number of unsuccessful start isoc attempts due to bus-expiry
status, issue END_TRANSFER command and retry on the next XferNotReady
event.
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f1aae4615cf1..a5ad02987536 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1406,7 +1406,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
int ret;
int i;
- if (list_empty(&dep->pending_list)) {
+ if (list_empty(&dep->pending_list) &&
+ list_empty(&dep->started_list)) {
dep->flags |= DWC3_EP_PENDING_REQUEST;
return -EAGAIN;
}
@@ -1429,6 +1430,27 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
break;
}
+ /*
+ * After a number of unsuccessful start attempts due to bus-expiry
+ * status, issue END_TRANSFER command and retry on the next XferNotReady
+ * event.
+ */
+ if (ret == -EAGAIN) {
+ struct dwc3_gadget_ep_cmd_params params;
+ u32 cmd;
+
+ cmd = DWC3_DEPCMD_ENDTRANSFER |
+ DWC3_DEPCMD_CMDIOC |
+ DWC3_DEPCMD_PARAM(dep->resource_index);
+
+ dep->resource_index = 0;
+ memset(¶ms, 0, sizeof(params));
+
+ ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
+ if (!ret)
+ dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+ }
+
return ret;
}
@@ -2609,6 +2631,18 @@ static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep,
const struct dwc3_event_depevt *event)
{
dwc3_gadget_endpoint_frame_from_event(dep, event);
+
+ /*
+ * The XferNotReady event is generated only once before the endpoint
+ * starts. It will be generated again when END_TRANSFER command is
+ * issued. For some controller versions, the XferNotReady event may be
+ * generated while the END_TRANSFER command is still in process. Ignore
+ * it and wait for the next XferNotReady event after the command is
+ * completed.
+ */
+ if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
+ return;
+
(void) __dwc3_gadget_start_isoc(dep);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-13 2:18 ` [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer Thinh Nguyen
@ 2020-03-13 14:20 ` Felipe Balbi
2020-03-13 19:50 ` Thinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2020-03-13 14:20 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Thinh Nguyen, linux-usb; +Cc: John Youn
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
Hi Thinh,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
> should properly end an active transfer and give back all the started
> requests. However if it's for an isoc endpoint, the failure maybe due to
> bus-expiry status. In this case, don't give back the requests and wait
> for the next retry.
>
> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
could you give some details regarding when does this happen?
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer
2020-03-13 2:18 ` [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer Thinh Nguyen
@ 2020-03-13 14:38 ` Felipe Balbi
2020-03-13 20:01 ` Thinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2020-03-13 14:38 UTC (permalink / raw)
To: Thinh Nguyen, Greg Kroah-Hartman, Thinh Nguyen, linux-usb; +Cc: John Youn
[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> After a number of unsuccessful start isoc attempts due to bus-expiry
> status, issue END_TRANSFER command and retry on the next XferNotReady
> event.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
> drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f1aae4615cf1..a5ad02987536 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1406,7 +1406,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> int ret;
> int i;
>
> - if (list_empty(&dep->pending_list)) {
> + if (list_empty(&dep->pending_list) &&
> + list_empty(&dep->started_list)) {
> dep->flags |= DWC3_EP_PENDING_REQUEST;
> return -EAGAIN;
> }
> @@ -1429,6 +1430,27 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> break;
> }
>
> + /*
> + * After a number of unsuccessful start attempts due to bus-expiry
> + * status, issue END_TRANSFER command and retry on the next XferNotReady
> + * event.
> + */
> + if (ret == -EAGAIN) {
> + struct dwc3_gadget_ep_cmd_params params;
> + u32 cmd;
> +
> + cmd = DWC3_DEPCMD_ENDTRANSFER |
> + DWC3_DEPCMD_CMDIOC |
> + DWC3_DEPCMD_PARAM(dep->resource_index);
> +
> + dep->resource_index = 0;
> + memset(¶ms, 0, sizeof(params));
> +
> + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
> + if (!ret)
> + dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> + }
I like this! Pretty good idea :-) I'll wait for your reply to my
question on the other patch, then start queueing again.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-13 14:20 ` Felipe Balbi
@ 2020-03-13 19:50 ` Thinh Nguyen
2020-03-15 8:48 ` Felipe Balbi
0 siblings, 1 reply; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-13 19:50 UTC (permalink / raw)
To: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
Hi Felipe,
Felipe Balbi wrote:
> Hi Thinh,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
>> should properly end an active transfer and give back all the started
>> requests. However if it's for an isoc endpoint, the failure maybe due to
>> bus-expiry status. In this case, don't give back the requests and wait
>> for the next retry.
>>
>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> could you give some details regarding when does this happen?
>
So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return
a negative errno:
* -EAGAIN: Isoc bus-expiry status
As you already know, this occurs when we try to schedule isoc too
late. If we're going to retry the request, don't unmap it.
* -EINVAL: No resource due to issuing START_TRANSFER to an already
started endpoint
This happens generally because of SW programming error
* -ETIMEDOUT: Polling for CMDACT timed out
This should not happen unless the controller is dead or in some bad
state
BR,
Thinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer
2020-03-13 14:38 ` Felipe Balbi
@ 2020-03-13 20:01 ` Thinh Nguyen
0 siblings, 0 replies; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-13 20:01 UTC (permalink / raw)
To: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
Hi Felipe,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>
>> After a number of unsuccessful start isoc attempts due to bus-expiry
>> status, issue END_TRANSFER command and retry on the next XferNotReady
>> event.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>> drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index f1aae4615cf1..a5ad02987536 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1406,7 +1406,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> int ret;
>> int i;
>>
>> - if (list_empty(&dep->pending_list)) {
>> + if (list_empty(&dep->pending_list) &&
>> + list_empty(&dep->started_list)) {
>> dep->flags |= DWC3_EP_PENDING_REQUEST;
>> return -EAGAIN;
>> }
>> @@ -1429,6 +1430,27 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>> break;
>> }
>>
>> + /*
>> + * After a number of unsuccessful start attempts due to bus-expiry
>> + * status, issue END_TRANSFER command and retry on the next XferNotReady
>> + * event.
>> + */
>> + if (ret == -EAGAIN) {
>> + struct dwc3_gadget_ep_cmd_params params;
>> + u32 cmd;
>> +
>> + cmd = DWC3_DEPCMD_ENDTRANSFER |
>> + DWC3_DEPCMD_CMDIOC |
>> + DWC3_DEPCMD_PARAM(dep->resource_index);
>> +
>> + dep->resource_index = 0;
>> + memset(¶ms, 0, sizeof(params));
>> +
>> + ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms);
>> + if (!ret)
>> + dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>> + }
> I like this! Pretty good idea :-) I'll wait for your reply to my
> question on the other patch, then start queueing again.
>
Great! I mentioned this awhile a go, but I didn't get to work on it
until now. See https://marc.info/?l=linux-usb&m=156088170723824&w=4
Glad you're back, can you also take a look and queue the other fixes
that I sent too. There are quite a few patches with these fix dependency
in my queue that I want to push out.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-13 19:50 ` Thinh Nguyen
@ 2020-03-15 8:48 ` Felipe Balbi
2020-03-16 0:33 ` Thinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2020-03-15 8:48 UTC (permalink / raw)
To: Thinh Nguyen, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
>>> should properly end an active transfer and give back all the started
>>> requests. However if it's for an isoc endpoint, the failure maybe due to
>>> bus-expiry status. In this case, don't give back the requests and wait
>>> for the next retry.
>>>
>>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> could you give some details regarding when does this happen?
>>
>
> So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return
> a negative errno:
>
> * -EAGAIN: Isoc bus-expiry status
> As you already know, this occurs when we try to schedule isoc too
> late. If we're going to retry the request, don't unmap it.
right
> * -EINVAL: No resource due to issuing START_TRANSFER to an already
> started endpoint
> This happens generally because of SW programming error
Sounds like this should be fixed separately and, probably, we should add
a WARN() so we catch these situations. Have you reproduced this
particular case?
> * -ETIMEDOUT: Polling for CMDACT timed out
> This should not happen unless the controller is dead or in some bad
> state
Understood
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-15 8:48 ` Felipe Balbi
@ 2020-03-16 0:33 ` Thinh Nguyen
2020-03-16 7:03 ` Felipe Balbi
0 siblings, 1 reply; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-16 0:33 UTC (permalink / raw)
To: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
>>>> should properly end an active transfer and give back all the started
>>>> requests. However if it's for an isoc endpoint, the failure maybe due to
>>>> bus-expiry status. In this case, don't give back the requests and wait
>>>> for the next retry.
>>>>
>>>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>> could you give some details regarding when does this happen?
>>>
>> So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return
>> a negative errno:
>>
>> * -EAGAIN: Isoc bus-expiry status
>> As you already know, this occurs when we try to schedule isoc too
>> late. If we're going to retry the request, don't unmap it.
> right
>
>> * -EINVAL: No resource due to issuing START_TRANSFER to an already
>> started endpoint
>> This happens generally because of SW programming error
> Sounds like this should be fixed separately and, probably, we should add
> a WARN() so we catch these situations. Have you reproduced this
> particular case?
There are a couple of examples of no resource issue that I recall:
1) When we removed the DWC3_EP_END_TRANSFER_PENDING flag, we wasn't able
to check if the endpoint had ended. So, if the function driver queues a
new request while END_TRANSFER command is in progress, it'd trigger
START_TRANSFER to an already started endpoint
2) For this new method of retrying for isoc, when we issue END_TRANSFER
command, for some controller versions, the controller would generate
XferNotReady event while the END_TRANSFER command is in progress plus
another after it completed. That means we'd start on the same endpoint
twice and trigger a no-resource error. (We'd run into this scenario
because END_TRANSFER completion cleared the started flag).
We added the checks to prevent this issue from happening, so this
scenario should not happen again.
If we want to add a WARN(), I think we should add that inside of
dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also
just look at the tracepoint for "no resource" status.
BR,
Thinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-16 0:33 ` Thinh Nguyen
@ 2020-03-16 7:03 ` Felipe Balbi
2020-03-16 19:06 ` Thinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2020-03-16 7:03 UTC (permalink / raw)
To: Thinh Nguyen, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
>>>>> should properly end an active transfer and give back all the started
>>>>> requests. However if it's for an isoc endpoint, the failure maybe due to
>>>>> bus-expiry status. In this case, don't give back the requests and wait
>>>>> for the next retry.
>>>>>
>>>>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> could you give some details regarding when does this happen?
>>>>
>>> So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return
>>> a negative errno:
>>>
>>> * -EAGAIN: Isoc bus-expiry status
>>> As you already know, this occurs when we try to schedule isoc too
>>> late. If we're going to retry the request, don't unmap it.
>> right
>>
>>> * -EINVAL: No resource due to issuing START_TRANSFER to an already
>>> started endpoint
>>> This happens generally because of SW programming error
>> Sounds like this should be fixed separately and, probably, we should add
>> a WARN() so we catch these situations. Have you reproduced this
>> particular case?
>
> There are a couple of examples of no resource issue that I recall:
> 1) When we removed the DWC3_EP_END_TRANSFER_PENDING flag, we wasn't able
> to check if the endpoint had ended. So, if the function driver queues a
> new request while END_TRANSFER command is in progress, it'd trigger
> START_TRANSFER to an already started endpoint
Okay, sounds like this deserves a patch of its own
> 2) For this new method of retrying for isoc, when we issue END_TRANSFER
> command, for some controller versions, the controller would generate
> XferNotReady event while the END_TRANSFER command is in progress plus
> another after it completed. That means we'd start on the same endpoint
> twice and trigger a no-resource error. (We'd run into this scenario
> because END_TRANSFER completion cleared the started flag).
>
> We added the checks to prevent this issue from happening, so this
> scenario should not happen again.
Cool, thanks
> If we want to add a WARN(), I think we should add that inside of
> dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also
> just look at the tracepoint for "no resource" status.
The "no resource" status is important, sure. But users don't usually run
with tracepoints enabled. They'll have a non-working USB port and forget
about it. If there's a WARN() triggered, we are more likely to get bug
reports.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-16 7:03 ` Felipe Balbi
@ 2020-03-16 19:06 ` Thinh Nguyen
2020-03-29 7:52 ` Felipe Balbi
0 siblings, 1 reply; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-16 19:06 UTC (permalink / raw)
To: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
Hi Felipe,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen<Thinh.Nguyen@synopsys.com> writes:
>>> Thinh Nguyen<Thinh.Nguyen@synopsys.com> writes:
>>>>> Thinh Nguyen<Thinh.Nguyen@synopsys.com> writes:
>>>>>> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we
>>>>>> should properly end an active transfer and give back all the started
>>>>>> requests. However if it's for an isoc endpoint, the failure maybe due to
>>>>>> bus-expiry status. In this case, don't give back the requests and wait
>>>>>> for the next retry.
>>>>>>
>>>>>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
>>>>>> Signed-off-by: Thinh Nguyen<thinhn@synopsys.com>
>>>>> could you give some details regarding when does this happen?
>>>>>
>>>> So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return
>>>> a negative errno:
>>>>
>>>> * -EAGAIN: Isoc bus-expiry status
>>>> As you already know, this occurs when we try to schedule isoc too
>>>> late. If we're going to retry the request, don't unmap it.
>>> right
>>>
>>>> * -EINVAL: No resource due to issuing START_TRANSFER to an already
>>>> started endpoint
>>>> This happens generally because of SW programming error
>>> Sounds like this should be fixed separately and, probably, we should add
>>> a WARN() so we catch these situations. Have you reproduced this
>>> particular case?
>> There are a couple of examples of no resource issue that I recall:
>> 1) When we removed the DWC3_EP_END_TRANSFER_PENDING flag, we wasn't able
>> to check if the endpoint had ended. So, if the function driver queues a
>> new request while END_TRANSFER command is in progress, it'd trigger
>> START_TRANSFER to an already started endpoint
> Okay, sounds like this deserves a patch of its own
Yes, that's why we resurrected that flag and made the fix (the patch is
upstream). It should not happen again.
>> 2) For this new method of retrying for isoc, when we issue END_TRANSFER
>> command, for some controller versions, the controller would generate
>> XferNotReady event while the END_TRANSFER command is in progress plus
>> another after it completed. That means we'd start on the same endpoint
>> twice and trigger a no-resource error. (We'd run into this scenario
>> because END_TRANSFER completion cleared the started flag).
>>
>> We added the checks to prevent this issue from happening, so this
>> scenario should not happen again.
> Cool, thanks
>
>> If we want to add a WARN(), I think we should add that inside of
>> dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also
>> just look at the tracepoint for "no resource" status.
> The "no resource" status is important, sure. But users don't usually run
> with tracepoints enabled. They'll have a non-working USB port and forget
> about it. If there's a WARN() triggered, we are more likely to get bug
> reports.
>
Understood. We can add a WARN() to dwc3_send_gadget_ep_cmd() in a
separate patch.
Is there any other concern regarding this series? Let me know if I need
to resend it.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-16 19:06 ` Thinh Nguyen
@ 2020-03-29 7:52 ` Felipe Balbi
2020-03-29 23:14 ` Thinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Felipe Balbi @ 2020-03-29 7:52 UTC (permalink / raw)
To: Thinh Nguyen, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
[-- Attachment #1: Type: text/plain, Size: 713 bytes --]
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> If we want to add a WARN(), I think we should add that inside of
>>> dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also
>>> just look at the tracepoint for "no resource" status.
>> The "no resource" status is important, sure. But users don't usually run
>> with tracepoints enabled. They'll have a non-working USB port and forget
>> about it. If there's a WARN() triggered, we are more likely to get bug
>> reports.
>>
>
> Understood. We can add a WARN() to dwc3_send_gadget_ep_cmd() in a
> separate patch.
I would prefer to see the WARN() patch in the same series, at
least. Care to resend with that?
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer
2020-03-29 7:52 ` Felipe Balbi
@ 2020-03-29 23:14 ` Thinh Nguyen
0 siblings, 0 replies; 14+ messages in thread
From: Thinh Nguyen @ 2020-03-29 23:14 UTC (permalink / raw)
To: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-usb; +Cc: John Youn
Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> If we want to add a WARN(), I think we should add that inside of
>>>> dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also
>>>> just look at the tracepoint for "no resource" status.
>>> The "no resource" status is important, sure. But users don't usually run
>>> with tracepoints enabled. They'll have a non-working USB port and forget
>>> about it. If there's a WARN() triggered, we are more likely to get bug
>>> reports.
>>>
>> Understood. We can add a WARN() to dwc3_send_gadget_ep_cmd() in a
>> separate patch.
> I would prefer to see the WARN() patch in the same series, at
> least. Care to resend with that?
>
Sure. I'll do that.
BR,
Thinh
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-03-29 23:14 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 2:18 [PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism Thinh Nguyen
2020-03-13 2:18 ` [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer Thinh Nguyen
2020-03-13 14:20 ` Felipe Balbi
2020-03-13 19:50 ` Thinh Nguyen
2020-03-15 8:48 ` Felipe Balbi
2020-03-16 0:33 ` Thinh Nguyen
2020-03-16 7:03 ` Felipe Balbi
2020-03-16 19:06 ` Thinh Nguyen
2020-03-29 7:52 ` Felipe Balbi
2020-03-29 23:14 ` Thinh Nguyen
2020-03-13 2:18 ` [PATCH 2/3] ute: dwc3: gadget: Store resource index of start cmd Thinh Nguyen
2020-03-13 2:18 ` [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer Thinh Nguyen
2020-03-13 14:38 ` Felipe Balbi
2020-03-13 20:01 ` Thinh Nguyen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).