[2/3] usb: dwc3: gadget: Don't skip updating remaining data

[Felipe Balbi <balbi@kernel.org>]

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

> Hi Felipe,
>
> On 7/29/2018 11:08 PM, Felipe Balbi wrote:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> DWC3 must check for the BUFSIZ and update the req->remaining
>>> regardless of transfer alignment. Returning early from transfer OUT
>>> unalignment will skip updating the req->remaining.
>>>
>>> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 032ea7d709ba..a5b8387a37ba 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2246,6 +2246,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>  	if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
>>>  		trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>>>  
>>> +	count = trb->size & DWC3_TRB_SIZE_MASK;
>>> +	req->remaining += count;
>>> +
>>>  	/*
>>>  	 * If we're dealing with unaligned size OUT transfer, we will be left
>>>  	 * with one TRB pending in the ring. We need to manually clear HWO bit
>>> @@ -2256,9 +2259,6 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>  		return 1;
>>>  	}
>>>  
>>> -	count = trb->size & DWC3_TRB_SIZE_MASK;
>>> -	req->remaining += count;
>>> -
>> this shouldn't be necessary, actually. The remaining TRB which was added
>> to fix alignment issues, is simply because DWC3 doesn't like OUT
>> transfers whose size are not aligned to wMaxPacketSize. We *know* we
>> have e.g. 511 bytes to receive from host, but dwc3 wants me to write
>> 1024 bytes on the TRB (for superspeed), so I need to add this chained
>> TRB for the remaining 513 bytes.
>>
>> I don't expect host to send us more than 511 bytes, if it does, the host
>> is faulty and I don't want to operate on those bytes anyway.
>>
>
> Right. But that's not the issue here. I realized that I did not describe
> the issue fully...
>
> The issue here is the reporting of the actual number of bytes
> transferred. For isoc OUT transfers, the last TRB of the Buffer
> Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31
> programming guide 4.3.7). So, the check for unaligned && HWO will be
> false and will not return early. As a result, req->remaining will still
> be updated with the BUFSIZ count, and the remaining will be 513+ in your
> example. The 'actual' bytes written calculation will be wrong (e.g 511 -
> 513 = -2).
>
> We can't rely on the current check to determine whether it's the last
> TRB. My current solution is to update req->remaining for all TRB
> completions. However, we have to also update the 'actual' bytes
> calculation for OUT transfers.

I think the bug here is another one. Look at the TRB types:

     irq/16-dwc3-2463  [003] d...  3589.933478: dwc3_complete_trb: ep1out: trb 000000007d3fbc0d buf 00000000b8ed3800 size 0 ctrl 2e46446c (hlCS:Sc:isoc-first)
     irq/16-dwc3-2463  [003] d...  3589.933492: dwc3_complete_trb: ep1out: trb 00000000d1fcb0c4 buf 000000003787e000 size 1023 ctrl 2e464c68 (hlcS:SC:isoc-first)

Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test
patch below and see if things behave better? Please also capture
tracepoints :)

modified   drivers/usb/dwc3/gadget.c
@@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
 			/* Now prepare one extra TRB to align transfer size */
 			trb = &dep->trb_pool[dep->trb_enqueue];
 			__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
-					maxp - rem, false, 0,
+					maxp - rem, false, 1,
 					req->request.stream_id,
 					req->request.short_not_ok,
 					req->request.no_interrupt);
@@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep,
 		/* Now prepare one extra TRB to align transfer size */
 		trb = &dep->trb_pool[dep->trb_enqueue];
 		__dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem,
-				false, 0, req->request.stream_id,
+				false, 1, req->request.stream_id,
 				req->request.short_not_ok,
 				req->request.no_interrupt);
 	} else if (req->request.zero && req->request.length &&