Thinh Nguyen <thinh.ngu...@synopsys.com> writes:
> Hi Felipe,
>
> On 7/29/2018 11:08 PM, Felipe Balbi wrote:
>> Thinh Nguyen <thinh.ngu...@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 <thi...@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 &&
--
balbi
signature.asc
Description: PGP signature
