All of lore.kernel.org
 help / color / mirror / Atom feed
From: smoch@web.de (Soeren Moch)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] USB: EHCI: fix for leaking isochronous data
Date: Thu, 21 Mar 2013 18:04:59 +0100	[thread overview]
Message-ID: <514B3DBB.3060302@web.de> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1303171320560.26486-100000@netrider.rowland.org>

On 03/17/13 18:36, Alan Stern wrote:
> On Sun, 17 Mar 2013, Soeren Moch wrote:
>
>> For each device only one isochronous endpoint is used (EP IN4, 1x 940
>> Bytes, Interval 1).
>> When the ENOMEM error occurs, a huge number of iTDs is in the free_list
>> of one stream. This number is much higher than the 2*M entries, which
>> should be there according to your description.
>
> Okay, but how did they get there?  With each URB requiring 9 iTDs, and
> about 5 URBs active at any time, there should be about 5*9 = 45 iTDs in
> use and 2*9 = 18 iTDs on the free list.  By the time each URB
> completes, it should have released all 9 iTDs back to the free list,
> and each time an URB is submitted, it should be able to acquire all 9
> of the iTDs that it needs from the free list -- it shouldn't have to
> allocate any from the DMA pool.
>
> Looks like you'll have to investigate what's going on inside
> itd_urb_transaction().  Print out some useful information whenever the
> size of stream->free_list is above 50, such as the value of num_itds,
> how many of the loop iterations could get an iTD from the free list,
> and the value of itd->frame in the case where the "goto alloc_itd"
> statement is followed.
>
> It might be a good idea also to print out the size of the free list in
> itd_complete(), where it calls ehci_urb_done(), and include the value
> of ehci->now_frame.
>

Now I found out what is going on here:

In itd_urb_transaction() we allocate 9 iTDs for each URB with 
number_of_packets == 64 in my case. The iTDs are added to 
sched->td_list. For a frame-aligned scheduling we need 8 iTDs, the 9th 
one is released back to the front of the streams free_list in 
iso_sched_free(). This iTD was cleared after allocation and has a frame 
number of 0 now. So for each allocation when now_frame == 0 we allocate 
from the dma_pool, not from the free_list. The attached patch 
invalidates the frame number in each iTD before it is sent to the 
scheduler. This fixes the problem without the need to iterate over a iTD 
list.

Signed-off-by: Soeren Moch <smoch@web.de>



-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehci.diff
Type: text/x-patch
Size: 603 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130321/5c488fe2/attachment.bin>

WARNING: multiple messages have this Message-ID (diff)
From: Soeren Moch <smoch@web.de>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Arnd Bergmann <arnd@arndb.de>,
	USB list <linux-usb@vger.kernel.org>,
	Jason Cooper <jason@lakedaemon.net>, Andrew Lunn <andrew@lunn.ch>,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
	linux-mm@kvack.org,
	Kernel development list <linux-kernel@vger.kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	michael@amarulasolutions.com
Subject: Re: [PATCH] USB: EHCI: fix for leaking isochronous data
Date: Thu, 21 Mar 2013 18:04:59 +0100	[thread overview]
Message-ID: <514B3DBB.3060302@web.de> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1303171320560.26486-100000@netrider.rowland.org>

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

On 03/17/13 18:36, Alan Stern wrote:
> On Sun, 17 Mar 2013, Soeren Moch wrote:
>
>> For each device only one isochronous endpoint is used (EP IN4, 1x 940
>> Bytes, Interval 1).
>> When the ENOMEM error occurs, a huge number of iTDs is in the free_list
>> of one stream. This number is much higher than the 2*M entries, which
>> should be there according to your description.
>
> Okay, but how did they get there?  With each URB requiring 9 iTDs, and
> about 5 URBs active at any time, there should be about 5*9 = 45 iTDs in
> use and 2*9 = 18 iTDs on the free list.  By the time each URB
> completes, it should have released all 9 iTDs back to the free list,
> and each time an URB is submitted, it should be able to acquire all 9
> of the iTDs that it needs from the free list -- it shouldn't have to
> allocate any from the DMA pool.
>
> Looks like you'll have to investigate what's going on inside
> itd_urb_transaction().  Print out some useful information whenever the
> size of stream->free_list is above 50, such as the value of num_itds,
> how many of the loop iterations could get an iTD from the free list,
> and the value of itd->frame in the case where the "goto alloc_itd"
> statement is followed.
>
> It might be a good idea also to print out the size of the free list in
> itd_complete(), where it calls ehci_urb_done(), and include the value
> of ehci->now_frame.
>

Now I found out what is going on here:

In itd_urb_transaction() we allocate 9 iTDs for each URB with 
number_of_packets == 64 in my case. The iTDs are added to 
sched->td_list. For a frame-aligned scheduling we need 8 iTDs, the 9th 
one is released back to the front of the streams free_list in 
iso_sched_free(). This iTD was cleared after allocation and has a frame 
number of 0 now. So for each allocation when now_frame == 0 we allocate 
from the dma_pool, not from the free_list. The attached patch 
invalidates the frame number in each iTD before it is sent to the 
scheduler. This fixes the problem without the need to iterate over a iTD 
list.

Signed-off-by: Soeren Moch <smoch@web.de>




[-- Attachment #2: ehci.diff --]
[-- Type: text/x-patch, Size: 603 bytes --]

--- linux-3.9.0-rc3-guru/drivers/usb/host/ehci-sched.c.orig	2013-03-21 17:36:21.000000000 +0100
+++ linux-3.9.0-rc3-guru/drivers/usb/host/ehci-sched.c	2013-03-21 17:38:56.000000000 +0100
@@ -1214,6 +1214,7 @@ itd_urb_transaction (
 
 		memset (itd, 0, sizeof *itd);
 		itd->itd_dma = itd_dma;
+		itd->frame = -1;
 		list_add (&itd->itd_list, &sched->td_list);
 	}
 	spin_unlock_irqrestore (&ehci->lock, flags);
@@ -1915,6 +1916,7 @@ sitd_urb_transaction (
 
 		memset (sitd, 0, sizeof *sitd);
 		sitd->sitd_dma = sitd_dma;
+		sitd->frame = -1;
 		list_add (&sitd->sitd_list, &iso_sched->td_list);
 	}
 

  parent reply	other threads:[~2013-03-21 17:04 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <Pine.LNX.4.44L0.1302211337580.1529-100000@iolanthe.rowland.org>
2013-03-10 18:45 ` [PATCH] USB: EHCI: fix for leaking isochronous data Soeren Moch
2013-03-10 18:45   ` Soeren Moch
2013-03-10 18:45   ` Soeren Moch
2013-03-10 20:59   ` Alan Stern
2013-03-10 20:59     ` Alan Stern
2013-03-10 20:59     ` Alan Stern
2013-03-14 18:48     ` Soeren Moch
2013-03-14 18:48       ` Soeren Moch
2013-03-14 18:48       ` Soeren Moch
2013-03-14 20:22       ` Soeren Moch
2013-03-14 20:22         ` Soeren Moch
2013-03-14 20:22         ` Soeren Moch
2013-03-14 20:32       ` Alan Stern
2013-03-14 20:32         ` Alan Stern
2013-03-14 20:32         ` Alan Stern
2013-03-14 20:51         ` Soeren Moch
2013-03-14 20:51           ` Soeren Moch
2013-03-14 21:33           ` Alan Stern
2013-03-14 21:33             ` Alan Stern
2013-03-14 21:33             ` Alan Stern
2013-03-15  0:00             ` Soeren Moch
2013-03-15  0:00               ` Soeren Moch
2013-03-15  0:00               ` Soeren Moch
2013-03-15 14:30               ` Alan Stern
2013-03-15 14:30                 ` Alan Stern
2013-03-15 14:30                 ` Alan Stern
2013-03-16  2:10             ` Soeren Moch
2013-03-16  2:10               ` Soeren Moch
2013-03-16  2:10               ` Soeren Moch
2013-03-16 17:39               ` Alan Stern
2013-03-16 17:39                 ` Alan Stern
2013-03-16 17:39                 ` Alan Stern
2013-03-17 16:56                 ` Soeren Moch
2013-03-17 16:56                   ` Soeren Moch
2013-03-17 16:56                   ` Soeren Moch
2013-03-17 17:36                   ` Alan Stern
2013-03-17 17:36                     ` Alan Stern
2013-03-17 17:36                     ` Alan Stern
2013-03-17 17:39                     ` Alan Stern
2013-03-17 17:39                       ` Alan Stern
2013-03-17 17:39                       ` Alan Stern
2013-03-21 17:04                     ` Soeren Moch [this message]
2013-03-21 17:04                       ` Soeren Moch
2013-03-21 17:33                       ` Jason Cooper
2013-03-21 17:33                         ` Jason Cooper
2013-03-21 17:33                         ` Jason Cooper
2013-03-21 19:10                         ` Arnd Bergmann
2013-03-21 19:10                           ` Arnd Bergmann
2013-03-21 19:10                           ` Arnd Bergmann
2013-03-21 19:34                           ` Michael Trimarchi
2013-03-21 19:34                             ` Michael Trimarchi
2013-03-21 19:34                             ` Michael Trimarchi
2013-03-21 21:52                           ` Soeren Moch
2013-03-21 21:52                             ` Soeren Moch
2013-03-21 21:52                             ` Soeren Moch
2013-03-21 21:06                       ` Alan Stern
2013-03-21 21:06                         ` Alan Stern
2013-03-21 21:06                         ` Alan Stern
2013-03-21 21:12                         ` Alan Stern
2013-03-21 21:12                           ` Alan Stern
2013-03-21 21:12                           ` Alan Stern
2013-03-21 21:20                           ` Andrew Lunn
2013-03-21 21:20                             ` Andrew Lunn
2013-03-21 21:20                             ` Andrew Lunn
2013-03-21 22:16                             ` Soeren Moch
2013-03-21 22:16                               ` Soeren Moch
2013-03-21 22:16                               ` Soeren Moch
2013-03-22 14:24                               ` Alan Stern
2013-03-22 14:24                                 ` Alan Stern
2013-03-22 14:24                                 ` Alan Stern
2013-03-21 21:45                           ` Soeren Moch
2013-03-21 21:45                             ` Soeren Moch
2013-03-21 21:45                             ` Soeren Moch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=514B3DBB.3060302@web.de \
    --to=smoch@web.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.