Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[Felipe Balbi]

Hi,

John Stultz <john.stultz@xxxxxxxxxx> writes:
> On Thu, Jul 18, 2019 at 6:12 PM Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> wrote:
>> fei.yang@xxxxxxxxx wrote:
>> > From: Fei Yang <fei.yang@xxxxxxxxx>
>> >
>> > If scatter-gather operation is allowed, a large USB request is split into
>> > multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
>> > except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
>> > Since only the last TRB has IOC set for the whole USB request, the
>> > dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
>> > completes and all the TRBs allocated for this request are supposed to be
>> > reclaimed. However that is not what the current code does.
>> >
>> > dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
>> > following for-loop,
>> >       for_each_sg(sg, s, pending, i) {
>> >               trb = &dep->trb_pool[dep->trb_dequeue];
>> >
>> >                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>> >                         break;
>> >
>> >                 req->sg = sg_next(s);
>> >                 req->num_pending_sgs--;
>> >
>> >                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>> >                                 trb, event, status, chain);
>> >                 if (ret)
>> >                         break;
>> >         }
>> > but since the interrupt comes only after the last TRB completes, the
>> > event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
>> > the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
>> >       if (event->status & DEPEVT_STATUS_IOC)
>> >               return 1;
>> >
>> > This patch addresses the issue by checking each TRB in function
>> > dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
>> > reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
>> > last TRB.
>> >
>> > Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
>> > Cc: stable <stable@xxxxxxxxxxxxxxx>
>> > ---
>> > v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
>> >     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
>> > v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
>> >     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
>> > ---
>> >  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>> >  1 file changed, 8 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 173f532..88eed49 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>> >       if (event->status & DEPEVT_STATUS_SHORT && !chain)
>> >               return 1;
>> >
>> > -     if (event->status & DEPEVT_STATUS_IOC)
>> > +     if (event->status & DEPEVT_STATUS_IOC && !chain)
>> >               return 1;
>> >
>> >       return 0;
>> > @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>> >               struct dwc3_request *req, const struct dwc3_event_depevt *event,
>> >               int status)
>> >  {
>> > -     struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
>> > +     struct dwc3_trb *trb;
>> >       struct scatterlist *sg = req->sg;
>> >       struct scatterlist *s;
>> >       unsigned int pending = req->num_pending_sgs;
>> >       unsigned int i;
>> > +     int chain = false;
>> >       int ret = 0;
>> >
>> >       for_each_sg(sg, s, pending, i) {
>> > @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>> >
>> >               req->sg = sg_next(s);
>> >               req->num_pending_sgs--;
>> > +             if (trb->ctrl & DWC3_TRB_CTRL_CHN)
>> > +                     chain = true;
>> > +             else
>> > +                     chain = false;
>> >
>> >               ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>> > -                             trb, event, status, true);
>> > +                             trb, event, status, chain);
>> >               if (ret)
>> >                       break;
>> >       }
>>
>> There was already a fix a long time ago by Anurag. But it never made it
>> to the kernel mainline. You can check this out:
>> https://patchwork.kernel.org/patch/10640137/
>
> So, back from a vacation last week, and just validated that both Fei's
> patch and a forward ported version of this patch Thinh pointed out
> both seem to resolve the usb stalls I've been seeing sinice 4.20 w/
> dwc3 hardware on both hikey960 and dragonboard 845c.
>
> Felipe: Does Anurag's patch above make more sense as a proper fix?

I think it's enough to check only the TRB. We won't get events for bits
we didn't enable on the TRB. The only problem here is when we get IOC
event for multiple TRBs where only the last one has IOC.

So, instead of checking:

	if (event->status & IOC && trb->ctrl & IOC)

It's probably enough to check:

	if (tbc->ctrl & IOC)

Could you check that?

Cheers

-- 
balbi

Attachment: signature.asc
Description: PGP signature