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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi Felipe,


On 8/1/2018 1:33 AM, Felipe Balbi wrote:
> 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:

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