usb: dwc3: gadget: Avoid canceling current request for queuing error

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Signed-off-by Wesley Cheng <[email protected]>

---
Changes in v2:
- Addressed feedback received by making sure the logic only applies to a req which is being queued, decrementing the enqueue pointer, and only clearing the HWO bit.

 drivers/usb/dwc3/gadget.c | 75 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 9 deletions(-)

drivers/usb/dwc3/gadget.c +66 -9

index dd80e5c..c8ddbe1 100644
@@ -140,6 +140,29 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
 }
 
 /**
+ * dwc3_ep_dec_trb - decrement a trb index.
+ * @index: Pointer to the TRB index to increment.
+ *
+ * The index should never point to the link TRB. After decrementing,
+ * if index is zero, wrap around to the TRB before the link TRB.
+ */
+static void dwc3_ep_dec_trb(u8 *index)
+{
+	(*index)--;
+	if (*index < 0)
+		*index = DWC3_TRB_NUM - 1;
+}
+
+/**
+ * dwc3_ep_dec_enq - decrement endpoint's enqueue pointer
+ * @dep: The endpoint whose enqueue pointer we're decrementing
+ */
+static void dwc3_ep_dec_enq(struct dwc3_ep *dep)
+{
+	dwc3_ep_dec_trb(&dep->trb_enqueue);
+}
+
+/**
  * dwc3_ep_inc_trb - increment a trb index.
  * @index: Pointer to the TRB index to increment.
  *

@@ -1352,7 +1375,26 @@ static int 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)
+static void dwc3_gadget_ep_revert_trbs(struct dwc3_ep *dep, struct dwc3_request *req)
+{
+	int i;
+
+	if (!req->trb)
+		return;
+
+	for (i = 0; i < req->num_trbs; i++) {
+		struct dwc3_trb *trb;
+
+		trb = &dep->trb_pool[dep->trb_enqueue];
+		trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+		dwc3_ep_dec_enq(dep);
+	}
+
+	req->num_trbs = 0;
+}
+
+static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep,
+				       struct dwc3_request *queued_req)
 {
 	struct dwc3_gadget_ep_cmd_params params;
 	struct dwc3_request		*req;

@@ -1410,8 +1452,23 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 
 		dwc3_stop_active_transfer(dep, true, true);
 
-		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
-			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_DEQUEUED);
+		/*
+		 * In order to ensure the logic is applied to a request being
+		 * queued by dwc3_gadget_ep_queue(), it needs to explicitly
+		 * check that req is the same as queued_req (request being
+		 * queued).  If so, then just unmap and decrement the enqueue
+		 * pointer, as the usb_ep_queue() error handler in the function
+		 * driver will handle cleaning up the USB request.
+		 */
+		list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
+			if (req == queued_req) {
+				dwc3_gadget_ep_revert_trbs(dep, req);
+				dwc3_gadget_del_and_unmap_request(dep, req, ret);
+			} else {
+				dwc3_gadget_move_cancelled_request(req,
+								   DWC3_REQUEST_STATUS_DEQUEUED);
+			}
+		}
 
 		/* If ep isn't started, then there's no end transfer pending */
 		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))

@@ -1546,7 +1603,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
 	dep->start_cmd_status = 0;
 	dep->combo_num = 0;
 
-	return __dwc3_gadget_kick_transfer(dep);
+	return __dwc3_gadget_kick_transfer(dep, NULL);
 }
 
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)

@@ -1593,7 +1650,7 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
 		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
 
-		ret = __dwc3_gadget_kick_transfer(dep);
+		ret = __dwc3_gadget_kick_transfer(dep, NULL);
 		if (ret != -EAGAIN)
 			break;
 	}

@@ -1684,7 +1741,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 		}
 	}
 
-	return __dwc3_gadget_kick_transfer(dep);
+	return __dwc3_gadget_kick_transfer(dep, req);
 }
 
 static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,

@@ -1893,7 +1950,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 
 		if ((dep->flags & DWC3_EP_DELAY_START) &&
 		    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
-			__dwc3_gadget_kick_transfer(dep);
+			__dwc3_gadget_kick_transfer(dep, NULL);
 
 		dep->flags &= ~DWC3_EP_DELAY_START;
 	}

@@ -2992,7 +3049,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
 		(list_empty(&dep->pending_list) || status == -EXDEV))
 		dwc3_stop_active_transfer(dep, true, true);
 	else if (dwc3_gadget_ep_should_continue(dep))
-		if (__dwc3_gadget_kick_transfer(dep) == 0)
+		if (__dwc3_gadget_kick_transfer(dep, NULL) == 0)
 			no_started_trb = false;
 
 out:

@@ -3106,7 +3163,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
 
 	if ((dep->flags & DWC3_EP_DELAY_START) &&
 	    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		__dwc3_gadget_kick_transfer(dep);
+		__dwc3_gadget_kick_transfer(dep, NULL);
 
 	dep->flags &= ~DWC3_EP_DELAY_START;
 }

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

Hi Felipe, can you also chime in?

Thanks,
Thinh

Quoted 153 lines

Signed-off-by Wesley Cheng <[email protected]>

---
Changes in v2:
- Addressed feedback received by making sure the logic only applies to a req which is being queued, decrementing the enqueue pointer, and only clearing the HWO bit.

 drivers/usb/dwc3/gadget.c | 75 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 9 deletions(-)

drivers/usb/dwc3/gadget.c +66 -9

index dd80e5c..c8ddbe1 100644
@@ -140,6 +140,29 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
 }
 
 /**
+ * dwc3_ep_dec_trb - decrement a trb index.
+ * @index: Pointer to the TRB index to increment.
+ *
+ * The index should never point to the link TRB. After decrementing,
+ * if index is zero, wrap around to the TRB before the link TRB.
+ */
+static void dwc3_ep_dec_trb(u8 *index)
+{
+	(*index)--;
+	if (*index < 0)
+		*index = DWC3_TRB_NUM - 1;
+}
+
+/**
+ * dwc3_ep_dec_enq - decrement endpoint's enqueue pointer
+ * @dep: The endpoint whose enqueue pointer we're decrementing
+ */
+static void dwc3_ep_dec_enq(struct dwc3_ep *dep)
+{
+	dwc3_ep_dec_trb(&dep->trb_enqueue);
+}
+
+/**
  * dwc3_ep_inc_trb - increment a trb index.
  * @index: Pointer to the TRB index to increment.
  *

@@ -1352,7 +1375,26 @@ static int 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)
+static void dwc3_gadget_ep_revert_trbs(struct dwc3_ep *dep, struct dwc3_request *req)
+{
+	int i;
+
+	if (!req->trb)
+		return;
+
+	for (i = 0; i < req->num_trbs; i++) {
+		struct dwc3_trb *trb;
+
+		trb = &dep->trb_pool[dep->trb_enqueue];
+		trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+		dwc3_ep_dec_enq(dep);
+	}
+
+	req->num_trbs = 0;
+}
+
+static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep,
+				       struct dwc3_request *queued_req)
 {
 	struct dwc3_gadget_ep_cmd_params params;
 	struct dwc3_request		*req;

@@ -1410,8 +1452,23 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 
 		dwc3_stop_active_transfer(dep, true, true);
 
-		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
-			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_DEQUEUED);
+		/*
+		 * In order to ensure the logic is applied to a request being
+		 * queued by dwc3_gadget_ep_queue(), it needs to explicitly
+		 * check that req is the same as queued_req (request being
+		 * queued).  If so, then just unmap and decrement the enqueue
+		 * pointer, as the usb_ep_queue() error handler in the function
+		 * driver will handle cleaning up the USB request.
+		 */
+		list_for_each_entry_safe(req, tmp, &dep->started_list, list) {
+			if (req == queued_req) {
+				dwc3_gadget_ep_revert_trbs(dep, req);
+				dwc3_gadget_del_and_unmap_request(dep, req, ret);
+			} else {
+				dwc3_gadget_move_cancelled_request(req,
+								   DWC3_REQUEST_STATUS_DEQUEUED);
+			}
+		}
 
 		/* If ep isn't started, then there's no end transfer pending */
 		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))

@@ -1546,7 +1603,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
 	dep->start_cmd_status = 0;
 	dep->combo_num = 0;
 
-	return __dwc3_gadget_kick_transfer(dep);
+	return __dwc3_gadget_kick_transfer(dep, NULL);
 }
 
 static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)

@@ -1593,7 +1650,7 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
 		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
 
-		ret = __dwc3_gadget_kick_transfer(dep);
+		ret = __dwc3_gadget_kick_transfer(dep, NULL);
 		if (ret != -EAGAIN)
 			break;
 	}

@@ -1684,7 +1741,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
 		}
 	}
 
-	return __dwc3_gadget_kick_transfer(dep);
+	return __dwc3_gadget_kick_transfer(dep, req);
 }
 
 static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,

@@ -1893,7 +1950,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 
 		if ((dep->flags & DWC3_EP_DELAY_START) &&
 		    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
-			__dwc3_gadget_kick_transfer(dep);
+			__dwc3_gadget_kick_transfer(dep, NULL);
 
 		dep->flags &= ~DWC3_EP_DELAY_START;
 	}

@@ -2992,7 +3049,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
 		(list_empty(&dep->pending_list) || status == -EXDEV))
 		dwc3_stop_active_transfer(dep, true, true);
 	else if (dwc3_gadget_ep_should_continue(dep))
-		if (__dwc3_gadget_kick_transfer(dep) == 0)
+		if (__dwc3_gadget_kick_transfer(dep, NULL) == 0)
 			no_started_trb = false;
 
 out:

@@ -3106,7 +3163,7 @@ static void dwc3_gadget_endpoint_command_complete(struct dwc3_ep *dep,
 
 	if ((dep->flags & DWC3_EP_DELAY_START) &&
 	    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
-		__dwc3_gadget_kick_transfer(dep);
+		__dwc3_gadget_kick_transfer(dep, NULL);
 
 	dep->flags &= ~DWC3_EP_DELAY_START;
 }

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Quoted 6 lines

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

We do have something similar as well downstream (returning success always on dwc3_gadget_ep_queue()) and its been working for us also. Problem is we don't test the ISOC path much, so this is the only type of EP that might come into question...

Coming up with a way to address the concerns you brought up was a bit difficult as there were scenarios we needed to consider. next_request() doesn't always have to be the request being queued (even if ep queue triggered it). There was no easy way to determine if kick transfer was due to ep queue, but even if there was, we'd need to remember the previous point as well.

Hi Wesley,

Quoted 49 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

sounds like a race issue.

Quoted 9 lines

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Quoted 2 lines

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Do you know which command and error code? It's strange if UPDATE_TRANSFER command failed.

Quoted 7 lines

Quoted 6 lines

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

Quoted 5 lines

We do have something similar as well downstream (returning success always on dwc3_gadget_ep_queue()) and its been working for us also. Problem is we don't test the ISOC path much, so this is the only type of EP that might come into question...

It should be similiar with isoc. I can't think of a potential issue yet.

Quoted 7 lines

Coming up with a way to address the concerns you brought up was a bit difficult as there were scenarios we needed to consider. next_request() doesn't always have to be the request being queued (even if ep queue triggered it). There was no easy way to determine if kick transfer was due to ep queue, but even if there was, we'd need to remember the previous point as well.

Yeah, there are a few pitfalls. I don't have a good solution to it if we want to return failure immediately and let the function driver handle the dequeue (if it wants to).

Thanks,
Thinh

Quoted 52 lines · Thinh Nguyen

Hi Wesley,

Quoted 49 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Hi Thinh,

Quoted 3 lines

sounds like a race issue.

It'll always happen if usb_ep_queue() fails with an error. Sorry for not clarifying, but the "..." represents executing in a different context :). Anything above the "..." is in the same context.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Quoted 6 lines

Quoted 2 lines

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Do you know which command and error code? It's strange if UPDATE_TRANSFER command failed.

Sorry for missing that part of the question. It is a no xfer resource error on a start transfer command. So far this happens on low system memory test cases, so there may be some sequences that were missed, which led to this particular command error.

Thanks
Wesley Cheng

Quoted 31 lines

Quoted 8 lines

Quoted 7 lines

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

Quoted 5 lines

We do have something similar as well downstream (returning success always on dwc3_gadget_ep_queue()) and its been working for us also. Problem is we don't test the ISOC path much, so this is the only type of EP that might come into question...

It should be similiar with isoc. I can't think of a potential issue yet.

Quoted 7 lines

Coming up with a way to address the concerns you brought up was a bit difficult as there were scenarios we needed to consider. next_request() doesn't always have to be the request being queued (even if ep queue triggered it). There was no easy way to determine if kick transfer was due to ep queue, but even if there was, we'd need to remember the previous point as well.

Yeah, there are a few pitfalls. I don't have a good solution to it if we want to return failure immediately and let the function driver handle the dequeue (if it wants to).

Thanks,
Thinh

Quoted 81 lines · Wesley Cheng

Quoted 52 lines · Thinh Nguyen

Hi Wesley,

Quoted 49 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Hi Thinh,

Quoted 3 lines

sounds like a race issue.

It'll always happen if usb_ep_queue() fails with an error. Sorry for not clarifying, but the "..." represents executing in a different context :). Anything above the "..." is in the same context.

Quoted 15 lines

Quoted 11 lines

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Do you know which command and error code? It's strange if UPDATE_TRANSFER command failed.

Quoted 7 lines

Sorry for missing that part of the question. It is a no xfer resource error on a start transfer command. So far this happens on low system memory test cases, so there may be some sequences that were missed, which led to this particular command error.

Thanks
Wesley Cheng

No xfer resource usually means that the driver attempted to send START_TRANSFER without waiting for END_TRANSFER command to complete. This may be a dwc3 driver issue. Did you check this?

Thanks,
Thinh

Quoted 32 lines

Quoted 31 lines

Quoted 8 lines

Quoted 7 lines

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

Quoted 5 lines

We do have something similar as well downstream (returning success always on dwc3_gadget_ep_queue()) and its been working for us also. Problem is we don't test the ISOC path much, so this is the only type of EP that might come into question...

It should be similiar with isoc. I can't think of a potential issue yet.

Quoted 7 lines

Coming up with a way to address the concerns you brought up was a bit difficult as there were scenarios we needed to consider. next_request() doesn't always have to be the request being queued (even if ep queue triggered it). There was no easy way to determine if kick transfer was due to ep queue, but even if there was, we'd need to remember the previous point as well.

Yeah, there are a few pitfalls. I don't have a good solution to it if we want to return failure immediately and let the function driver handle the dequeue (if it wants to).

Thanks,
Thinh

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 52 lines · Thinh Nguyen

Hi Wesley,

Quoted 49 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Hi Thinh,

Quoted 3 lines

sounds like a race issue.

It'll always happen if usb_ep_queue() fails with an error. Sorry for not clarifying, but the "..." represents executing in a different context :). Anything above the "..." is in the same context.

Quoted 15 lines

Quoted 11 lines

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Do you know which command and error code? It's strange if UPDATE_TRANSFER command failed.

Quoted 7 lines

Quoted 7 lines

Sorry for missing that part of the question. It is a no xfer resource error on a start transfer command. So far this happens on low system memory test cases, so there may be some sequences that were missed, which led to this particular command error.

Thanks
Wesley Cheng

Hi Thinh,

Quoted 9 lines

No xfer resource usually means that the driver attempted to send START_TRANSFER without waiting for END_TRANSFER command to complete. This may be a dwc3 driver issue. Did you check this?

Thanks,
Thinh

Yes, we know the reason why this happens, and its due to one of the downstream changes we had that led to the scenario above. Although, that has been fixed, I still believe the error path is a potential scenario we'd still want to address.

I think the returning success always on dwc3_gadget_ep_queue(), and allowing the error in the completion handler/giveback at the function driver level to do the cleanup is a feasible solution. Doesn't change the flow of the DWC3 gadget, and so far all function drivers we've used handle this in the correct manner.

Thanks
Wesley Cheng

Quoted 34 lines

Quoted 33 lines

Quoted 32 lines

Quoted 9 lines

Quoted 8 lines

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

Quoted 5 lines

We do have something similar as well downstream (returning success always on dwc3_gadget_ep_queue()) and its been working for us also. Problem is we don't test the ISOC path much, so this is the only type of EP that might come into question...

It should be similiar with isoc. I can't think of a potential issue yet.

Quoted 7 lines

Coming up with a way to address the concerns you brought up was a bit difficult as there were scenarios we needed to consider. next_request() doesn't always have to be the request being queued (even if ep queue triggered it). There was no easy way to determine if kick transfer was due to ep queue, but even if there was, we'd need to remember the previous point as well.

Yeah, there are a few pitfalls. I don't have a good solution to it if we want to return failure immediately and let the function driver handle the dequeue (if it wants to).

Thanks,
Thinh

Quoted 110 lines · Wesley Cheng

Quoted 82 lines · Thinh Nguyen

Quoted 81 lines · Wesley Cheng

Quoted 52 lines · Thinh Nguyen

Hi Wesley,

Quoted 49 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Hi Thinh,

Quoted 3 lines

sounds like a race issue.

It'll always happen if usb_ep_queue() fails with an error. Sorry for not clarifying, but the "..." represents executing in a different context :). Anything above the "..." is in the same context.

Quoted 15 lines

Quoted 11 lines

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Do you know which command and error code? It's strange if UPDATE_TRANSFER command failed.

Quoted 7 lines

Quoted 7 lines

Sorry for missing that part of the question. It is a no xfer resource error on a start transfer command. So far this happens on low system memory test cases, so there may be some sequences that were missed, which led to this particular command error.

Thanks
Wesley Cheng

Hi Thinh,

Quoted 9 lines

No xfer resource usually means that the driver attempted to send START_TRANSFER without waiting for END_TRANSFER command to complete. This may be a dwc3 driver issue. Did you check this?

Thanks,
Thinh

Yes, we know the reason why this happens, and its due to one of the downstream changes we had that led to the scenario above. Although, that has been fixed, I still believe the error path is a potential scenario we'd still want to address.

Quoted 8 lines

I think the returning success always on dwc3_gadget_ep_queue(), and allowing the error in the completion handler/giveback at the function driver level to do the cleanup is a feasible solution. Doesn't change the flow of the DWC3 gadget, and so far all function drivers we've used handle this in the correct manner.

Thanks
Wesley Cheng

Right. I think for now we should do that (return success always except for cases of disconnect or already in-flight etc). This helps keeping it simple and avoid some pitfalls dealing with giving back the request. Currently we return the error status to dwc3_gadget_ep_queue if we failed to send a command that may not even related to the same request being queued.

This way, I think it matches with how we handle it in the driver. We always put the request in the pending list (queued) first and possibly start/update the controller with new data.

Thanks,
Thinh

Quoted 35 lines

Quoted 34 lines

Quoted 33 lines

Quoted 32 lines

Quoted 9 lines

Quoted 8 lines

This seems to add more complexity and I don't have a good solution to it. Since you're already cancelling all the active request anyway, what do you think of always letting dwc3_gadget_ep_queue() to go through with success, but report failure through request completion?

Quoted 5 lines

We do have something similar as well downstream (returning success always on dwc3_gadget_ep_queue()) and its been working for us also. Problem is we don't test the ISOC path much, so this is the only type of EP that might come into question...

It should be similiar with isoc. I can't think of a potential issue yet.

Quoted 7 lines

Coming up with a way to address the concerns you brought up was a bit difficult as there were scenarios we needed to consider. next_request() doesn't always have to be the request being queued (even if ep queue triggered it). There was no easy way to determine if kick transfer was due to ep queue, but even if there was, we'd need to remember the previous point as well.

Yeah, there are a few pitfalls. I don't have a good solution to it if we want to return failure immediately and let the function driver handle the dequeue (if it wants to).

Thanks,
Thinh

Quoted 71 lines

Quoted 40 lines

Quoted 40 lines

Quoted 12 lines

Quoted 12 lines

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

Quoted 2 lines

Quoted 2 lines

work scheduled
--> NULL pointer/memory fault as io_data is freed

Hi Thinh,

Quoted 3 lines

sounds like a race issue.

It'll always happen if usb_ep_queue() fails with an error. Sorry for not clarifying, but the "..." represents executing in a different context :). Anything above the "..." is in the same context.

Quoted 15 lines

Quoted 11 lines

Quoted 8 lines

BTW, what kinds of command and error do you see in your setup and for what type endpoint? I'm thinking of letting the function driver to dequeue the requests instead of letting dwc3 automatically ending/cancelling the queued requests. However, it's a bit tricky to do that if the error is -ETIMEDOUT since we're not sure if the controller had already cached the TRBs.

Happens on bulk EPs so far, but I think it wouldn't matter as long as its over the FFS interface. (and using async IO transfers)

Do you know which command and error code? It's strange if UPDATE_TRANSFER command failed.

Quoted 7 lines

Quoted 7 lines

Sorry for missing that part of the question. It is a no xfer resource error on a start transfer command. So far this happens on low system memory test cases, so there may be some sequences that were missed, which led to this particular command error.

Thanks
Wesley Cheng

Hi Thinh,

Quoted 9 lines

No xfer resource usually means that the driver attempted to send START_TRANSFER without waiting for END_TRANSFER command to complete. This may be a dwc3 driver issue. Did you check this?

Thanks,
Thinh

Yes, we know the reason why this happens, and its due to one of the downstream changes we had that led to the scenario above. Although, that has been fixed, I still believe the error path is a potential scenario we'd still want to address.

I think the returning success always on dwc3_gadget_ep_queue(), and allowing the error in the completion handler/giveback at the function driver level to do the cleanup is a feasible solution. Doesn't change the flow of the DWC3 gadget, and so far all function drivers we've used handle this in the correct manner.

Quoted 2 lines

Thanks
Wesley Cheng

Right. I think for now we should do that (return success always except for cases of disconnect or already in-flight etc). This helps keeping it

simple and avoid some pitfalls dealing with giving back the request. Currently we return the error status to dwc3_gadget_ep_queue if we failed to send a command that may not even related to the same request being queued.

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Hi,

Quoted 47 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this.

OTOH, we're returning failure during
usb_ep_queue() which tells me there's something with dwc3 (perhaps not exclusively, but that's yet to be shown).

If I understood the whole thing correctly, we want everything except the current request (the one that failed START or UPDATE transfer) to go through giveback(). This really tells me that we're not handling error case in kick_transfer and/or prepare_trbs() correctly.

I also don't want to pass another argument to kick_transfer because it should be unnecessary: the current request should *always* be the last one in the list. Therefore we should rely on something like list_last_entry() followed by list_for_each_entry_safe_reverse() to handle this without a special case.

ret = dwc3_send_gadget_ep_cmd();

if (ret < 0) {
	current = list_last_entry();

	unmap(current);
        for_each_trb_in(current) {
        	clear_HWO(trb);
        }

	list_for_entry_safe_reverse() {
        	move_cancelled();
        }
}

Signature · 1 line

balbi

Quoted 49 lines · Felipe Balbi

Hi,

Quoted 45 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

Quoted 2 lines

Quoted 2 lines

work scheduled
--> NULL pointer/memory fault as io_data is freed

Am I reading this correctly? It looks like usb_ep_queue() returns a -EINVAL error, but then the completion callback gets invoked with a status of -ECONNRESET.

Quoted 3 lines

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this.

Indeed, this predates the creation of the Gadget API; the same design principle applies to the host-side API. It's a very simple idea:

If an URB or usb_request submission succeeds, it is guaranteed that the completion routine will be called when the transfer is finished, cancelled, aborted, or whatever (note that this may happen before the submission call returns).

If an URB or usb_request submission fails, it is guaranteed that the completion routine will not be called.

So if dwc3 behaves as described above (usb_ep_queue() fails _and_ the completion handler is called), this is a bug

Alan Stern

Quoted 31 lines

OTOH, we're returning failure during
usb_ep_queue() which tells me there's something with dwc3 (perhaps not exclusively, but that's yet to be shown).

If I understood the whole thing correctly, we want everything except the current request (the one that failed START or UPDATE transfer) to go through giveback(). This really tells me that we're not handling error case in kick_transfer and/or prepare_trbs() correctly.

I also don't want to pass another argument to kick_transfer because it should be unnecessary: the current request should *always* be the last one in the list. Therefore we should rely on something like list_last_entry() followed by list_for_each_entry_safe_reverse() to handle this without a special case.

ret = dwc3_send_gadget_ep_cmd();

if (ret < 0) {
	current = list_last_entry();

	unmap(current);
        for_each_trb_in(current) {
        	clear_HWO(trb);
        }

	list_for_entry_safe_reverse() {
        	move_cancelled();
        }
}

Signature · 1 line

balbi

Quoted 51 lines · Felipe Balbi

Hi,

Quoted 47 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Am I reading this correctly? It looks like usb_ep_queue() returns a -EINVAL error, but then the completion callback gets invoked with a status of -ECONNRESET.

Quoted 3 lines

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this.

Indeed, this predates the creation of the Gadget API; the same design principle applies to the host-side API. It's a very simple idea:

If an URB or usb_request submission succeeds, it is guaranteed that the completion routine will be called when the transfer is finished, cancelled, aborted, or whatever (note that this may happen before the submission call returns).

If an URB or usb_request submission fails, it is guaranteed that the completion routine will not be called.

So if dwc3 behaves as described above (usb_ep_queue() fails _and_ the completion handler is called), this is a bug.

Alan Stern

Quoted 69 lines · Alan Stern

Quoted 51 lines · Felipe Balbi

Hi,

Quoted 47 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Am I reading this correctly? It looks like usb_ep_queue() returns a -EINVAL error, but then the completion callback gets invoked with a status of -ECONNRESET.

Quoted 3 lines

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this.

Indeed, this predates the creation of the Gadget API; the same design principle applies to the host-side API. It's a very simple idea:

If an URB or usb_request submission succeeds, it is guaranteed that the completion routine will be called when the transfer is finished, cancelled, aborted, or whatever (note that this may happen before the submission call returns).

Quoted 8 lines

If an URB or usb_request submission fails, it is guaranteed that the completion routine will not be called.

So if dwc3 behaves as described above (usb_ep_queue() fails _and_ the completion handler is called), this is a bug.

Alan Stern

Hi Alan,

Yes, it's a bug, no question about that. The concern here is how should we fix it.

In dwc3, when the usb_ep_queue() is called, it does 3 main things:
1) Put the request in a pending list to be processed
2) Process any started but partially processed request and any outstanding request from the pending list and map them to TRBs
3) Send a command to the controller telling it to cache and process these new TRBs

Currently, if step 3) fails, then usb_ep_queue() returns an error status and we stop the controller from processing TRBs and return any request related to those outstanding TRBs. This is a bug.

My suggestion here is don't immediately return an error code and let the completion interrupt inform of a transfer failure. The reasons are:

a) Step 3) happened when the request is already (arguably) queued.
b) If the error from step 3) is due to command timed out, the controller may have partially cached/processed some of these TRBs. We can't simply give back the request immediately without stopping the transfer and fail all the in-flight request.
c) It adds unnecessary complexity to the driver and there are a few pitfalls that we have to watch out for when handling giving back the request.
d) Except the case where the device is disconnected or that the request is already in-flight, step 1) and 2) are always done after usb_ep_queue(). The controller driver already owns these requests and can be considered "queued".

If our definition whether a request is "queued" must be a combination of all those 3 steps, then my suggestion is not enough and we'd have to explore other options.

Note that we already handle it this way for isoc this way. We don't send the START_TRANSFER command immediately and consider the requests to be queued in the usb_ep_queue(). So here we're just extending this to other endpoints too.

Quoted 69 lines · Alan Stern

Quoted 51 lines · Felipe Balbi

Hi,

Quoted 47 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Am I reading this correctly? It looks like usb_ep_queue() returns a -EINVAL error, but then the completion callback gets invoked with a status of -ECONNRESET.

Quoted 3 lines

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this.

Indeed, this predates the creation of the Gadget API; the same design principle applies to the host-side API. It's a very simple idea:

If an URB or usb_request submission succeeds, it is guaranteed that the completion routine will be called when the transfer is finished, cancelled, aborted, or whatever (note that this may happen before the submission call returns).

Quoted 8 lines

If an URB or usb_request submission fails, it is guaranteed that the completion routine will not be called.

So if dwc3 behaves as described above (usb_ep_queue() fails _and_ the completion handler is called), this is a bug.

Alan Stern

Hi Alan,

Yes, it's a bug, no question about that. The concern here is how should we fix it.

In dwc3, when the usb_ep_queue() is called, it does 3 main things:
1) Put the request in a pending list to be processed
2) Process any started but partially processed request and any outstanding request from the pending list and map them to TRBs
3) Send a command to the controller telling it to cache and process these new TRBs

Currently, if step 3) fails, then usb_ep_queue() returns an error status and we stop the controller from processing TRBs and return any request related to those outstanding TRBs. This is a bug.

My suggestion here is don't immediately return an error code and let the completion interrupt inform of a transfer failure. The reasons are:

a) Step 3) happened when the request is already (arguably) queued.
b) If the error from step 3) is due to command timed out, the controller may have partially cached/processed some of these TRBs. We can't simply give back the request immediately without stopping the transfer and fail all the in-flight request.
c) It adds unnecessary complexity to the driver and there are a few pitfalls that we have to watch out for when handling giving back the request.
d) Except the case where the device is disconnected or that the request is already in-flight, step 1) and 2) are always done after usb_ep_queue(). The controller driver already owns these requests and can be considered "queued".

Hi,

Quoted 99 lines · Thinh Nguyen

Quoted 69 lines · Alan Stern

Quoted 51 lines · Felipe Balbi

Hi,

Quoted 47 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

work scheduled
--> NULL pointer/memory fault as io_data is freed

Am I reading this correctly? It looks like usb_ep_queue() returns a -EINVAL error, but then the completion callback gets invoked with a status of -ECONNRESET.

Quoted 3 lines

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this.

Indeed, this predates the creation of the Gadget API; the same design principle applies to the host-side API. It's a very simple idea:

If an URB or usb_request submission succeeds, it is guaranteed that the completion routine will be called when the transfer is finished, cancelled, aborted, or whatever (note that this may happen before the submission call returns).

Quoted 8 lines

If an URB or usb_request submission fails, it is guaranteed that the completion routine will not be called.

So if dwc3 behaves as described above (usb_ep_queue() fails _and_ the completion handler is called), this is a bug.

Alan Stern

Hi Alan,

Yes, it's a bug, no question about that. The concern here is how should we fix it.

In dwc3, when the usb_ep_queue() is called, it does 3 main things:
1) Put the request in a pending list to be processed
2) Process any started but partially processed request and any outstanding request from the pending list and map them to TRBs
3) Send a command to the controller telling it to cache and process these new TRBs

Currently, if step 3) fails, then usb_ep_queue() returns an error status and we stop the controller from processing TRBs and return any request related to those outstanding TRBs. This is a bug.

My suggestion here is don't immediately return an error code and let the completion interrupt inform of a transfer failure. The reasons are:

Quoted 12 lines

a) Step 3) happened when the request is already (arguably) queued.
b) If the error from step 3) is due to command timed out, the controller may have partially cached/processed some of these TRBs. We can't simply give back the request immediately without stopping the transfer and fail all the in-flight request.
c) It adds unnecessary complexity to the driver and there are a few pitfalls that we have to watch out for when handling giving back the request.
d) Except the case where the device is disconnected or that the request is already in-flight, step 1) and 2) are always done after usb_ep_queue(). The controller driver already owns these requests and can be considered "queued".

Oh, I see. Indeed this sounds like the best minimal fix for the -rc and backporting to stables. We may still want to get back to this at some point and, potentially, split kick_transfer() into two parts that can make assumptions about the context where they can be called.

Also, we may want to move the request to the pending list only if the command succeeds. There should be no race condition as kick_transfer is always called with interrupts disabled.

Hi,

Quoted 45 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

Quoted 2 lines

work scheduled
--> NULL pointer/memory fault as io_data is freed

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this. OTOH, we're returning failure during usb_ep_queue() which tells me there's something with dwc3 (perhaps not exclusively, but that's yet to be shown).

If I understood the whole thing correctly, we want everything except the current request (the one that failed START or UPDATE transfer) to go through giveback(). This really tells me that we're not handling error case in kick_transfer and/or prepare_trbs() correctly.

I also don't want to pass another argument to kick_transfer because it should be unnecessary: the current request should *always* be the last one in the list. Therefore we should rely on something like list_last_entry() followed by list_for_each_entry_safe_reverse() to handle this without a special case.

ret = dwc3_send_gadget_ep_cmd();

if (ret < 0) {
	current = list_last_entry();

	unmap(current);
        for_each_trb_in(current) {
        	clear_HWO(trb);
        }

	list_for_entry_safe_reverse() {
        	move_cancelled();
        }
}

Quoted 5 lines

If I understood the whole thing correctly, we want everything except the current request (the one that failed START or UPDATE transfer) to go through giveback(). This really tells me that we're not handling error case in kick_transfer and/or prepare_trbs() correctly.

We don't want the request passed in usb_ep_queue() to be calling giveback() IF DONE IN the usb_ep_queue() context only.

Quoted 7 lines

I also don't want to pass another argument to kick_transfer because it should be unnecessary: the current request should *always* be the last one in the list. Therefore we should rely on something like list_last_entry() followed by list_for_each_entry_safe_reverse() to handle this without a special case.

ret = dwc3_send_gadget_ep_cmd();

Quoted 13 lines

if (ret < 0) {
	current = list_last_entry();

	unmap(current);
        for_each_trb_in(current) {
        	clear_HWO(trb);
        }

	list_for_entry_safe_reverse() {
        	move_cancelled();
        }
}

Nice, thanks for the suggestion and info! Problem we have is that kick transfer is being used elsewhere, for example, during the TRB complete path:

static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
const struct dwc3_event_depevt *event, int status)
{
...

	else if (dwc3_gadget_ep_should_continue(dep))
		if (__dwc3_gadget_kick_transfer(dep) == 0)
			no_started_trb = false;

So in these types of calls, we would still want ALL requests to be cancelled w/ giveback() called, so that the completion() callbacks can cleanup/free those requests accordingly.

If we went and only unmapped the last entry (and removed it from any list), then no one would clean it up as it is outside of the usb_ep_queue() context, and not within any of the DWC3 lists.

Hi,

Quoted 45 lines · Wesley Cheng

Quoted 12 lines · Thinh Nguyen

Hi,

Quoted 9 lines · Wesley Cheng

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Hi Thinh,

Quoted 4 lines

It looks like you're still letting dwc3 stopping and cancelling all the active requests instead letting the function driver doing the dequeue.

Yeah, main issue isn't due to the function driver doing dequeue, but having cleanup (ie USB request free) if there is an error during usb_ep_queue().

The function driver in question at the moment is the f_fs driver in AIO mode. When async IO is enabled in the FFS driver, every time it queues a packet, it will allocate a io_data struct beforehand. If the usb_ep_queue() fails it will free this io_data memory. Problem is that, since the DWC3 gadget calls the completion with -ECONNRESET, the FFS driver will also schedule a work item (within io_data struct) to handle the completion. So you end up with a flow like below

allocate io_data (ffs)
--> usb_ep_queue()
--> __dwc3_gadget_kick_transfer()
--> dwc3_send_gadget_ep_cmd(EINVAL)
--> dwc3_gadget_ep_cleanup_cancelled_requests() --> dwc3_gadget_giveback(ECONNRESET)
ffs completion callback
queue work item within io_data
--> usb_ep_queue returns EINVAL
ffs frees io_data
...

Quoted 2 lines

work scheduled
--> NULL pointer/memory fault as io_data is freed

I have some vague memory of discussing (something like) this with Alan Stern long ago and the conclusion was that the gadget driver should handle cases such as this. OTOH, we're returning failure during usb_ep_queue() which tells me there's something with dwc3 (perhaps not exclusively, but that's yet to be shown).

If I understood the whole thing correctly, we want everything except the current request (the one that failed START or UPDATE transfer) to go through giveback(). This really tells me that we're not handling error case in kick_transfer and/or prepare_trbs() correctly.

I also don't want to pass another argument to kick_transfer because it should be unnecessary: the current request should *always* be the last one in the list. Therefore we should rely on something like list_last_entry() followed by list_for_each_entry_safe_reverse() to handle this without a special case.

ret = dwc3_send_gadget_ep_cmd();

if (ret < 0) {
	current = list_last_entry();

	unmap(current);
        for_each_trb_in(current) {
        	clear_HWO(trb);
        }

	list_for_entry_safe_reverse() {
        	move_cancelled();
        }
}

If an error is received when issuing a start or update transfer command, the error handler will stop all active requests (including the current USB request), and call dwc3_gadget_giveback() to notify function drivers of the requests which have been stopped. Avoid having to cancel the current request which is trying to be queued, as the function driver will handle the EP queue error accordingly. Simply unmap the request as it was done before, and allow previously started transfers to be cleaned up.

Signed-off-by Wesley Cheng <[email protected]>

---
Changes in v2:
- Addressed feedback received by making sure the logic only applies to a req which is being queued, decrementing the enqueue pointer, and only clearing the HWO bit.

 drivers/usb/dwc3/gadget.c | 75 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 9 deletions(-)

drivers/usb/dwc3/gadget.c +16 -0

index dd80e5c..c8ddbe1 100644
@@ -140,6 +140,29 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state)
 }
 
 /**
+ * dwc3_ep_dec_trb - decrement a trb index.
+ * @index: Pointer to the TRB index to increment.
+ *
+ * The index should never point to the link TRB. After decrementing,
+ * if index is zero, wrap around to the TRB before the link TRB.
+ */
+static void dwc3_ep_dec_trb(u8 *index)
+{
+	(*index)--;
+	if (*index < 0)
+		*index = DWC3_TRB_NUM - 1;
+}
+
+/**
+ * dwc3_ep_dec_enq - decrement endpoint's enqueue pointer
+ * @dep: The endpoint whose enqueue pointer we're decrementing

drivers/usb/dwc3/gadget.c +1 -1

@@ -1352,7 +1375,26 @@ static int 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)
+static void dwc3_gadget_ep_revert_trbs(struct dwc3_ep *dep, struct dwc3_request *req)

drivers/usb/dwc3/gadget.c +5 -2

@@ -1410,8 +1452,23 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 
 		dwc3_stop_active_transfer(dep, true, true);
 
-		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
-			dwc3_gadget_move_cancelled_request(req, DWC3_REQUEST_STATUS_DEQUEUED);
+		/*
+		 * In order to ensure the logic is applied to a request being
+		 * queued by dwc3_gadget_ep_queue(), it needs to explicitly
+		 * check that req is the same as queued_req (request being
+		 * queued).  If so, then just unmap and decrement the enqueue