usb: dwc3: gadget: Improve isoc starting mechanism

If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we should properly end an active transfer and give back all the started requests. However if it's for an isoc endpoint, the failure maybe due to bus-expiry status. In this case, don't give back the requests and wait for the next retry.

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")

Signed-off-by Thinh Nguyen <[email protected]>

After a number of unsuccessful start isoc attempts due to bus-expiry status, issue END_TRANSFER command and retry on the next XferNotReady event.

Signed-off-by Thinh Nguyen <[email protected]>

---
 drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

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

index f1aae4615cf1..a5ad02987536 100644
@@ -1406,7 +1406,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 	int ret;
 	int i;
 
-	if (list_empty(&dep->pending_list)) {
+	if (list_empty(&dep->pending_list) &&
+	    list_empty(&dep->started_list)) {
 		dep->flags |= DWC3_EP_PENDING_REQUEST;
 		return -EAGAIN;
 	}

@@ -1429,6 +1430,27 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
 			break;
 	}
 
+	/*
+	 * After a number of unsuccessful start attempts due to bus-expiry
+	 * status, issue END_TRANSFER command and retry on the next XferNotReady
+	 * event.
+	 */
+	if (ret == -EAGAIN) {
+		struct dwc3_gadget_ep_cmd_params params;
+		u32 cmd;
+
+		cmd = DWC3_DEPCMD_ENDTRANSFER |

Hi Thinh,

Quoted 6 lines · Thinh Nguyen

If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we should properly end an active transfer and give back all the started requests. However if it's for an isoc endpoint, the failure maybe due to bus-expiry status. In this case, don't give back the requests and wait for the next retry.

Quoted 2 lines

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")

Signed-off-by Thinh Nguyen <[email protected]>

could you give some details regarding when does this happen?

Hi,

Quoted 41 lines · Thinh Nguyen

After a number of unsuccessful start isoc attempts due to bus-expiry status, issue END_TRANSFER command and retry on the next XferNotReady event.

Signed-off-by Thinh Nguyen <[email protected]>

---
  drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
  1 file changed, 35 insertions(+), 1 deletion(-)

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

index f1aae4615cf1..a5ad02987536 100644
@@ -1406,7 +1406,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
  	int ret;
  	int i;
 
-	if (list_empty(&dep->pending_list)) {
+	if (list_empty(&dep->pending_list) &&
+	    list_empty(&dep->started_list)) {
  		dep->flags |= DWC3_EP_PENDING_REQUEST;
  		return -EAGAIN;
  	}

@@ -1429,6 +1430,27 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
  			break;
  	}
 
+	/*
+	 * After a number of unsuccessful start attempts due to bus-expiry
+	 * status, issue END_TRANSFER command and retry on the next XferNotReady
+	 * event.
+	 */
+	if (ret == -EAGAIN) {
+		struct dwc3_gadget_ep_cmd_params params;
+		u32 cmd;
+
+		cmd = DWC3_DEPCMD_ENDTRANSFER |
+			DWC3_DEPCMD_CMDIOC |
+			DWC3_DEPCMD_PARAM(dep->resource_index);
+
+		dep->resource_index = 0;

drivers/usb/dwc3/gadget.c static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) · line 1452

Quoted 6 lines

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

+		memset(&params, 0, sizeof(params));
+
+		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+		if (!ret)
+			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	}

I like this! Pretty good idea :-) I'll wait for your reply to my question on the other patch, then start queueing again.

Quoted 11 lines

Quoted 8 lines · Thinh Nguyen

If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we should properly end an active transfer and give back all the started requests. However if it's for an isoc endpoint, the failure maybe due to bus-expiry status. In this case, don't give back the requests and wait for the next retry.

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")

Signed-off-by Thinh Nguyen <[email protected]>

could you give some details regarding when does this happen?

So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return a negative errno:

* -EAGAIN: Isoc bus-expiry status
   As you already know, this occurs when we try to schedule isoc too
late. If we're going to retry the request, don't unmap it.

* -EINVAL: No resource due to issuing START_TRANSFER to an already started endpoint
   This happens generally because of SW programming error

* -ETIMEDOUT: Polling for CMDACT timed out
   This should not happen unless the controller is dead or in some bad
state

Hi,

Quoted 9 lines · Thinh Nguyen

Quoted 9 lines

Quoted 8 lines · Thinh Nguyen

If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we should properly end an active transfer and give back all the started requests. However if it's for an isoc endpoint, the failure maybe due to bus-expiry status. In this case, don't give back the requests and wait for the next retry.

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")

Signed-off-by Thinh Nguyen <[email protected]>

Quoted 8 lines

Quoted 2 lines

could you give some details regarding when does this happen?

So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return a negative errno:

* -EAGAIN: Isoc bus-expiry status
   As you already know, this occurs when we try to schedule isoc too
late. If we're going to retry the request, don't unmap it.

right

Quoted 3 lines

* -EINVAL: No resource due to issuing START_TRANSFER to an already started endpoint
   This happens generally because of SW programming error

Sounds like this should be fixed separately and, probably, we should add a WARN() so we catch these situations. Have you reproduced this particular case?

Quoted 18 lines

Quoted 9 lines · Thinh Nguyen

Quoted 9 lines

Quoted 8 lines · Thinh Nguyen

If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we should properly end an active transfer and give back all the started requests. However if it's for an isoc endpoint, the failure maybe due to bus-expiry status. In this case, don't give back the requests and wait for the next retry.

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")

Signed-off-by Thinh Nguyen <[email protected]>

Quoted 8 lines

Quoted 2 lines

could you give some details regarding when does this happen?

So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return a negative errno:

* -EAGAIN: Isoc bus-expiry status
   As you already know, this occurs when we try to schedule isoc too
late. If we're going to retry the request, don't unmap it.

Quoted 8 lines

right

Quoted 3 lines

* -EINVAL: No resource due to issuing START_TRANSFER to an already started endpoint
   This happens generally because of SW programming error

Sounds like this should be fixed separately and, probably, we should add a WARN() so we catch these situations. Have you reproduced this particular case?

There are a couple of examples of no resource issue that I recall:
1) When we removed the DWC3_EP_END_TRANSFER_PENDING flag, we wasn't able to check if the endpoint had ended. So, if the function driver queues a new request while END_TRANSFER command is in progress, it'd trigger START_TRANSFER to an already started endpoint

2) For this new method of retrying for isoc, when we issue END_TRANSFER command, for some controller versions, the controller would generate XferNotReady event while the END_TRANSFER command is in progress plus another after it completed. That means we'd start on the same endpoint twice and trigger a no-resource error. (We'd run into this scenario because END_TRANSFER completion cleared the started flag).

We added the checks to prevent this issue from happening, so this scenario should not happen again.

If we want to add a WARN(), I think we should add that inside of dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also just look at the tracepoint for "no resource" status.

Hi,

Quoted 26 lines · Thinh Nguyen

Quoted 26 lines

Quoted 9 lines · Thinh Nguyen

Quoted 9 lines

Quoted 8 lines · Thinh Nguyen

If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we should properly end an active transfer and give back all the started requests. However if it's for an isoc endpoint, the failure maybe due to bus-expiry status. In this case, don't give back the requests and wait for the next retry.

Fixes 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")

Signed-off-by Thinh Nguyen<[email protected]>

Quoted 8 lines

Quoted 2 lines

could you give some details regarding when does this happen?

So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return a negative errno:

* -EAGAIN: Isoc bus-expiry status
   As you already know, this occurs when we try to schedule isoc too
late. If we're going to retry the request, don't unmap it.

right

Quoted 3 lines

* -EINVAL: No resource due to issuing START_TRANSFER to an already started endpoint
   This happens generally because of SW programming error

Sounds like this should be fixed separately and, probably, we should add a WARN() so we catch these situations. Have you reproduced this particular case?

Quoted 5 lines

There are a couple of examples of no resource issue that I recall:
1) When we removed the DWC3_EP_END_TRANSFER_PENDING flag, we wasn't able to check if the endpoint had ended. So, if the function driver queues a new request while END_TRANSFER command is in progress, it'd trigger START_TRANSFER to an already started endpoint

Okay, sounds like this deserves a patch of its own

Quoted 9 lines

2) For this new method of retrying for isoc, when we issue END_TRANSFER command, for some controller versions, the controller would generate XferNotReady event while the END_TRANSFER command is in progress plus another after it completed. That means we'd start on the same endpoint twice and trigger a no-resource error. (We'd run into this scenario because END_TRANSFER completion cleared the started flag).

We added the checks to prevent this issue from happening, so this scenario should not happen again.

Cool, thanks

Quoted 3 lines

If we want to add a WARN(), I think we should add that inside of dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also just look at the tracepoint for "no resource" status.

The "no resource" status is important, sure. But users don't usually run with tracepoints enabled. They'll have a non-working USB port and forget about it. If there's a WARN() triggered, we are more likely to get bug reports.

Quoted 8 lines

Quoted 3 lines

If we want to add a WARN(), I think we should add that inside of dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also just look at the tracepoint for "no resource" status.

The "no resource" status is important, sure. But users don't usually run with tracepoints enabled. They'll have a non-working USB port and forget about it. If there's a WARN() triggered, we are more likely to get bug reports.

Understood. We can add a WARN() to dwc3_send_gadget_ep_cmd() in a separate patch.

Hi,

Quoted 8 lines · Thinh Nguyen

Quoted 8 lines

Quoted 3 lines

If we want to add a WARN(), I think we should add that inside of dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also just look at the tracepoint for "no resource" status.

The "no resource" status is important, sure. But users don't usually run with tracepoints enabled. They'll have a non-working USB port and forget about it. If there's a WARN() triggered, we are more likely to get bug reports.

Quoted 2 lines

Understood. We can add a WARN() to dwc3_send_gadget_ep_cmd() in a separate patch.

I would prefer to see the WARN() patch in the same series, at least. Care to resend with that?