[PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism

[PATCH 0/3] usb: dwc3: gadget: Improve isoc starting mechanism

[Thinh Nguyen]

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

[PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Thinh Nguyen]

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, &params);
 	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

[PATCH 2/3] ute: dwc3: gadget: Store resource index of start cmd

[Thinh Nguyen]

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

[PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer

[Thinh Nguyen]

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(&params, 0, sizeof(params));
+
+		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+		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

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Felipe Balbi]

[-- 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 --]

Re: [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer

[Felipe Balbi]

[-- 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(&params, 0, sizeof(params));
> +
> +		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> +		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 --]

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Thinh Nguyen]

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

Re: [PATCH 3/3] usb: dwc3: gadget: Issue END_TRANSFER to retry isoc transfer

[Thinh Nguyen]

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(&params, 0, sizeof(params));
>> +
>> +		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>> +		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

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Felipe Balbi]

[-- 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 --]

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Thinh Nguyen]

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

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Felipe Balbi]

[-- 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 --]

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Thinh Nguyen]

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

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Felipe Balbi]

[-- 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 --]

Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Thinh Nguyen]

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