usb: dwc3: gadget: Don't skip updating remaining data

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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.

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)) {
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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;
         ^^^^^^^^^^^^^^^^^^^^^^^

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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;
                                                    ^^^^^^^^^^^^^^

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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. :)