[PATCH 0/6] usb: dwc3: gadget: Misc transfer cancellation fixes

[PATCH 0/6] usb: dwc3: gadget: Misc transfer cancellation fixes

[Thinh Nguyen]

This series contains miscellaneous fixes to dwc3's gadget. Most of them are
related to transfer cancellation handling.


Thinh Nguyen (6):
  usb: dwc3: gadget: Don't clear flags before transfer ended
  usb: dwc3: gadget: Properly handle ClearFeature(halt)
  usb: dwc3: gadget: Wrap around when skip TRBs
  usb: dwc3: gadget: Give back staled requests
  usb: dwc3: gadget: Remove unnecessary checks
  usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

 drivers/usb/dwc3/gadget.c | 85 +++++++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 28 deletions(-)

-- 
2.11.0

[PATCH 1/6] usb: dwc3: gadget: Don't clear flags before transfer ended

[Thinh Nguyen]

We track END_TRANSFER command completion. Don't clear transfer
started/ended flag prematurely. Otherwise, we'd run into the problem
with restarting transfer before END_TRANSFER command finishes.

Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1b7d2f9cb673..b87bdec84210 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2563,10 +2563,8 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep,
 
 	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
-	if (stop) {
+	if (stop)
 		dwc3_stop_active_transfer(dep, true, true);
-		dep->flags = DWC3_EP_ENABLED;
-	}
 
 	/*
 	 * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround.
-- 
2.11.0

[PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

[Thinh Nguyen]

DWC3 must not issue CLEAR_STALL command to control endpoints. The
controller automatically clears the STALL when it receives the SETUP
token. Also, when the driver receives ClearFeature(halt_ep), DWC3 must
stop any active transfer from the endpoint and give back all the
requests to the function drivers.

Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b87bdec84210..21b10364b888 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1501,6 +1501,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r
 {
 	int i;
 
+	/* If req->trb is not set, then the request has not started */
+	if (!req->trb)
+		return;
+
 	/*
 	 * If request was already started, this means we had to
 	 * stop the transfer. With that we also need to ignore
@@ -1591,6 +1595,8 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 {
 	struct dwc3_gadget_ep_cmd_params	params;
 	struct dwc3				*dwc = dep->dwc;
+	struct dwc3_request			*req;
+	struct dwc3_request			*tmp;
 	int					ret;
 
 	if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
@@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
 		else
 			dep->flags |= DWC3_EP_STALL;
 	} else {
+		/*
+		 * Don't issue CLEAR_STALL command to control endpoints. The
+		 * controller automatically clears the STALL when it receives
+		 * the SETUP token.
+		 */
+		if (dep->number <= 1) {
+			dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
+			return 0;
+		}
 
 		ret = dwc3_send_clear_stall_ep_cmd(dep);
-		if (ret)
+		if (ret) {
 			dev_err(dwc->dev, "failed to clear STALL on %s\n",
 					dep->name);
-		else
-			dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
+			return ret;
+		}
+
+		dep->flags &= ~(DWC3_EP_STALL | DWC3_EP_WEDGE);
+
+		dwc3_stop_active_transfer(dep, true, true);
+
+		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
+			dwc3_gadget_move_cancelled_request(req);
+
+		list_for_each_entry_safe(req, tmp, &dep->pending_list, list)
+			dwc3_gadget_move_cancelled_request(req);
+
+		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) {
+			dep->flags &= ~DWC3_EP_DELAY_START;
+			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
+		}
 	}
 
 	return ret;
-- 
2.11.0

[PATCH 3/6] usb: dwc3: gadget: Wrap around when skip TRBs

[Thinh Nguyen]

When skipping TRBs, we need to account for wrapping around the ring
buffer and not modifying some invalid TRBs. Without this fix, dwc3 won't
be able to check for available TRBs.

Cc: stable <stable@vger.kernel.org>
Fixes: 7746a8dfb3f9 ("usb: dwc3: gadget: extract dwc3_gadget_ep_skip_trbs()")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 21b10364b888..8c27c6ede7c4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1518,7 +1518,7 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_ep *dep, struct dwc3_request *r
 	for (i = 0; i < req->num_trbs; i++) {
 		struct dwc3_trb *trb;
 
-		trb = req->trb + i;
+		trb = &dep->trb_pool[dep->trb_dequeue];
 		trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
 		dwc3_ep_inc_deq(dep);
 	}
-- 
2.11.0

[PATCH 4/6] usb: dwc3: gadget: Give back staled requests

[Thinh Nguyen]

If a request is dequeued, the transfer is cancelled. Give back all
the started requests.

In most scenarios, the function driver dequeues all requests of a
transfer when there's a failure. If the function driver follows this,
then it's fine. If not, then we'd be skipping TRBs at different points
within the dequeue and enqueue pointers, making dequeue/enqueue pointers
useless. To enforce and make sure that we're properly skipping TRBs,
cancel all the started requests and give back all the cancelled requests
to the function drivers.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8c27c6ede7c4..42dc4973a997 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1553,6 +1553,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	list_for_each_entry(r, &dep->cancelled_list, list) {
+		if (r == req)
+			goto out0;
+	}
+
 	list_for_each_entry(r, &dep->pending_list, list) {
 		if (r == req)
 			break;
@@ -1564,13 +1569,21 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 				break;
 		}
 		if (r == req) {
+			struct dwc3_request *t;
+
 			/* wait until it is processed */
 			dwc3_stop_active_transfer(dep, true, true);
 
 			if (!r->trb)
 				goto out0;
 
-			dwc3_gadget_move_cancelled_request(req);
+			/*
+			 * Remove any started request if the transfer is
+			 * cancelled.
+			 */
+			list_for_each_entry_safe(r, t, &dep->started_list, list)
+				dwc3_gadget_move_cancelled_request(r);
+
 			if (dep->flags & DWC3_EP_TRANSFER_STARTED)
 				goto out0;
 			else
-- 
2.11.0

[PATCH 5/6] usb: dwc3: gadget: Remove unnecessary checks

[Thinh Nguyen]

Remove 2 unnecessary checks:
1) A request in the started_list must have its trb field set. So
checking for req->trb is unnecessary.

2) An endpoint must have started (and have not ended) for the request to
still be in the started_list. There's no point to check if the endpoint
is started. We had this check because previously the driver didn't
handle the endpoint's started/ended flags for END_TRANSFER command
properly. See commit 9f45581f5eec ("usb: dwc3: gadget: early giveback
if End Transfer already completed").

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 42dc4973a997..cd00f2757cb5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1574,9 +1574,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			/* wait until it is processed */
 			dwc3_stop_active_transfer(dep, true, true);
 
-			if (!r->trb)
-				goto out0;
-
 			/*
 			 * Remove any started request if the transfer is
 			 * cancelled.
@@ -1584,10 +1581,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			list_for_each_entry_safe(r, t, &dep->started_list, list)
 				dwc3_gadget_move_cancelled_request(r);
 
-			if (dep->flags & DWC3_EP_TRANSFER_STARTED)
-				goto out0;
-			else
-				goto out1;
+			goto out0;
 		}
 		dev_err(dwc->dev, "request %pK was not queued to %s\n",
 				request, ep->name);
@@ -1595,7 +1589,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 		goto out0;
 	}
 
-out1:
 	dwc3_gadget_giveback(dep, req, -ECONNRESET);
 
 out0:
-- 
2.11.0

[PATCH 6/6] usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

[Thinh Nguyen]

The flow from function dwc3_gadget_ep_dequeue() is not easy to follow.
Refactor it for easier read. No functional change in this commit.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index cd00f2757cb5..39c92df6e188 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1555,19 +1555,17 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	list_for_each_entry(r, &dep->cancelled_list, list) {
 		if (r == req)
-			goto out0;
+			goto out;
 	}
 
 	list_for_each_entry(r, &dep->pending_list, list) {
-		if (r == req)
-			break;
+		if (r == req) {
+			dwc3_gadget_giveback(dep, req, -ECONNRESET);
+			goto out;
+		}
 	}
 
-	if (r != req) {
-		list_for_each_entry(r, &dep->started_list, list) {
-			if (r == req)
-				break;
-		}
+	list_for_each_entry(r, &dep->started_list, list) {
 		if (r == req) {
 			struct dwc3_request *t;
 
@@ -1581,17 +1579,14 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			list_for_each_entry_safe(r, t, &dep->started_list, list)
 				dwc3_gadget_move_cancelled_request(r);
 
-			goto out0;
+			goto out;
 		}
-		dev_err(dwc->dev, "request %pK was not queued to %s\n",
-				request, ep->name);
-		ret = -EINVAL;
-		goto out0;
 	}
 
-	dwc3_gadget_giveback(dep, req, -ECONNRESET);
-
-out0:
+	dev_err(dwc->dev, "request %pK was not queued to %s\n",
+		request, ep->name);
+	ret = -EINVAL;
+out:
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return ret;
-- 
2.11.0

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>  		else
>  			dep->flags |= DWC3_EP_STALL;
>  	} else {
> +		/*
> +		 * Don't issue CLEAR_STALL command to control endpoints. The
> +		 * controller automatically clears the STALL when it receives
> +		 * the SETUP token.
> +		 */

we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was
this triggered?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> If a request is dequeued, the transfer is cancelled. Give back all
> the started requests.
>
> In most scenarios, the function driver dequeues all requests of a
> transfer when there's a failure. If the function driver follows this,
> then it's fine. If not, then we'd be skipping TRBs at different points
> within the dequeue and enqueue pointers, making dequeue/enqueue pointers
> useless. To enforce and make sure that we're properly skipping TRBs,
> cancel all the started requests and give back all the cancelled requests
> to the function drivers.

Which function driver is *not* cancelling transfers correctly? We can
(and should) be defensive on dwc3, but let's not hide bugs on function
drivers either.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 6/6] usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:

> The flow from function dwc3_gadget_ep_dequeue() is not easy to follow.
> Refactor it for easier read. No functional change in this commit.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>

doesn't apply:

checking file drivers/usb/dwc3/gadget.c
Hunk #1 FAILED at 1555.
Hunk #2 FAILED at 1581.
2 out of 2 hunks FAILED

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>>   		else
>>   			dep->flags |= DWC3_EP_STALL;
>>   	} else {
>> +		/*
>> +		 * Don't issue CLEAR_STALL command to control endpoints. The
>> +		 * controller automatically clears the STALL when it receives
>> +		 * the SETUP token.
>> +		 */
> we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was
> this triggered?
>

I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() 
has the similar name as __dwc3_gadget_ep_set_halt(). However, that 
function only calls dwc3_ep0_stall_and_restart() and not handled through 
SET/CLEAR_FEATURE(halt) request.

If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control 
endpoints, it still goes through __dwc3_gadget_ep_set_halt().

BR,
Thinh

Re: [PATCH 6/6] usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue

[Thinh Nguyen]

Hi Felipe,

Felipe Balbi wrote:
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>
>> The flow from function dwc3_gadget_ep_dequeue() is not easy to follow.
>> Refactor it for easier read. No functional change in this commit.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> doesn't apply:
>
> checking file drivers/usb/dwc3/gadget.c
> Hunk #1 FAILED at 1555.
> Hunk #2 FAILED at 1581.
> 2 out of 2 hunks FAILED
>

You'd need to pick up some dependency patches of the same series to 
avoid conflict.
"usb: dwc3: gadget: Give back staled requests"
"usb: dwc3: gadget: Remove unnecessary checks"

Seeing that you have some comments for one of them, it's probably not 
picked up. I'll resend the patches after review.

Thanks,
Thinh

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

[Thinh Nguyen]

Hi Felipe,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> If a request is dequeued, the transfer is cancelled. Give back all
>> the started requests.
>>
>> In most scenarios, the function driver dequeues all requests of a
>> transfer when there's a failure. If the function driver follows this,
>> then it's fine. If not, then we'd be skipping TRBs at different points
>> within the dequeue and enqueue pointers, making dequeue/enqueue pointers
>> useless. To enforce and make sure that we're properly skipping TRBs,
>> cancel all the started requests and give back all the cancelled requests
>> to the function drivers.
> Which function driver is *not* cancelling transfers correctly? We can
> (and should) be defensive on dwc3, but let's not hide bugs on function
> drivers either.
>

I didn't review all the function drivers for this. I just see a 
potential issue and go for a more defensive approach. What's your 
suggestion?

Thanks,
Thinh

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>>>   		else
>>>   			dep->flags |= DWC3_EP_STALL;
>>>   	} else {
>>> +		/*
>>> +		 * Don't issue CLEAR_STALL command to control endpoints. The
>>> +		 * controller automatically clears the STALL when it receives
>>> +		 * the SETUP token.
>>> +		 */
>> we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was
>> this triggered?
>>
>
> I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt() 
> has the similar name as __dwc3_gadget_ep_set_halt(). However, that 
> function only calls dwc3_ep0_stall_and_restart() and not handled through 
> SET/CLEAR_FEATURE(halt) request.
>
> If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control 
> endpoints, it still goes through __dwc3_gadget_ep_set_halt().

Perhaps that should be fixed, then?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> If a request is dequeued, the transfer is cancelled. Give back all
>>> the started requests.
>>>
>>> In most scenarios, the function driver dequeues all requests of a
>>> transfer when there's a failure. If the function driver follows this,
>>> then it's fine. If not, then we'd be skipping TRBs at different points
>>> within the dequeue and enqueue pointers, making dequeue/enqueue pointers
>>> useless. To enforce and make sure that we're properly skipping TRBs,
>>> cancel all the started requests and give back all the cancelled requests
>>> to the function drivers.
>> Which function driver is *not* cancelling transfers correctly? We can
>> (and should) be defensive on dwc3, but let's not hide bugs on function
>> drivers either.
>>
>
> I didn't review all the function drivers for this. I just see a 
> potential issue and go for a more defensive approach. What's your 
> suggestion?

Fair enough, that's good for my understading of why the patch was
created. Is there a way to add a WARN() or something like that so we
catch erroneous gadget drivers easily? Also, could you check if we need
a documentation update for the gadget API with regards to this finding?

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/6] usb: dwc3: gadget: Properly handle ClearFeature(halt)

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> @@ -1627,13 +1633,37 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
>>>>    		else
>>>>    			dep->flags |= DWC3_EP_STALL;
>>>>    	} else {
>>>> +		/*
>>>> +		 * Don't issue CLEAR_STALL command to control endpoints. The
>>>> +		 * controller automatically clears the STALL when it receives
>>>> +		 * the SETUP token.
>>>> +		 */
>>> we have a separate dwc3_gadget_ep0_set_halt() to handle that. How was
>>> this triggered?
>>>
>> I think it's a bit confusing here that the dwc3_gadget_ep0_set_halt()
>> has the similar name as __dwc3_gadget_ep_set_halt(). However, that
>> function only calls dwc3_ep0_stall_and_restart() and not handled through
>> SET/CLEAR_FEATURE(halt) request.
>>
>> If host issues SET_FEATURE(halt) or CLEAR_FEATURE(halt) to control
>> endpoints, it still goes through __dwc3_gadget_ep_set_halt().
> Perhaps that should be fixed, then?
>

If we want to add a patch to make this clear, we can add a separate 
patch to rename dwc3_gadget_ep0_set_halt() to something along the line 
of dwc3_ep0_stall_and_restart().

Everything else looks fine to me. Let me know if you have any other concern.

Thanks,
Thinh

Re: [PATCH 4/6] usb: dwc3: gadget: Give back staled requests

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> If a request is dequeued, the transfer is cancelled. Give back all
>>>> the started requests.
>>>>
>>>> In most scenarios, the function driver dequeues all requests of a
>>>> transfer when there's a failure. If the function driver follows this,
>>>> then it's fine. If not, then we'd be skipping TRBs at different points
>>>> within the dequeue and enqueue pointers, making dequeue/enqueue pointers
>>>> useless. To enforce and make sure that we're properly skipping TRBs,
>>>> cancel all the started requests and give back all the cancelled requests
>>>> to the function drivers.
>>> Which function driver is *not* cancelling transfers correctly? We can
>>> (and should) be defensive on dwc3, but let's not hide bugs on function
>>> drivers either.
>>>
>> I didn't review all the function drivers for this. I just see a
>> potential issue and go for a more defensive approach. What's your
>> suggestion?
> Fair enough, that's good for my understading of why the patch was
> created. Is there a way to add a WARN() or something like that so we
> catch erroneous gadget drivers easily? Also, could you check if we need
> a documentation update for the gadget API with regards to this finding?
>
> cheers
>

It's a bit difficult to add a WARN() as the function drivers can dequeue 
the requests in any order and any time they like. We may be able to put 
some contraints via documentation so we could do a WARN(), but that is 
also not easy.

For example, in dwc3, the current way we implement bulk transfer is that 
there's no concept of where one transfer starts and ends (at least for 
non-stream transfers). So, the function driver may queue multiple 
transfers but dwc3 only sees a single transfer. Dequeuing one transfer 
in the function driver means dequeuing all for dwc3.

Updating the documentation for this requires a bit of thoughts, and it 
also means we need to review all the different controller drivers. We 
may need to document this, but in the mean time, what do you think of 
this approach for now?

BR,
Thinh