Hi,
Felipe Balbi <balbi@xxxxxxxxxx> 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:
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;
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
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
