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

[Felipe Balbi <balbi@kernel.org>]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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.