[PATCH] usb: dwc3: gadget: Remove incomplete check

[PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

We only care to resume transfer for SG because the request maybe
partially completed. dwc3_gadget_ep_request_completed() doesn't check
that of a request, at least not fully.

1) It doesn't account for OUT direction.
2) It doesn't account for isoc. For isoc, a request maybe completed with
partial data.

Remove this check.

Fixes: e0c42ce590fe ("usb: dwc3: gadget: simplify IOC handling")
Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/gadget.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 39c92df6e188..9f46c8bc1114 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2506,18 +2506,6 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
 			event, status, false);
 }
 
-static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
-{
-	/*
-	 * For OUT direction, host may send less than the setup
-	 * length. Return true for all OUT requests.
-	 */
-	if (!req->direction)
-		return true;
-
-	return req->request.actual == req->request.length;
-}
-
 static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
 		const struct dwc3_event_depevt *event,
 		struct dwc3_request *req, int status)
@@ -2539,8 +2527,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
 
 	req->request.actual = req->request.length - req->remaining;
 
-	if (!dwc3_gadget_ep_request_completed(req) ||
-			req->num_pending_sgs) {
+	if (req->num_pending_sgs) {
 		__dwc3_gadget_kick_transfer(dep);
 		goto out;
 	}
-- 
2.11.0

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> We only care to resume transfer for SG because the request maybe
> partially completed. dwc3_gadget_ep_request_completed() doesn't check
> that of a request, at least not fully.
>
> 1) It doesn't account for OUT direction.
> 2) It doesn't account for isoc. For isoc, a request maybe completed with
> partial data.

I would rather fix the function for these cases instead of removing it
completely. While at that, also move the req->num_pending_sgs check
inside dwc3_gadget_ep_request_completed()

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

Hi Felipe,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> We only care to resume transfer for SG because the request maybe
>> partially completed. dwc3_gadget_ep_request_completed() doesn't check
>> that of a request, at least not fully.
>>
>> 1) It doesn't account for OUT direction.
>> 2) It doesn't account for isoc. For isoc, a request maybe completed with
>> partial data.
> I would rather fix the function for these cases instead of removing it
> completely. While at that, also move the req->num_pending_sgs check
> inside dwc3_gadget_ep_request_completed()
>

If we want to keep this function, the only thing this function does is 
to check req->num_pending_sgs. We'd only resume the request because 
there are pending TRBs from SG not completed yet. If all the TRBs of a 
request are completed, regardless if all the data are received/sent, we 
don't queue them again. Do you still want to have this function?

BR,
Thinh

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> We only care to resume transfer for SG because the request maybe
>>> partially completed. dwc3_gadget_ep_request_completed() doesn't check
>>> that of a request, at least not fully.
>>>
>>> 1) It doesn't account for OUT direction.
>>> 2) It doesn't account for isoc. For isoc, a request maybe completed with
>>> partial data.
>> I would rather fix the function for these cases instead of removing it
>> completely. While at that, also move the req->num_pending_sgs check
>> inside dwc3_gadget_ep_request_completed()
>>
>
> If we want to keep this function, the only thing this function does is 
> to check req->num_pending_sgs. We'd only resume the request because 
> there are pending TRBs from SG not completed yet. If all the TRBs of a 
> request are completed, regardless if all the data are received/sent, we 
> don't queue them again. Do you still want to have this function?

The function gives a name to a very specific "concept", that of a
completed request. You can see that even today, the function is super
simple: OUT direction is always completed, IN direction is completed
when actual == length, we're just missing the pending_sgs check. So
something like the hunks below.

One thing I don't get from your patch is why you're completely removing
the function and why isn't req->direction and actual == length not
needed anymore. Could you explain?

hunks:

@@ -2482,7 +2482,8 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
 			event, status, false);
 }
 
-static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
+static bool dwc3_gadget_ep_request_completed(struct dwc3_ep *dep,
+		struct dwc3_request *req)
 {
 	/*
 	 * For OUT direction, host may send less than the setup
@@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
 	if (!req->direction)
 		return true;
 
+	/*
+	 * If there are pending scatterlist entries, we should
+	 * continue processing them.
+	 */
+	if (req->num_pending_sgs)
+		return false;
+
+	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		do_something();
+
 	return req->request.actual == req->request.length;
 }
 
@@ -2515,8 +2526,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
 
 	req->request.actual = req->request.length - req->remaining;
 
-	if (!dwc3_gadget_ep_request_completed(req) ||
-			req->num_pending_sgs) {
+	if (!dwc3_gadget_ep_request_completed(dep, req))
 		__dwc3_gadget_kick_transfer(dep);
 		goto out;
 	}

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> We only care to resume transfer for SG because the request maybe
>>>> partially completed. dwc3_gadget_ep_request_completed() doesn't check
>>>> that of a request, at least not fully.
>>>>
>>>> 1) It doesn't account for OUT direction.
>>>> 2) It doesn't account for isoc. For isoc, a request maybe completed with
>>>> partial data.
>>> I would rather fix the function for these cases instead of removing it
>>> completely. While at that, also move the req->num_pending_sgs check
>>> inside dwc3_gadget_ep_request_completed()
>>>
>> If we want to keep this function, the only thing this function does is
>> to check req->num_pending_sgs. We'd only resume the request because
>> there are pending TRBs from SG not completed yet. If all the TRBs of a
>> request are completed, regardless if all the data are received/sent, we
>> don't queue them again. Do you still want to have this function?
> The function gives a name to a very specific "concept", that of a
> completed request. You can see that even today, the function is super
> simple: OUT direction is always completed, IN direction is completed
> when actual == length, we're just missing the pending_sgs check. So
> something like the hunks below.

Fair point.

>
> One thing I don't get from your patch is why you're completely removing
> the function and why isn't req->direction and actual == length not
> needed anymore. Could you explain?

It's because there's no use for that function outside of checking for 
number of pending SGs and resume.

>
> hunks:
>
> @@ -2482,7 +2482,8 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>   			event, status, false);
>   }
>   
> -static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
> +static bool dwc3_gadget_ep_request_completed(struct dwc3_ep *dep,
> +		struct dwc3_request *req)
>   {
>   	/*
>   	 * For OUT direction, host may send less than the setup
> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>   	if (!req->direction)
>   		return true;
>   
> +	/*
> +	 * If there are pending scatterlist entries, we should
> +	 * continue processing them.
> +	 */
> +	if (req->num_pending_sgs)
> +		return false;
> +
> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
> +		do_something();

do_something() will always return true here.

> +
>   	return req->request.actual == req->request.length;

This should always be true if the request completes. By spec, bulk and 
interrupt endpoints data delivery are guaranteed, and the retry/error 
detection is done at the lower level.  If by chance that the host fails 
to request for data multiple times at the packet level, it will issue a 
ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to 
stop the endpoint and cancel the transfer, and we still won't resume 
this transfer.

>   }
>   
> @@ -2515,8 +2526,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>   
>   	req->request.actual = req->request.length - req->remaining;
>   
> -	if (!dwc3_gadget_ep_request_completed(req) ||
> -			req->num_pending_sgs) {
> +	if (!dwc3_gadget_ep_request_completed(dep, req))
>   		__dwc3_gadget_kick_transfer(dep);
>   		goto out;
>   	}
>

BR,
Thinh

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> One thing I don't get from your patch is why you're completely removing
>> the function and why isn't req->direction and actual == length not
>> needed anymore. Could you explain?
>
> It's because there's no use for that function outside of checking for 
> number of pending SGs and resume.

wait, huh? What about cases when user unplugs cable midtransfer? We have
versions of dwc3 HW which fail to produce disconnect interrupt, right?

>> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>>   	if (!req->direction)
>>   		return true;
>>   
>> +	/*
>> +	 * If there are pending scatterlist entries, we should
>> +	 * continue processing them.
>> +	 */
>> +	if (req->num_pending_sgs)
>> +		return false;
>> +
>> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
>> +		do_something();
>
> do_something() will always return true here.

Will do "do_something", then return true or simply return true?

>>   	return req->request.actual == req->request.length;
>
> This should always be true if the request completes. By spec, bulk and 
> interrupt endpoints data delivery are guaranteed, and the retry/error 
> detection is done at the lower level.  If by chance that the host fails 
> to request for data multiple times at the packet level, it will issue a 
> ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to 
> stop the endpoint and cancel the transfer, and we still won't resume 
> this transfer.

we can unplug the cable at any time, even mid-transfer.

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

Hi Felipe,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> One thing I don't get from your patch is why you're completely removing
>>> the function and why isn't req->direction and actual == length not
>>> needed anymore. Could you explain?
>> It's because there's no use for that function outside of checking for
>> number of pending SGs and resume.
> wait, huh? What about cases when user unplugs cable midtransfer? We have
> versions of dwc3 HW which fail to produce disconnect interrupt, right?

There won't even enter this code path if disconnect event is generated 
or not.

>
>>> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>>>    	if (!req->direction)
>>>    		return true;
>>>    
>>> +	/*
>>> +	 * If there are pending scatterlist entries, we should
>>> +	 * continue processing them.
>>> +	 */
>>> +	if (req->num_pending_sgs)
>>> +		return false;
>>> +
>>> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
>>> +		do_something();
>> do_something() will always return true here.
> Will do "do_something", then return true or simply return true?

I mean simply return true here.

>
>>>    	return req->request.actual == req->request.length;
>> This should always be true if the request completes. By spec, bulk and
>> interrupt endpoints data delivery are guaranteed, and the retry/error
>> detection is done at the lower level.  If by chance that the host fails
>> to request for data multiple times at the packet level, it will issue a
>> ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to
>> stop the endpoint and cancel the transfer, and we still won't resume
>> this transfer.
> we can unplug the cable at any time, even mid-transfer.
>

That's fine if there's a disconnection mid-transfer. The transfer is 
cancelled in that case, why would we want to kick_transfer again? Also, 
the controller would not generate XferInProgress event to notify TRB 
completion for the driver to enter this code path.

The condition here is if (!request_complete()), then kick_transfer(). 
Let's take a look at what kick_transfer() do:

kick_transfer() will prepare new TRBs and issue START_TRANSFER command 
or UPDATE_TRANSFER command. The endpoint is already started, and nothing 
is causing it to end at this point. So it should just be UPDATE_TRANSFER 
command. UPDATE_TRANSFER command tells the controller to update its TRB 
cache because there will be new TRBs prepared for the request.

If this is non-SG/non-chained TRB request, then there's only 1 TRB per 
request for IN endpoints. If that TRB is completed, that means that the 
request is completed. There's no reason to issue kick_transfer() again. 
When the function driver queues a new request, then there will be new 
TRBs to prepare and then the driver can kick_transfer() again.

So, this condition to check if request_complete() is only valid for a 
request with multiple chained TRBs. Since we can only check for IN 
direction, the chained TRB setup related to OUT direction to fit 
MaxPacketSize does not apply here. What left is chained TRBs for SG. In 
this case, we do want to kick_transfer again. This may happen when we 
run out of TRBs and we have to wait for available TRBs. When there are 
available TRBs and still pending SGs, then we want to prepare the rest 
of the SG entries to finish the request. So kick_transfer() makes sense 
here.

Let me know if there's something not clear.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> @@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>>>>    	if (!req->direction)
>>>>    		return true;
>>>>    
>>>> +	/*
>>>> +	 * If there are pending scatterlist entries, we should
>>>> +	 * continue processing them.
>>>> +	 */
>>>> +	if (req->num_pending_sgs)
>>>> +		return false;
>>>> +
>>>> +	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
>>>> +		do_something();
>>> do_something() will always return true here.
>> Will do "do_something", then return true or simply return true?
>
> I mean simply return true here.

got it

>>>>    	return req->request.actual == req->request.length;
>>> This should always be true if the request completes. By spec, bulk and
>>> interrupt endpoints data delivery are guaranteed, and the retry/error
>>> detection is done at the lower level.  If by chance that the host fails
>>> to request for data multiple times at the packet level, it will issue a
>>> ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to
>>> stop the endpoint and cancel the transfer, and we still won't resume
>>> this transfer.
>> we can unplug the cable at any time, even mid-transfer.
>
> That's fine if there's a disconnection mid-transfer. The transfer is 
> cancelled in that case, why would we want to kick_transfer again? Also, 
> the controller would not generate XferInProgress event to notify TRB 
> completion for the driver to enter this code path.

d'oh! That's true

> The condition here is if (!request_complete()), then kick_transfer(). 
> Let's take a look at what kick_transfer() do:
>
> kick_transfer() will prepare new TRBs and issue START_TRANSFER command 
> or UPDATE_TRANSFER command. The endpoint is already started, and nothing 
> is causing it to end at this point. So it should just be UPDATE_TRANSFER 
> command. UPDATE_TRANSFER command tells the controller to update its TRB 
> cache because there will be new TRBs prepared for the request.
>
> If this is non-SG/non-chained TRB request, then there's only 1 TRB per 
> request for IN endpoints. If that TRB is completed, that means that the 
> request is completed. There's no reason to issue kick_transfer() again. 

not entirely true for bulk. We never set LST bit; we will never complete
a transfer, we continually add more TRBs. The reason for this is to
amortize the cost of adding new transfers to the controller cache before
it runs out of TRBs without HWO.

How about we change the test to say "if I have non-started TRBs and I'm
bulk (non-stream) or interrupt endpoint, kick more transfers"?

> When the function driver queues a new request, then there will be new 
> TRBs to prepare and then the driver can kick_transfer() again.

We may already have more TRBs in the pending list which may not have
been started before we didn't have free TRBs to use. We just completed a
TRB, might as well try to use it for more requests.

> So, this condition to check if request_complete() is only valid for a 
> request with multiple chained TRBs. Since we can only check for IN 
> direction, the chained TRB setup related to OUT direction to fit 
> MaxPacketSize does not apply here. What left is chained TRBs for SG. In 

this part is clear now and you're correct. Thanks

> this case, we do want to kick_transfer again. This may happen when we 
> run out of TRBs and we have to wait for available TRBs. When there are 
> available TRBs and still pending SGs, then we want to prepare the rest 
> of the SG entries to finish the request. So kick_transfer() makes sense 
> here.

Right but we can run out of TRBs even in non-chained case. I remember
testing this years ago by giving g_mass_storage a list of 300
requests. The reason for kicking the transfer is different, but it's
beneficial anyhow.

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>
>> The condition here is if (!request_complete()), then kick_transfer().
>> Let's take a look at what kick_transfer() do:
>>
>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command
>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing
>> is causing it to end at this point. So it should just be UPDATE_TRANSFER
>> command. UPDATE_TRANSFER command tells the controller to update its TRB
>> cache because there will be new TRBs prepared for the request.
>>
>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per
>> request for IN endpoints. If that TRB is completed, that means that the
>> request is completed. There's no reason to issue kick_transfer() again.
> not entirely true for bulk. We never set LST bit; we will never complete
> a transfer, we continually add more TRBs. The reason for this is to
> amortize the cost of adding new transfers to the controller cache before
> it runs out of TRBs without HWO.

Right, I was referring to "request" rather than transfer (as in a 
transfer may have 1 or more requests).

>
> How about we change the test to say "if I have non-started TRBs and I'm
> bulk (non-stream) or interrupt endpoint, kick more transfers"?
>
>> When the function driver queues a new request, then there will be new
>> TRBs to prepare and then the driver can kick_transfer() again.
> We may already have more TRBs in the pending list which may not have
> been started before we didn't have free TRBs to use. We just completed a
> TRB, might as well try to use it for more requests.

Yes we can and we should, but we didn't check that. Also it shouldn't be 
in the request_complete() check function as they are part of different 
requests.

>
>> So, this condition to check if request_complete() is only valid for a
>> request with multiple chained TRBs. Since we can only check for IN
>> direction, the chained TRB setup related to OUT direction to fit
>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In
> this part is clear now and you're correct. Thanks
>
>> this case, we do want to kick_transfer again. This may happen when we
>> run out of TRBs and we have to wait for available TRBs. When there are
>> available TRBs and still pending SGs, then we want to prepare the rest
>> of the SG entries to finish the request. So kick_transfer() makes sense
>> here.
> Right but we can run out of TRBs even in non-chained case. I remember
> testing this years ago by giving g_mass_storage a list of 300
> requests. The reason for kicking the transfer is different, but it's
> beneficial anyhow.
>

In this case, the check should be for if the pending_list is not empty, 
then kick again.

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 6a04c9afcab6..d8318de55000 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2975,14 +2975,7 @@ static int 
dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,

  static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
  {
-       /*
-        * For OUT direction, host may send less than the setup
-        * length. Return true for all OUT requests.
-        */
-       if (!req->direction)
-               return true;
-
-       return req->request.actual == req->request.length;
+       return req->num_pending_sgs == 0;
  }

  static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
@@ -3007,7 +3000,7 @@ static int 
dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
         req->request.actual = req->request.length - req->remaining;

         if (!dwc3_gadget_ep_request_completed(req) ||
-                       req->num_pending_sgs) {
+           !list_empty(&dep->pending_list)) {
                 __dwc3_gadget_kick_transfer(dep);
                 goto out;
         }


This is unlikely to happen, but it's necessary to be there.

Let me know if you're ok with the change, I'll create a formal patch for it.

BR,
Thinh

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> The condition here is if (!request_complete()), then kick_transfer().
>>> Let's take a look at what kick_transfer() do:
>>>
>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command
>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing
>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER
>>> command. UPDATE_TRANSFER command tells the controller to update its TRB
>>> cache because there will be new TRBs prepared for the request.
>>>
>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per
>>> request for IN endpoints. If that TRB is completed, that means that the
>>> request is completed. There's no reason to issue kick_transfer() again.
>> not entirely true for bulk. We never set LST bit; we will never complete
>> a transfer, we continually add more TRBs. The reason for this is to
>> amortize the cost of adding new transfers to the controller cache before
>> it runs out of TRBs without HWO.
>
> Right, I was referring to "request" rather than transfer (as in a 
> transfer may have 1 or more requests).
>
>>
>> How about we change the test to say "if I have non-started TRBs and I'm
>> bulk (non-stream) or interrupt endpoint, kick more transfers"?
>>
>>> When the function driver queues a new request, then there will be new
>>> TRBs to prepare and then the driver can kick_transfer() again.
>> We may already have more TRBs in the pending list which may not have
>> been started before we didn't have free TRBs to use. We just completed a
>> TRB, might as well try to use it for more requests.
>
> Yes we can and we should, but we didn't check that. Also it shouldn't be 
> in the request_complete() check function as they are part of different 
> requests.
>
>>
>>> So, this condition to check if request_complete() is only valid for a
>>> request with multiple chained TRBs. Since we can only check for IN
>>> direction, the chained TRB setup related to OUT direction to fit
>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In
>> this part is clear now and you're correct. Thanks
>>
>>> this case, we do want to kick_transfer again. This may happen when we
>>> run out of TRBs and we have to wait for available TRBs. When there are
>>> available TRBs and still pending SGs, then we want to prepare the rest
>>> of the SG entries to finish the request. So kick_transfer() makes sense
>>> here.
>> Right but we can run out of TRBs even in non-chained case. I remember
>> testing this years ago by giving g_mass_storage a list of 300
>> requests. The reason for kicking the transfer is different, but it's
>> beneficial anyhow.
>>
>
> In this case, the check should be for if the pending_list is not empty, 
> then kick again.
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6a04c9afcab6..d8318de55000 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2975,14 +2975,7 @@ static int 
> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>
>   static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>   {
> -       /*
> -        * For OUT direction, host may send less than the setup
> -        * length. Return true for all OUT requests.
> -        */
> -       if (!req->direction)
> -               return true;
> -
> -       return req->request.actual == req->request.length;
> +       return req->num_pending_sgs == 0;
>   }
>
>   static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
> @@ -3007,7 +3000,7 @@ static int 
> dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>          req->request.actual = req->request.length - req->remaining;
>
>          if (!dwc3_gadget_ep_request_completed(req) ||
> -                       req->num_pending_sgs) {
> +           !list_empty(&dep->pending_list)) {
>                  __dwc3_gadget_kick_transfer(dep);
>                  goto out;
>          }
>
>
> This is unlikely to happen, but it's necessary to be there.
>
> Let me know if you're ok with the change, I'll create a formal patch for it.

Looks good, we may just rename the function to
dwc3_gadget_ep_should_continue() or something similar and move the
!list_empty() check in there too.

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> The condition here is if (!request_complete()), then kick_transfer().
>>>> Let's take a look at what kick_transfer() do:
>>>>
>>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command
>>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing
>>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER
>>>> command. UPDATE_TRANSFER command tells the controller to update its TRB
>>>> cache because there will be new TRBs prepared for the request.
>>>>
>>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per
>>>> request for IN endpoints. If that TRB is completed, that means that the
>>>> request is completed. There's no reason to issue kick_transfer() again.
>>> not entirely true for bulk. We never set LST bit; we will never complete
>>> a transfer, we continually add more TRBs. The reason for this is to
>>> amortize the cost of adding new transfers to the controller cache before
>>> it runs out of TRBs without HWO.
>> Right, I was referring to "request" rather than transfer (as in a
>> transfer may have 1 or more requests).
>>
>>> How about we change the test to say "if I have non-started TRBs and I'm
>>> bulk (non-stream) or interrupt endpoint, kick more transfers"?
>>>
>>>> When the function driver queues a new request, then there will be new
>>>> TRBs to prepare and then the driver can kick_transfer() again.
>>> We may already have more TRBs in the pending list which may not have
>>> been started before we didn't have free TRBs to use. We just completed a
>>> TRB, might as well try to use it for more requests.
>> Yes we can and we should, but we didn't check that. Also it shouldn't be
>> in the request_complete() check function as they are part of different
>> requests.
>>
>>>> So, this condition to check if request_complete() is only valid for a
>>>> request with multiple chained TRBs. Since we can only check for IN
>>>> direction, the chained TRB setup related to OUT direction to fit
>>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In
>>> this part is clear now and you're correct. Thanks
>>>
>>>> this case, we do want to kick_transfer again. This may happen when we
>>>> run out of TRBs and we have to wait for available TRBs. When there are
>>>> available TRBs and still pending SGs, then we want to prepare the rest
>>>> of the SG entries to finish the request. So kick_transfer() makes sense
>>>> here.
>>> Right but we can run out of TRBs even in non-chained case. I remember
>>> testing this years ago by giving g_mass_storage a list of 300
>>> requests. The reason for kicking the transfer is different, but it's
>>> beneficial anyhow.
>>>
>> In this case, the check should be for if the pending_list is not empty,
>> then kick again.
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 6a04c9afcab6..d8318de55000 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2975,14 +2975,7 @@ static int
>> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>>
>>    static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>>    {
>> -       /*
>> -        * For OUT direction, host may send less than the setup
>> -        * length. Return true for all OUT requests.
>> -        */
>> -       if (!req->direction)
>> -               return true;
>> -
>> -       return req->request.actual == req->request.length;
>> +       return req->num_pending_sgs == 0;
>>    }
>>
>>    static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>> @@ -3007,7 +3000,7 @@ static int
>> dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>>           req->request.actual = req->request.length - req->remaining;
>>
>>           if (!dwc3_gadget_ep_request_completed(req) ||
>> -                       req->num_pending_sgs) {
>> +           !list_empty(&dep->pending_list)) {
>>                   __dwc3_gadget_kick_transfer(dep);
>>                   goto out;
>>           }
>>
>>
>> This is unlikely to happen, but it's necessary to be there.
>>
>> Let me know if you're ok with the change, I'll create a formal patch for it.
> Looks good, we may just rename the function to
> dwc3_gadget_ep_should_continue() or something similar and move the
> !list_empty() check in there too.
>

I forgot this condition skips the dwc3_gadget_giveback(). I have to 
split it. Let me send out the revised patches and you can review.

Thanks,
Thinh

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> The condition here is if (!request_complete()), then kick_transfer().
>>>>> Let's take a look at what kick_transfer() do:
>>>>>
>>>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command
>>>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing
>>>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER
>>>>> command. UPDATE_TRANSFER command tells the controller to update its TRB
>>>>> cache because there will be new TRBs prepared for the request.
>>>>>
>>>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per
>>>>> request for IN endpoints. If that TRB is completed, that means that the
>>>>> request is completed. There's no reason to issue kick_transfer() again.
>>>> not entirely true for bulk. We never set LST bit; we will never complete
>>>> a transfer, we continually add more TRBs. The reason for this is to
>>>> amortize the cost of adding new transfers to the controller cache before
>>>> it runs out of TRBs without HWO.
>>> Right, I was referring to "request" rather than transfer (as in a
>>> transfer may have 1 or more requests).
>>>
>>>> How about we change the test to say "if I have non-started TRBs and I'm
>>>> bulk (non-stream) or interrupt endpoint, kick more transfers"?
>>>>
>>>>> When the function driver queues a new request, then there will be new
>>>>> TRBs to prepare and then the driver can kick_transfer() again.
>>>> We may already have more TRBs in the pending list which may not have
>>>> been started before we didn't have free TRBs to use. We just completed a
>>>> TRB, might as well try to use it for more requests.
>>> Yes we can and we should, but we didn't check that. Also it shouldn't be
>>> in the request_complete() check function as they are part of different
>>> requests.
>>>
>>>>> So, this condition to check if request_complete() is only valid for a
>>>>> request with multiple chained TRBs. Since we can only check for IN
>>>>> direction, the chained TRB setup related to OUT direction to fit
>>>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In
>>>> this part is clear now and you're correct. Thanks
>>>>
>>>>> this case, we do want to kick_transfer again. This may happen when we
>>>>> run out of TRBs and we have to wait for available TRBs. When there are
>>>>> available TRBs and still pending SGs, then we want to prepare the rest
>>>>> of the SG entries to finish the request. So kick_transfer() makes sense
>>>>> here.
>>>> Right but we can run out of TRBs even in non-chained case. I remember
>>>> testing this years ago by giving g_mass_storage a list of 300
>>>> requests. The reason for kicking the transfer is different, but it's
>>>> beneficial anyhow.
>>>>
>>> In this case, the check should be for if the pending_list is not empty,
>>> then kick again.
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 6a04c9afcab6..d8318de55000 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2975,14 +2975,7 @@ static int
>>> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>>>
>>>    static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>>>    {
>>> -       /*
>>> -        * For OUT direction, host may send less than the setup
>>> -        * length. Return true for all OUT requests.
>>> -        */
>>> -       if (!req->direction)
>>> -               return true;
>>> -
>>> -       return req->request.actual == req->request.length;
>>> +       return req->num_pending_sgs == 0;
>>>    }
>>>
>>>    static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>>> @@ -3007,7 +3000,7 @@ static int
>>> dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>>>           req->request.actual = req->request.length - req->remaining;
>>>
>>>           if (!dwc3_gadget_ep_request_completed(req) ||
>>> -                       req->num_pending_sgs) {
>>> +           !list_empty(&dep->pending_list)) {
>>>                   __dwc3_gadget_kick_transfer(dep);
>>>                   goto out;
>>>           }
>>>
>>>
>>> This is unlikely to happen, but it's necessary to be there.
>>>
>>> Let me know if you're ok with the change, I'll create a formal patch for it.
>> Looks good, we may just rename the function to
>> dwc3_gadget_ep_should_continue() or something similar and move the
>> !list_empty() check in there too.
>>
>
> I forgot this condition skips the dwc3_gadget_giveback(). I have to 
> split it. Let me send out the revised patches and you can review.

Sure, I think patch 1 can go in during -rc. Do we need a Cc stable on
it, though?

Patch 2 will have to wait until v5.8.

-- 
balbi

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

Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Thinh Nguyen]

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>> The condition here is if (!request_complete()), then kick_transfer().
>>>>>> Let's take a look at what kick_transfer() do:
>>>>>>
>>>>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command
>>>>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing
>>>>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER
>>>>>> command. UPDATE_TRANSFER command tells the controller to update its TRB
>>>>>> cache because there will be new TRBs prepared for the request.
>>>>>>
>>>>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per
>>>>>> request for IN endpoints. If that TRB is completed, that means that the
>>>>>> request is completed. There's no reason to issue kick_transfer() again.
>>>>> not entirely true for bulk. We never set LST bit; we will never complete
>>>>> a transfer, we continually add more TRBs. The reason for this is to
>>>>> amortize the cost of adding new transfers to the controller cache before
>>>>> it runs out of TRBs without HWO.
>>>> Right, I was referring to "request" rather than transfer (as in a
>>>> transfer may have 1 or more requests).
>>>>
>>>>> How about we change the test to say "if I have non-started TRBs and I'm
>>>>> bulk (non-stream) or interrupt endpoint, kick more transfers"?
>>>>>
>>>>>> When the function driver queues a new request, then there will be new
>>>>>> TRBs to prepare and then the driver can kick_transfer() again.
>>>>> We may already have more TRBs in the pending list which may not have
>>>>> been started before we didn't have free TRBs to use. We just completed a
>>>>> TRB, might as well try to use it for more requests.
>>>> Yes we can and we should, but we didn't check that. Also it shouldn't be
>>>> in the request_complete() check function as they are part of different
>>>> requests.
>>>>
>>>>>> So, this condition to check if request_complete() is only valid for a
>>>>>> request with multiple chained TRBs. Since we can only check for IN
>>>>>> direction, the chained TRB setup related to OUT direction to fit
>>>>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In
>>>>> this part is clear now and you're correct. Thanks
>>>>>
>>>>>> this case, we do want to kick_transfer again. This may happen when we
>>>>>> run out of TRBs and we have to wait for available TRBs. When there are
>>>>>> available TRBs and still pending SGs, then we want to prepare the rest
>>>>>> of the SG entries to finish the request. So kick_transfer() makes sense
>>>>>> here.
>>>>> Right but we can run out of TRBs even in non-chained case. I remember
>>>>> testing this years ago by giving g_mass_storage a list of 300
>>>>> requests. The reason for kicking the transfer is different, but it's
>>>>> beneficial anyhow.
>>>>>
>>>> In this case, the check should be for if the pending_list is not empty,
>>>> then kick again.
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 6a04c9afcab6..d8318de55000 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -2975,14 +2975,7 @@ static int
>>>> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>>>>
>>>>     static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>>>>     {
>>>> -       /*
>>>> -        * For OUT direction, host may send less than the setup
>>>> -        * length. Return true for all OUT requests.
>>>> -        */
>>>> -       if (!req->direction)
>>>> -               return true;
>>>> -
>>>> -       return req->request.actual == req->request.length;
>>>> +       return req->num_pending_sgs == 0;
>>>>     }
>>>>
>>>>     static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>>>> @@ -3007,7 +3000,7 @@ static int
>>>> dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>>>>            req->request.actual = req->request.length - req->remaining;
>>>>
>>>>            if (!dwc3_gadget_ep_request_completed(req) ||
>>>> -                       req->num_pending_sgs) {
>>>> +           !list_empty(&dep->pending_list)) {
>>>>                    __dwc3_gadget_kick_transfer(dep);
>>>>                    goto out;
>>>>            }
>>>>
>>>>
>>>> This is unlikely to happen, but it's necessary to be there.
>>>>
>>>> Let me know if you're ok with the change, I'll create a formal patch for it.
>>> Looks good, we may just rename the function to
>>> dwc3_gadget_ep_should_continue() or something similar and move the
>>> !list_empty() check in there too.
>>>
>> I forgot this condition skips the dwc3_gadget_giveback(). I have to
>> split it. Let me send out the revised patches and you can review.
> Sure, I think patch 1 can go in during -rc. Do we need a Cc stable on
> it, though?
>
> Patch 2 will have to wait until v5.8.
>

Sure. I'll resend with Cc stable tag.

Thanks,
Thinh