Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>>>>
>>>> 1. When a TRB receives whose CSP bit is 0 and CHN bit is 1 receives a
>>>> short packet, the chained TRBs that follow it are not written back
>>>> (e.g. the BUFSIZ and HWO fields remain the same as the
>>>> software-prepared value)
>>>>
>>>> 2. In the case of an OUT endpoint, if the CHN bit is set (and CSP is
>>>> also set), and a short packet is received, the core retires the TRB in
>>>> progress and skip past the TRB where CHN=0, accumulating the ISP and
>>>> IOC bits from each TRB. If ISP or IOC is set in any TRB, the core
>>>> generates an XferInProgress event. Hardware does not set the HWO bit
>>>> to 0 in skipped TRBs. If the endpoint type is isochronous, the CHN=0
>>>> TRB will also be retired and its buffer size field updated with the
>>>> total number of bytes remaining in the BD.
>>>>
>>>> Note that at most we have confirmation that SIZE will be updated in
>>>> case of Isoc endpoints, but there's nothing there about HWO, so I
>>>> expected it to remain set according to the rest of the text.
>>>>
>>>> There's one possibility that "TRB will also be *retired*" means that
>>>> it's not skipped, which would mean that HWO is, indeed, set to 0. To
>
> Right. And the programming guide also says that for isoc OUT transfer,
> - Any non-first TRBs with CHN=1 that had not already been retired will
> not be written back. HWO will still be 1, and software can reclaim them
> for another transfer.
> - The last TRB of the Buffer Descriptor will be retired with:
> * HWO = 0.
> * BUFSIZ = The total remaining buffer size of the Buffer Descriptor.
>
> So we know that the last isoc TRB will have CHN=0 and HWO=0.
yeah, only now I understood the actual problem. req->length is 1, so
reading BUFSIZ from last TRB will, actually, cause the problem.
> static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> struct dwc3_request *req, struct dwc3_trb *trb,
> const struct dwc3_event_depevt *event, int status, int chain)
> {
> unsigned int count;
>
> dwc3_ep_inc_deq(dep);
>
> trace_dwc3_complete_trb(dep, trb);
>
> /*
> * If we're in the middle of series of chained TRBs and we
> * receive a short transfer along the way, DWC3 will skip
> * through all TRBs including the last TRB in the chain (the
> * where CHN bit is zero. DWC3 will also avoid clearing HWO
> * bit and SW has to do it manually.
> *
> * We're going to do that here to avoid problems of HW trying
> * to use bogus TRBs for transfers.
> */
> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>
> /*
> * 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
> * from that TRB.
> */
> if ((req->zero || req->unaligned) && (trb->ctrl & DWC3_TRB_CTRL_HWO)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>>>>>>>
> This tries to check for the last TRB and return early. This works in normal
> cases and it stops incrementing the req->remaining. But for isoc OUT
> transfers,
> this case won't hit due to reason mention previously.
> <<<<<<<<<<
>
> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> return 1;
> }
>
> count = trb->size & DWC3_TRB_SIZE_MASK;
> req->remaining += count;
> ^^^^^^^^^^^^^^^^^^^^^^^
>>>>>>>>>>>
> So, your proposals will still update the req->remaining with the BUFSIZ
> count of
> the last TRB.
> <<<<<<<<<<
>
>
> if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
> return 1;
>
> if (event->status & DEPEVT_STATUS_SHORT && !chain)
> return 1;
>
> if (event->status & DEPEVT_STATUS_IOC)
> return 1;
>
> return 0;
> }
>
>
>
> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
> const struct dwc3_event_depevt *event,
> struct dwc3_request *req, int status)
> {
> ....
> req->request.actual = req->request.length - req->remaining;
> ^^^^^^^^^^^^^^
>>>>>>>>>>>
> And the calculation is off because req->remaining includes the remaining
> of the
> last TRB BUFSIZ count.
> <<<<<<<<<<
> ....
> }
>
> I included the traces. Hopefully it will go out to the mailing list.
> Thanks for making the patches, and of course I will test them. :)
I'll read your traces shortly and reply again.
--
balbi
Attachment:
signature.asc
Description: PGP signature
