Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1563497183-7114-1-git-send-email-fei.yang@intel.com> <CY4PR1201MB003708ADAD79BF4FD24D3445AACB0@CY4PR1201MB0037.namprd12.prod.outlook.com>
In-Reply-To: <CY4PR1201MB003708ADAD79BF4FD24D3445AACB0@CY4PR1201MB0037.namprd12.prod.outlook.com>
From:   John Stultz <john.stultz@linaro.org>
Date:   Tue, 23 Jul 2019 11:51:47 -0700
Message-ID: <CALAqxLURCLHf3UJsMWKZUirDE9bWNYEhv-sKb01g7cTfCz5tOg@mail.gmail.com>
Subject: Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc:     "fei.yang@intel.com" <fei.yang@intel.com>,
        "felipe.balbi@linux.intel.com" <felipe.balbi@linux.intel.com>,
        "andrzej.p@collabora.com" <andrzej.p@collabora.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 18, 2019 at 6:12 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> fei.yang@intel.com wrote:
> > From: Fei Yang <fei.yang@intel.com>
> >
> > 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@intel.com>
> > Cc: stable <stable@vger.kernel.org>
> > ---
> > 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