Message-ID: <87pomksrrm.fsf@linux.intel.com>
Date:   Fri, 28 Oct 2016 13:16:13 +0300
From:   Felipe Balbi <felipe.balbi@...ux.intel.com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     linux-usb@...r.kernel.org, stable@...r.kernel.org,
        davem@...emloft.net, Oliver Neukum <oneukum@...e.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH] Revert "usb: dwc3: gadget: use allocated/queued reqs for LST bit"


Hi,

Ville Syrjälä <ville.syrjala@...ux.intel.com> writes:
> 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@...ux.intel.com> writes:
>> <snip>
>> > Okay, I have found a regression on dwc3 and another patch follows:
>> > 
>> > commit 5e1a2af3e46248c55098cdae643c4141851b703e
>> > Author: Felipe Balbi <felipe.balbi@...ux.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@...r.kernel.org> # v4.8
>> >     Fixes: 55a0237f8f47 ("usb: dwc3: gadget: use allocated/queued reqs for LST bit")
>> >     Signed-off-by: Felipe Balbi <felipe.balbi@...ux.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...

Yeah, I'm guessing we're gonna need some help from networking folks. The
only thing we did since v4.7 was actually respect req->no_interrupt flag
coming from u_ether itself. No idea why that causes so much trouble for
u_ether.

BTW, Instead of reverting so many patches, you can just remove
throttling:

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index f4a640216913..119a2e5848e8 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -589,14 +589,6 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 
        req->length = length;
 
-       /* throttle high/super speed IRQ rate back slightly */
-       if (gadget_is_dualspeed(dev->gadget))
-               req->no_interrupt = (((dev->gadget->speed == USB_SPEED_HIGH ||
-                                      dev->gadget->speed == USB_SPEED_SUPER)) &&
-                                       !list_empty(&dev->tx_reqs))
-                       ? ((atomic_read(&dev->tx_qlen) % dev->qmult) != 0)
-                       : 0;
-
        retval = usb_ep_queue(in, req, GFP_ATOMIC);
        switch (retval) {
        default:

I'm adding netdev and couple other folks to the loop.

Just to summarize, USB peripheral controller now actually throttles
interrupt when requested to do so and that causes lags for USB
networking gadgets.

Without throttle we, potentially, call netif_wake_queue() more
frequently than with throttling. I'm wondering if something changed in
NET layer within the past few years but the USB networking gadgets ended
up being forgotten.

Anyway, if anybody has any hints, I'd be glad to hear about them.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (801 bytes)

Powered by blists - more mailing lists