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

Hi,

Felipe Balbi [off-list ref] writes:

<snip>

quoted

This is an issue, but it's not the same one.

     irq/16-dwc3-5032  [003] d...    65.404194: dwc3_complete_trb:
ep1out: trb 00000000890522d5 buf 00000000b8d6d000 size 0 ctrl 3b56446c
(hlCS:Sc:isoc-first)

So this is chained to the next one, that's correct.

quoted

     irq/16-dwc3-5032  [003] d...    65.404209: dwc3_complete_trb:
ep1out: trb 00000000c15f388f buf 0000000037821000 size 1023 ctrl
3b564c78 (hlcS:SC:isoc)

But here, HWO should've been left as is, because of the short packet.
That's what the databook says on the two notes on section 8.2.3 after
table 8-8 of Databook 2.60a:

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
patch that, however, we don't need all the extra checking you have
implemented. I'll try to propose a slightly simpler fix if you're
willing to test it out.

Something like below:


If this minimal fix works, then as a follow-up I'll make a cleanup patch
to make this part of the code slightly better. We're starting to get way
too many conditionals here and they can be cleaned up a little.

I just wanna know if the minimal fix works for you and if it does, we
can mark it for stable and make sure the backport goes as far back as
possible.

cheers

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9ba614032d5d..a8c7271a83b5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c

@@ -2227,6 +2227,7 @@ 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)
 {
+       const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
        unsigned int            count;
 
        dwc3_ep_inc_deq(dep);

@@ -2257,7 +2258,17 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
        }
 
        count = trb->size & DWC3_TRB_SIZE_MASK;
-       req->remaining += count;
+
+       /*
+        * In case of ISOC endpoints and Short or Zero packets, the
+        * last TRB will be retired and its size field will be updated
+        * to contain the full remaining size; meaning req->remaining
+        * will be count from the last TRB in the chain.
+        */
+       if ((req->zero || req->unaligned) && usb_endpoint_xfer_isoc(desc))
+               req->remaining = count;
+       else
+               req->remaining += count;
 
        if ((trb->ctrl & DWC3_TRB_CTRL_HWO) && status != -ESHUTDOWN)
                return 1;