usb: dwc3: gadget: check for Missed Isoc from event status

Hi,

Quoted 38 lines · Felipe Balbi

Hi,

Quoted 34 lines · Felipe Balbi

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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:

(quoted diff) +0 -0

@@ -2371,7 +2371,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
  	if (event->status & DEPEVT_STATUS_BUSERR)
  		status = -ECONNRESET;
 

Quoted 4 lines

Review static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, · line 2376

Quoted 4 lines

(quoted diff) +2 -1

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

Quoted 9 lines

Quoted 8 lines

(quoted diff) +0 -0

  		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?

Quoted 4 lines

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.