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?
thanks
-john
