usb: dwc3: gadget: check if dep->frame_number is still valid

Quoted 101 lines

Quoted 33 lines · Felipe Balbi

In the case of interval of 1ms, it will start on the next interval.

Quoted 6 lines

frame_number + max(4, interval) will start at least 4 uframes in the future.

In any case, what about immediately retry the START_TRANSFER command with a new frame_number + (interval*retry) should it fail with bus-expiry? You can set the number of retries to maybe 5 times. This should remove the need to do time stamping.

That seems like a good idea. Something like below? (on top of $subject)

modified drivers/usb/dwc3/core.h

(quoted diff) +1 -0

@@ -37,6 +37,7 @@
 #define DWC3_EP0_SETUP_SIZE	512
 #define DWC3_ENDPOINTS_NUM	32
 #define DWC3_XHCI_RESOURCES_NUM	2
+#define DWC3_ISOC_MAX_RETRIES	5
 
 #define DWC3_SCRATCHBUF_SIZE	4096	/* each buffer is assumed to be 4KiB */
 #define DWC3_EVENT_BUFFERS_SIZE	4096

modified drivers/usb/dwc3/gadget.c

(quoted diff) +3 -1

@@ -1271,20 +1271,29 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	u64 current_timestamp;
 	u64 diff_timestamp;
 	u32 elapsed_frames;
+	int retries;
+	int delta = 1
+	int ret;
 
 	if (list_empty(&dep->pending_list)) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
 		return -EAGAIN;
 	}
 
-	current_timestamp = ktime_get_ns();

Review static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) · line 1288

Quoted 10 lines

(quoted diff) +5 -4

-	diff_timestamp = current_timestamp - dep->frame_timestamp;
-	elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
+	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+		current_timestamp = ktime_get_ns();
+		diff_timestamp = current_timestamp - dep->frame_timestamp;
+		elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
 
-	dep->frame_number += elapsed_frames;
-	dep->frame_number = DWC3_ALIGN_FRAME(dep);
+		dep->frame_number += elapsed_frames + (delta * i);

the other possibility is that we can call DWC3_ALIGN_FRAME() n times since that will put the transfer on the following interval. That would look like so:

modified drivers/usb/dwc3/core.h

(quoted diff) +1 -0

@@ -37,6 +37,7 @@
 #define DWC3_EP0_SETUP_SIZE	512
 #define DWC3_ENDPOINTS_NUM	32
 #define DWC3_XHCI_RESOURCES_NUM	2
+#define DWC3_ISOC_MAX_RETRIES	5
 
 #define DWC3_SCRATCHBUF_SIZE	4096	/* each buffer is assumed to be 4KiB */
 #define DWC3_EVENT_BUFFERS_SIZE	4096

modified drivers/usb/dwc3/gadget.c

(quoted diff) +15 -7

@@ -27,7 +27,7 @@
 #include "gadget.h"
 #include "io.h"
 
-#define DWC3_ALIGN_FRAME(d)	(((d)->frame_number + (d)->interval) \
+#define DWC3_ALIGN_FRAME(d, n)	(((d)->frame_number + ((d)->interval * (n))) \
 					& ~((d)->interval - 1))
 
 /**

@@ -1271,20 +1271,28 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	u64 current_timestamp;
 	u64 diff_timestamp;
 	u32 elapsed_frames;
+	int retries;
+	int ret;
 
 	if (list_empty(&dep->pending_list)) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
 		return -EAGAIN;
 	}
 
-	current_timestamp = ktime_get_ns();
-	diff_timestamp = current_timestamp - dep->frame_timestamp;
-	elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
+	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
+		current_timestamp = ktime_get_ns();
+		diff_timestamp = current_timestamp - dep->frame_timestamp;
+		elapsed_frames = DIV_ROUND_UP_ULL(diff_timestamp, 125000);
 
-	dep->frame_number += elapsed_frames;
-	dep->frame_number = DWC3_ALIGN_FRAME(dep);
+		dep->frame_number += elapsed_frames;
+		dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
 
-	return __dwc3_gadget_kick_transfer(dep);
+		ret = __dwc3_gadget_kick_transfer(dep);
+		if (ret != -EAGAIN)
+			break;
+	}
+
+	return ret;
 }
 

Review static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) · line 1298

Quoted 3 lines

(quoted diff) +0 -0

 static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)

I like the second method. But do we need to keep track of the frame_timestamp? I don't think it accurately reflects the timestamp of

XferNotReady frame number.

As for the number of retries, we should adjust it according to the service interval. For example, for larger service interval such as 16, then we don't need to try more than once. To calculate the max number of retries, we can do this check (where interval is from 1 to 16):

if (interval >= (16 - (MAX_NUM_RETRIES >> 1))

        num_retries = 1 << (16 - interval);

Please check my math..

If it fails for over 5 times in a row, then we should probably wait for the next XferNotReady to get the frame_number again. Also 5 is an arbitrary number I came up without any testing, we can probably decide on a better number. What do you think?