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

[Felipe Balbi <balbi@kernel.org>]

Felipe Balbi <balbi@kernel.org> writes:

> Hi,
>
> Felipe Balbi <balbi@kernel.org> writes:
>
> <snip>
>
>>> 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.
>>
>>
>>>      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:

and here's another possibility. This makes it clearer that we want to
skip all TRBs with CHN bit set:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9ba614032d5d..56e2a2ebae66 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);
@@ -2256,6 +2257,16 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
                return 1;
        }
 
+       /*
+        * 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)
+                       && chain)
+               return 0;
+
        count = trb->size & DWC3_TRB_SIZE_MASK;
        req->remaining += count;