[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Hi Felipe,
>
> On 4/9/2018 4:28 AM, Felipe Balbi wrote:
>> In case we get an event with status set to Missed Isoc, this means we
>> have missed an isochronous interval and should issue End Transfer
>> command and wait for the following XferNotReady.
>
> Why does DWC3 need to issue End Transfer if there are still queued requests?

Without XferNotReady, we won't have a reliable way to know the uFrame
number. Read the Isochronous programming sequence from your databook.

>> @@ -2383,14 +2380,25 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>   {
>>   	struct dwc3		*dwc = dep->dwc;
>>   	unsigned		status = 0;
>> +	bool			stop = false;
>>   
>>   	dwc3_gadget_endpoint_frame_from_event(dep, event);
>>   
>>   	if (event->status & DEPEVT_STATUS_BUSERR)
>>   		status = -ECONNRESET;
>>   
>> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>> +		status = -ECONNRESET;
>
> Missed isoc shouldn't cause this error status or if it should return an 
> error status at all. Maybe the status can be -EXDEV, similar to the host 
> side (/Documentation/driver-api/usb/error-codes.rst).

fair enough. I'll change to EXDEV

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Maybe we should return the -EXDEV status every time there's a missed isoc.
>> 
>> you mean like this?
>
> Yes, this will work.

updated to branch

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Thinh Nguyen]

Hi,

On 4/12/2018 12:18 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Hi,
>>
>> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Felipe Balbi <felipe.balbi@linux.intel.com> writes:
>>>>>> Without XferNotReady, we won't have a reliable way to know the uFrame
>>>>>> number. Read the Isochronous programming sequence from your databook.
>>>>>
>>>>> Right. We need XferNotReady to know when to start isoc transfer. But if
>>>>> there are still queued requests, DWC3 can just wait to see if any of
>>>>> them will succeed to continue with the transfer just as how DWC3 is
>>>>> handling it now.
>>>>
>>>> That's not what the databook says though. And that's also not intention
>>>> of how the code is written as of now either. The way the code is written
>>>> is the following:
>>>>
>>>> queue() -> XferNotReady -> start_isoc() -> if (missed) do_nothing() ->
>>>> queue() -> end_transfer.
>>>>
>>>> That's not really waiting for the queue to be consumed, it's just
>>>> delaying end transfer until we get another queue(). IOW, it just
>>>> *happens* to give the controller time to go through the list of started
>>>> requests.
>>>>
>>>>> If we end and restart the transfer right away, then we may lose more
>>>>> isoc data than necessary (due to isoc scheduling at least 4 uFrame
>>>>> ahead of time). Is there something you see that doesn't work with the
>>>>> current implementation?
>>>>
>>>> Not _really_, I'm just trying to make the code easier to read and, I
>>>> think, I've achieved that. Now, if we need to delay end transfer in the
>>>> case where we have more requests in the controller's queue, that's easy
>>>> enough to implement:
>>>>
>>>> @@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>>>    	if (event->status & DEPEVT_STATUS_BUSERR)
>>>>    		status = -ECONNRESET;
>>>>    
>>>> -	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>>>> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
>>>> +			list_empty(&dep->started_list) {
>>>>    		status = -EXDEV;
>>
>> Maybe we should return the -EXDEV status every time there's a missed isoc.
> 
> you mean like this?

Yes, this will work.

> 
> @@ -2358,10 +2358,11 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>   	if (event->status & DEPEVT_STATUS_BUSERR)
>   		status = -ECONNRESET;
>   
> -	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
> -			list_empty(&dep->started_list)) {
> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>   		status = -EXDEV;
> -		stop = true;
> +
> +		if (list_empty(&dep->started_list))
> +			stop = true;
>   	}
>   
>   	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
> 

BR,
Thinh
---
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

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Hi,
>
> On 4/11/2018 1:21 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Felipe Balbi <felipe.balbi@linux.intel.com> writes:
>>>>> Without XferNotReady, we won't have a reliable way to know the uFrame
>>>>> number. Read the Isochronous programming sequence from your databook.
>>>>
>>>> Right. We need XferNotReady to know when to start isoc transfer. But if
>>>> there are still queued requests, DWC3 can just wait to see if any of
>>>> them will succeed to continue with the transfer just as how DWC3 is
>>>> handling it now.
>>>
>>> That's not what the databook says though. And that's also not intention
>>> of how the code is written as of now either. The way the code is written
>>> is the following:
>>>
>>> queue() -> XferNotReady -> start_isoc() -> if (missed) do_nothing() ->
>>> queue() -> end_transfer.
>>>
>>> That's not really waiting for the queue to be consumed, it's just
>>> delaying end transfer until we get another queue(). IOW, it just
>>> *happens* to give the controller time to go through the list of started
>>> requests.
>>>
>>>> If we end and restart the transfer right away, then we may lose more
>>>> isoc data than necessary (due to isoc scheduling at least 4 uFrame
>>>> ahead of time). Is there something you see that doesn't work with the
>>>> current implementation?
>>>
>>> Not _really_, I'm just trying to make the code easier to read and, I
>>> think, I've achieved that. Now, if we need to delay end transfer in the
>>> case where we have more requests in the controller's queue, that's easy
>>> enough to implement:
>>>
>>> @@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>>   	if (event->status & DEPEVT_STATUS_BUSERR)
>>>   		status = -ECONNRESET;
>>>   
>>> -	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>>> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
>>> +			list_empty(&dep->started_list) {
>>>   		status = -EXDEV;
>
> Maybe we should return the -EXDEV status every time there's a missed isoc.

you mean like this?

@@ -2358,10 +2358,11 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_BUSERR)
 		status = -ECONNRESET;
 
-	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
-			list_empty(&dep->started_list)) {
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
 		status = -EXDEV;
-		stop = true;
+
+		if (list_empty(&dep->started_list))
+			stop = true;
 	}
 
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);

>>>   		stop = true;
>>>   	}
>>>
>>> I'm not sure this is a good idea though. Once we miss an interval, don't
>>> we need to know the next frame when transfer needs to be scheduled?
>>>
>>> Meaning we would need XferNotReady to properly schedule the new
>>> transfer?
>> 
>> thinking about this a little more. This extra list_empty() check is not
>> wrong at all :-) I've amended this series with the 3 patches below. I'll
>> resend the series once I've given more time for people to test. Patches
>> have been updated to the branch on kernel.org as well, btw.
>
> Great! :)
> Thanks for all the new updates. I'll test it out when I have a chance.

sure, thanks a lot.

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Thinh Nguyen]

Hi,

On 4/11/2018 1:21 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Balbi <felipe.balbi@linux.intel.com> writes:
>>>> Without XferNotReady, we won't have a reliable way to know the uFrame
>>>> number. Read the Isochronous programming sequence from your databook.
>>>
>>> Right. We need XferNotReady to know when to start isoc transfer. But if
>>> there are still queued requests, DWC3 can just wait to see if any of
>>> them will succeed to continue with the transfer just as how DWC3 is
>>> handling it now.
>>
>> That's not what the databook says though. And that's also not intention
>> of how the code is written as of now either. The way the code is written
>> is the following:
>>
>> queue() -> XferNotReady -> start_isoc() -> if (missed) do_nothing() ->
>> queue() -> end_transfer.
>>
>> That's not really waiting for the queue to be consumed, it's just
>> delaying end transfer until we get another queue(). IOW, it just
>> *happens* to give the controller time to go through the list of started
>> requests.
>>
>>> If we end and restart the transfer right away, then we may lose more
>>> isoc data than necessary (due to isoc scheduling at least 4 uFrame
>>> ahead of time). Is there something you see that doesn't work with the
>>> current implementation?
>>
>> Not _really_, I'm just trying to make the code easier to read and, I
>> think, I've achieved that. Now, if we need to delay end transfer in the
>> case where we have more requests in the controller's queue, that's easy
>> enough to implement:
>>
>> @@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>   	if (event->status & DEPEVT_STATUS_BUSERR)
>>   		status = -ECONNRESET;
>>   
>> -	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
>> +			list_empty(&dep->started_list) {
>>   		status = -EXDEV;

Maybe we should return the -EXDEV status every time there's a missed isoc.

>>   		stop = true;
>>   	}
>>
>> I'm not sure this is a good idea though. Once we miss an interval, don't
>> we need to know the next frame when transfer needs to be scheduled?
>>
>> Meaning we would need XferNotReady to properly schedule the new
>> transfer?
> 
> thinking about this a little more. This extra list_empty() check is not
> wrong at all :-) I've amended this series with the 3 patches below. I'll
> resend the series once I've given more time for people to test. Patches
> have been updated to the branch on kernel.org as well, btw.

Great! :)
Thanks for all the new updates. I'll test it out when I have a chance.

Thinh
---
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

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi]

Hi,

Felipe Balbi <felipe.balbi@linux.intel.com> writes:
>>> Without XferNotReady, we won't have a reliable way to know the uFrame
>>> number. Read the Isochronous programming sequence from your databook.
>>
>> Right. We need XferNotReady to know when to start isoc transfer. But if 
>> there are still queued requests, DWC3 can just wait to see if any of 
>> them will succeed to continue with the transfer just as how DWC3 is 
>> handling it now.
>
> That's not what the databook says though. And that's also not intention
> of how the code is written as of now either. The way the code is written
> is the following:
>
> queue() -> XferNotReady -> start_isoc() -> if (missed) do_nothing() ->
> queue() -> end_transfer.
>
> That's not really waiting for the queue to be consumed, it's just
> delaying end transfer until we get another queue(). IOW, it just
> *happens* to give the controller time to go through the list of started
> requests.
>
>> If we end and restart the transfer right away, then we may lose more
>> isoc data than necessary (due to isoc scheduling at least 4 uFrame
>> ahead of time). Is there something you see that doesn't work with the
>> current implementation?
>
> Not _really_, I'm just trying to make the code easier to read and, I
> think, I've achieved that. Now, if we need to delay end transfer in the
> case where we have more requests in the controller's queue, that's easy
> enough to implement:
>
> @@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_BUSERR)
>  		status = -ECONNRESET;
>  
> -	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
> +			list_empty(&dep->started_list) {
>  		status = -EXDEV;
>  		stop = true;
>  	}
>
> I'm not sure this is a good idea though. Once we miss an interval, don't
> we need to know the next frame when transfer needs to be scheduled?
>
> Meaning we would need XferNotReady to properly schedule the new
> transfer?

thinking about this a little more. This extra list_empty() check is not
wrong at all :-) I've amended this series with the 3 patches below. I'll
resend the series once I've given more time for people to test. Patches
have been updated to the branch on kernel.org as well, btw.



8<------------------------------------------------------------------------

From 811476d1799c606b3af2b40022fe333cef586387 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Wed, 11 Apr 2018 10:31:53 +0300
Subject: [PATCH 1/3] usb: dwc3: debug: decode uFrame from event too

XferNotReady and XferInProgress give us the uFrame number we're
currently in. Printing that out on tracepoints may help us find bugs
in transfer scheduling.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/debug.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 0be6a554be57..c66d216dcc30 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -493,14 +493,18 @@ dwc3_ep_event_string(char *str, const struct dwc3_event_depevt *event,
 	case DWC3_DEPEVT_XFERINPROGRESS:
 		len = strlen(str);
 
-		sprintf(str + len, "Transfer In Progress (%c%c%c)",
+		sprintf(str + len, "Transfer In Progress [%d] (%c%c%c)",
+				event->parameters,
 				status & DEPEVT_STATUS_SHORT ? 'S' : 's',
 				status & DEPEVT_STATUS_IOC ? 'I' : 'i',
 				status & DEPEVT_STATUS_LST ? 'M' : 'm');
 		break;
 	case DWC3_DEPEVT_XFERNOTREADY:
-		strcat(str, "Transfer Not Ready");
-		strcat(str, status & DEPEVT_STATUS_TRANSFER_ACTIVE ?
+		len = strlen(str);
+
+		sprintf(str + len, "Transfer Not Ready [%d]%s",
+				event->parameters,
+				status & DEPEVT_STATUS_TRANSFER_ACTIVE ?
 				" (Active)" : " (Not Active)");
 
 		/* Control Endpoints */
-- 
2.16.1


8<------------------------------------------------------------------------

From 77f9d84d785c2d088e82c90b7d3ad844ce59d668 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Wed, 11 Apr 2018 10:32:52 +0300
Subject: [PATCH 2/3] usb: dwc3: gadget: don't issue End Transfer if we have
 started reqs

In case we have many started requests and one of them in the middle is
completed with Missed Isoc, let's not End Transfer as that would
result in us loosing (possibly) many more intervals.

Instead, let's allow the controller to go through its list of started
requests.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e0618afa14cb..9d4dc8bed644 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_BUSERR)
 		status = -ECONNRESET;
 
-	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
+			list_empty(&dep->started_list)) {
 		status = -EXDEV;
 		stop = true;
 	}
-- 
2.16.1

8<------------------------------------------------------------------------

From 09ded2962aaf0ebbb40e3e7147d5ad4486c04601 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Wed, 11 Apr 2018 10:34:34 +0300
Subject: [PATCH 3/3] usb: dwc3: gadget: always start isochronous aligned to
 dep->interval

We will *always* start transfer to the next uFrame number aligned to
dep->interval.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9d4dc8bed644..9cf89f3cf203 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1233,6 +1233,8 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+#define DWC3_ALIGN_FRAME(d)	(((d)->frame_number + (d)->interval) & ~((d)->interval - 1))
+
 static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 {
 	if (list_empty(&dep->pending_list)) {
@@ -1242,11 +1244,7 @@ static void __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 		return;
 	}
 
-	/*
-	 * Schedule the first trb for one interval in the future or at
-	 * least 4 microframes.
-	 */
-	dep->frame_number += max_t(u32, 4, dep->interval);
+	dep->frame_number = DWC3_ALIGN_FRAME(dep);
 	__dwc3_gadget_kick_transfer(dep);
 }
 
@@ -2352,11 +2350,7 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
 static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep,
 		const struct dwc3_event_depevt *event)
 {
-	u32 cur_uf, mask;
-
-	mask = ~(dep->interval - 1);
-	cur_uf = event->parameters & mask;
-	dep->frame_number = cur_uf;
+	dep->frame_number = event->parameters;
 }
 
 static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> On 4/9/2018 4:28 AM, Felipe Balbi wrote:
>>>> In case we get an event with status set to Missed Isoc, this means we
>>>> have missed an isochronous interval and should issue End Transfer
>>>> command and wait for the following XferNotReady.
>>>
>>> Why does DWC3 need to issue End Transfer if there are still queued requests?
>> 
>> Without XferNotReady, we won't have a reliable way to know the uFrame
>> number. Read the Isochronous programming sequence from your databook.
>
> Right. We need XferNotReady to know when to start isoc transfer. But if 
> there are still queued requests, DWC3 can just wait to see if any of 
> them will succeed to continue with the transfer just as how DWC3 is 
> handling it now.

That's not what the databook says though. And that's also not intention
of how the code is written as of now either. The way the code is written
is the following:

queue() -> XferNotReady -> start_isoc() -> if (missed) do_nothing() ->
queue() -> end_transfer.

That's not really waiting for the queue to be consumed, it's just
delaying end transfer until we get another queue(). IOW, it just
*happens* to give the controller time to go through the list of started
requests.

> If we end and restart the transfer right away, then we may lose more
> isoc data than necessary (due to isoc scheduling at least 4 uFrame
> ahead of time). Is there something you see that doesn't work with the
> current implementation?

Not _really_, I'm just trying to make the code easier to read and, I
think, I've achieved that. Now, if we need to delay end transfer in the
case where we have more requests in the controller's queue, that's easy
enough to implement:

@@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_BUSERR)
 		status = -ECONNRESET;
 
-	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC &&
+			list_empty(&dep->started_list) {
 		status = -EXDEV;
 		stop = true;
 	}

I'm not sure this is a good idea though. Once we miss an interval, don't
we need to know the next frame when transfer needs to be scheduled?

Meaning we would need XferNotReady to properly schedule the new
transfer?

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Thinh Nguyen]

Hi,

On 4/10/2018 12:36 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Hi Felipe,
>>
>> On 4/9/2018 4:28 AM, Felipe Balbi wrote:
>>> In case we get an event with status set to Missed Isoc, this means we
>>> have missed an isochronous interval and should issue End Transfer
>>> command and wait for the following XferNotReady.
>>
>> Why does DWC3 need to issue End Transfer if there are still queued requests?
> 
> Without XferNotReady, we won't have a reliable way to know the uFrame
> number. Read the Isochronous programming sequence from your databook.

Right. We need XferNotReady to know when to start isoc transfer. But if 
there are still queued requests, DWC3 can just wait to see if any of 
them will succeed to continue with the transfer just as how DWC3 is 
handling it now. If we end and restart the transfer right away, then we 
may lose more isoc data than necessary (due to isoc scheduling at least 
4 uFrame ahead of time). Is there something you see that doesn't work 
with the current implementation?

> 
>>> @@ -2383,14 +2380,25 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>>>    {
>>>    	struct dwc3		*dwc = dep->dwc;
>>>    	unsigned		status = 0;
>>> +	bool			stop = false;
>>>    
>>>    	dwc3_gadget_endpoint_frame_from_event(dep, event);
>>>    
>>>    	if (event->status & DEPEVT_STATUS_BUSERR)
>>>    		status = -ECONNRESET;
>>>    
>>> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
>>> +		status = -ECONNRESET;
>>
>> Missed isoc shouldn't cause this error status or if it should return an
>> error status at all. Maybe the status can be -EXDEV, similar to the host
>> side (/Documentation/driver-api/usb/error-codes.rst).
> 
> fair enough. I'll change to EXDEV
> 

BR,
Thinh
---
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

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Thinh Nguyen]

Hi Felipe,

On 4/9/2018 4:28 AM, Felipe Balbi wrote:
> In case we get an event with status set to Missed Isoc, this means we
> have missed an isochronous interval and should issue End Transfer
> command and wait for the following XferNotReady.

Why does DWC3 need to issue End Transfer if there are still queued requests?

> 
> Let's do that early, rather than late.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>   drivers/usb/dwc3/core.h   |  5 +++--
>   drivers/usb/dwc3/gadget.c | 14 +++++++++++---
>   2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5ee895113906..8862118c3b79 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1192,11 +1192,12 @@ struct dwc3_event_depevt {
>   /* Within XferNotReady */
>   #define DEPEVT_STATUS_TRANSFER_ACTIVE	BIT(3)
>   
> -/* Within XferComplete */
> +/* Within XferComplete or XferInProgress */
>   #define DEPEVT_STATUS_BUSERR	BIT(0)
>   #define DEPEVT_STATUS_SHORT	BIT(1)
>   #define DEPEVT_STATUS_IOC	BIT(2)
> -#define DEPEVT_STATUS_LST	BIT(3)
> +#define DEPEVT_STATUS_LST	BIT(3) /* XferComplete */
> +#define DEPEVT_STATUS_MISSED_ISOC BIT(3) /* XferInProgress */
>   
>   /* Stream event only */
>   #define DEPEVT_STREAMEVT_FOUND		1
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0b003367cc7c..ed44b85e59dc 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2361,9 +2361,6 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
>   			 * entry is added into request list.
>   			 */
>   			dep->flags = DWC3_EP_PENDING_REQUEST;
> -		} else {
> -			dwc3_stop_active_transfer(dep, true);
> -			dep->flags = DWC3_EP_ENABLED;
>   		}
>   	}
>   }
> @@ -2383,14 +2380,25 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
>   {
>   	struct dwc3		*dwc = dep->dwc;
>   	unsigned		status = 0;
> +	bool			stop = false;
>   
>   	dwc3_gadget_endpoint_frame_from_event(dep, event);
>   
>   	if (event->status & DEPEVT_STATUS_BUSERR)
>   		status = -ECONNRESET;
>   
> +	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
> +		status = -ECONNRESET;

Missed isoc shouldn't cause this error status or if it should return an 
error status at all. Maybe the status can be -EXDEV, similar to the host 
side (/Documentation/driver-api/usb/error-codes.rst).

> +		stop = true;
> +	}
> +
>   	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
>   
> +	if (stop) {
> +		dwc3_stop_active_transfer(dep, true);
> +		dep->flags = DWC3_EP_ENABLED;
> +	}
> +
>   	/*
>   	 * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
>   	 * See dwc3_gadget_linksts_change_interrupt() for 1st half.
> 

BR,
Thinh
---
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

[RFT/PATCH,18/38] usb: dwc3: gadget: check for Missed Isoc from event status

[Felipe Balbi]

In case we get an event with status set to Missed Isoc, this means we
have missed an isochronous interval and should issue End Transfer
command and wait for the following XferNotReady.

Let's do that early, rather than late.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/core.h   |  5 +++--
 drivers/usb/dwc3/gadget.c | 14 +++++++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5ee895113906..8862118c3b79 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1192,11 +1192,12 @@ struct dwc3_event_depevt {
 /* Within XferNotReady */
 #define DEPEVT_STATUS_TRANSFER_ACTIVE	BIT(3)
 
-/* Within XferComplete */
+/* Within XferComplete or XferInProgress */
 #define DEPEVT_STATUS_BUSERR	BIT(0)
 #define DEPEVT_STATUS_SHORT	BIT(1)
 #define DEPEVT_STATUS_IOC	BIT(2)
-#define DEPEVT_STATUS_LST	BIT(3)
+#define DEPEVT_STATUS_LST	BIT(3) /* XferComplete */
+#define DEPEVT_STATUS_MISSED_ISOC BIT(3) /* XferInProgress */
 
 /* Stream event only */
 #define DEPEVT_STREAMEVT_FOUND		1
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 0b003367cc7c..ed44b85e59dc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2361,9 +2361,6 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
 			 * entry is added into request list.
 			 */
 			dep->flags = DWC3_EP_PENDING_REQUEST;
-		} else {
-			dwc3_stop_active_transfer(dep, true);
-			dep->flags = DWC3_EP_ENABLED;
 		}
 	}
 }
@@ -2383,14 +2380,25 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 {
 	struct dwc3		*dwc = dep->dwc;
 	unsigned		status = 0;
+	bool			stop = false;
 
 	dwc3_gadget_endpoint_frame_from_event(dep, event);
 
 	if (event->status & DEPEVT_STATUS_BUSERR)
 		status = -ECONNRESET;
 
+	if (event->status & DEPEVT_STATUS_MISSED_ISOC) {
+		status = -ECONNRESET;
+		stop = true;
+	}
+
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
+	if (stop) {
+		dwc3_stop_active_transfer(dep, true);
+		dep->flags = DWC3_EP_ENABLED;
+	}
+
 	/*
 	 * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
 	 * See dwc3_gadget_linksts_change_interrupt() for 1st half.