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

[Felipe Balbi <balbi@kernel.org>]

Hi,

First of all, not receiving your mails through the mailing list. Can you
check that you're not
being blocked for some reason? You may wanna send trace data compressed
with gzip or xz.

On Wed, Aug 1, 2018 at 2:27 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com>
wrote:

> On 7/30/2018 11:58 PM, Felipe Balbi wrote:
> > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >> Hi Felipe,
> >>
> >> On 7/29/2018 11:08 PM, Felipe Balbi wrote:
> >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >>>> DWC3 must check for the BUFSIZ and update the req->remaining
> >>>> regardless of transfer alignment. Returning early from transfer OUT
> >>>> unalignment will skip updating the req->remaining.
> >>>>
> >>>> Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to
> wMaxPacketSize")
> >>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >>>> ---
> >>>>  drivers/usb/dwc3/gadget.c | 6 +++---
> >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >>>> index 032ea7d709ba..a5b8387a37ba 100644
> >>>> --- a/drivers/usb/dwc3/gadget.c
> >>>> +++ b/drivers/usb/dwc3/gadget.c
> >>>> @@ -2246,6 +2246,9 @@ static int
> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> >>>>    if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
> >>>>            trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> >>>>
> >>>> +  count = trb->size & DWC3_TRB_SIZE_MASK;
> >>>> +  req->remaining += count;
> >>>> +
> >>>>    /*
> >>>>     * 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
> >>>> @@ -2256,9 +2259,6 @@ static int
> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> >>>>            return 1;
> >>>>    }
> >>>>
> >>>> -  count = trb->size & DWC3_TRB_SIZE_MASK;
> >>>> -  req->remaining += count;
> >>>> -
> >>> this shouldn't be necessary, actually. The remaining TRB which was
> added
> >>> to fix alignment issues, is simply because DWC3 doesn't like OUT
> >>> transfers whose size are not aligned to wMaxPacketSize. We *know* we
> >>> have e.g. 511 bytes to receive from host, but dwc3 wants me to write
> >>> 1024 bytes on the TRB (for superspeed), so I need to add this chained
> >>> TRB for the remaining 513 bytes.
> >>>
> >>> I don't expect host to send us more than 511 bytes, if it does, the
> host
> >>> is faulty and I don't want to operate on those bytes anyway.
> >>>
> >> Right. But that's not the issue here. I realized that I did not describe
> >> the issue fully...
> >>
> >> The issue here is the reporting of the actual number of bytes
> >> transferred. For isoc OUT transfers, the last TRB of the Buffer
> >> Descriptor will be retired with HWO = 0 (dwc_usb3 and dwc_usb31
> >> programming guide 4.3.7). So, the check for unaligned && HWO will be
> >> false and will not return early. As a result, req->remaining will still
> >> be updated with the BUFSIZ count, and the remaining will be 513+ in your
> >> example. The 'actual' bytes written calculation will be wrong (e.g 511 -
> >> 513 = -2).
> >>
> >> We can't rely on the current check to determine whether it's the last
> >> TRB. My current solution is to update req->remaining for all TRB
> >> completions. However, we have to also update the 'actual' bytes
> >> calculation for OUT transfers.
> > I think the bug here is another one. Look at the TRB types:
> >
> >      irq/16-dwc3-2463  [003] d...  3589.933478: dwc3_complete_trb:
> ep1out: trb 000000007d3fbc0d buf 00000000b8ed3800 size 0 ctrl 2e46446c
> (hlCS:Sc:isoc-first)

>      irq/16-dwc3-2463  [003] d...  3589.933492: dwc3_complete_trb:
> ep1out: trb 00000000d1fcb0c4 buf 000000003787e000 size 1023 ctrl 2e464c68
> (hlcS:SC:isoc-first)

>
> > Shouldn't the one be of type ISOC instead of ISOC_FIRST? Can you test
> > patch below and see if things behave better? Please also capture
> > tracepoints :)
> >
> > modified   drivers/usb/dwc3/gadget.c
> > @@ -1073,7 +1073,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep
> *dep,
> >                       /* Now prepare one extra TRB to align transfer
> size */
> >                       trb = &dep->trb_pool[dep->trb_enqueue];
> >                       __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr,
> > -                                     maxp - rem, false, 0,
> > +                                     maxp - rem, false, 1,
> >                                       req->request.stream_id,
> >                                       req->request.short_not_ok,
> >                                       req->request.no_interrupt);
> > @@ -1117,7 +1117,7 @@ static void dwc3_prepare_one_trb_linear(struct
> dwc3_ep *dep,
> >               /* Now prepare one extra TRB to align transfer size */
> >               trb = &dep->trb_pool[dep->trb_enqueue];
> >               __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp -
> rem,
> > -                             false, 0, req->request.stream_id,
> > +                             false, 1, req->request.stream_id,
> >                               req->request.short_not_ok,
> >                               req->request.no_interrupt);
> >       } else if (req->request.zero && req->request.length &&
> >
> >
>
> 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.


> Are you going to make a patch for this one?
>

Yes, I'll prepare an official patch.

cheers