Hi Felipe,
On 8/1/2018 1:33 AM, Felipe Balbi wrote:
> Felipe Balbi <balbi@xxxxxxxxxx> writes:
>
>> 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:
My proposal doesn't change how we handle the TRB's HWO currently.
>>>
>>> 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.
>>> 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;
>
These patches will not fix the issue.
Let's take a look at the problem again.
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. :)
Thinh
Attachment:
ktraces_isoc_20180801.tar.xz
Description: ktraces_isoc_20180801.tar.xz
