Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 06, 2016 at 12:08:26PM +0300, Ville Syrj�l� wrote:
> On Thu, Oct 06, 2016 at 10:36:09AM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Felipe Balbi <felipe.balbi@linux.intel.com> writes:
> <snip>
> > Okay, I have found a regression on dwc3 and another patch follows:
> > 
> > commit 5e1a2af3e46248c55098cdae643c4141851b703e
> > Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> > Date:   Wed Oct 5 14:24:37 2016 +0300
> > 
> >     usb: dwc3: gadget: properly account queued requests
> >     
> >     Some requests could be accounted for multiple
> >     times. Let's fix that so each and every requests is
> >     accounted for only once.
> >     
> >     Cc: <stable@vger.kernel.org> # v4.8
> >     Fixes: 55a0237f8f47 ("usb: dwc3: gadget: use allocated/queued reqs for LST bit")
> >     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > 
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 07cc8929f271..3c3ced128c77 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -783,6 +783,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> >  		req->trb = trb;
> >  		req->trb_dma = dwc3_trb_dma_offset(dep, trb);
> >  		req->first_trb_index = dep->trb_enqueue;
> > +		dep->queued_requests++;
> >  	}
> >  
> >  	dwc3_ep_inc_enq(dep);
> > @@ -833,8 +834,6 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> >  
> >  	trb->ctrl |= DWC3_TRB_CTRL_HWO;
> >  
> > -	dep->queued_requests++;
> > -
> >  	trace_dwc3_prepare_trb(dep, trb);
> >  }
> >  
> > @@ -1861,8 +1860,11 @@ static int __dwc3_cleanup_done_trbs(struct dwc3 *dwc, struct dwc3_ep *dep,
> >  	unsigned int		s_pkt = 0;
> >  	unsigned int		trb_status;
> >  
> > -	dep->queued_requests--;
> >  	dwc3_ep_inc_deq(dep);
> > +
> > +	if (req->trb == trb)
> > +		dep->queued_requests--;
> > +
> >  	trace_dwc3_complete_trb(dep, trb);
> >  
> >  	/*
> > 
> > I have also built a branch which you can use for testing. Here's a pull
> > request, once you tell me it works for you, then I can send proper
> > patches out:
> > 
> > The following changes since commit c8d2bc9bc39ebea8437fd974fdbc21847bb897a3:
> > 
> >   Linux 4.8 (2016-10-02 16:24:33 -0700)
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tmp-test-v4.8
> > 
> > for you to fetch changes up to c968b8d1effe64a7980802d1eef29f4e1922faca:
> > 
> >   usb: dwc3: gadget: properly account queued requests (2016-10-06 10:16:37 +0300)
> > 
> > ----------------------------------------------------------------
> > Felipe Balbi (2):
> >       usb: gadget: function: u_ether: don't starve tx request queue
> >       usb: dwc3: gadget: properly account queued requests
> > 
> >  drivers/usb/dwc3/gadget.c             | 7 ++++---
> >  drivers/usb/gadget/function/u_ether.c | 5 +++--
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> Tried your branch, but unfortunately I'm still seeing the lags. New trace
> attached.

Just a reminder that the regressions is still there in 4.9-rc2.
Unfortunateyly with all the stuff already piled on top, getting USB
working on my device is no longer a simple matter of reverting the
one commit. I had to revert 10 patches to get even a clean revert, but
unfortunately that approach just lead to the transfer hanging immediately.

So what I ended up doing is reverting all of this:
git log --no-merges --format=oneline 55a0237f8f47957163125e20ee9260538c5c341c^..HEAD -- drivers/usb/dwc3/ include/linux/ulpi/interface.h drivers/usb/Kconfig drivers/usb/core/Kconfig

which comes out at whopping 70 commits. Having to carry that around
is going to be quite a pain especially as more stuff might be piled on
top.

/me thinks a stable backport of any fix (assuming one is found
eventually) is going to be quite the challenge...

-- 
Ville Syrj�l�
Intel OTC