Hi,

Ville Syrjälä [off-list ref] writes:

On Thu, Oct 06, 2016 at 12:08:26PM +0300, Ville Syrjälä wrote:

quoted

On Thu, Oct 06, 2016 at 10:36:09AM +0300, Felipe Balbi wrote:

quoted

Hi,

Felipe Balbi [off-list ref] writes:

<snip>

quoted

Okay, I have found a regression on dwc3 and another patch follows:

commit 5e1a2af3e46248c55098cdae643c4141851b703e
Author: Felipe Balbi [off-list ref]
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: [off-list ref] # v4.8
    Fixes: 55a0237f8f47 ("usb: dwc3: gadget: use allocated/queued reqs for LST bit")
    Signed-off-by: Felipe Balbi [off-list ref]

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