Allocating buffers for USB transfers (again)

Allocating buffers for USB transfers (again)

[Daniel Mack]

I have to revisit an issue we've discussed in length around a year ago
and which still remains unsolved. I've been getting feedback from more
users of my driver snd-usb-caiaq who report the issue found in bug
#15580 in the kernel bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=15580

Let me quickly summarize the current state of this issue as I see it.

The problem seems to be that certain 64bit chipsets can't deal with
the fact that an URB's transfer_buffer is allocated with
kmalloc(GFP_KERNEL). The effect is that kmalloc() is very likely to
hand out memory which is not addressable by devices that are connected
via 32bit PCI busses, such as EHCI controllers. In theory, DMA bounce
buffers should be installed in such cases, or the IOMMU would be in
charge to re-map these buffers to suitable locations, but for at least
two people who have reported the issue, this obviously fails.

So my approach was to force the driver using memory that is DMA
coherent, which leads to 32bit addressable memory if the device's
DMA_MASK is set accordingly. Other drivers do the same, and the patch
for doing this is found here: http://lkml.org/lkml/2010/5/7/61. Users
that see the bug all report that this patch fixes their problem with
my driver. However, it was rejected back then as other developers said
it was fixing the wrong end, and that the driver doesn't necessarily
need coherent memory.

Takashi recently posted a patch to the bugzilla entry which uses a
different approach: it introduces a function to determine suitable GFP
flags for USB devices, and passes __DMA32 to kmalloc() eventually.
However, using this flags directly with the SLUB allocator is illegal
and causes a BUG() in mm/slub.c, cache_grow().

The question now is how to proceed. I see three possible ways.

1. Find a way to allocate 32bit-aware memory with kmalloc(), following
Takashi's idea
2. Find out exactly why these machines fail to install bounce buffers
or set up their IOMMU correctly
3. re-activate the currently disabled functions
usb_buffer_{map,unmap,sync} functions and let the USB stack do the
memory mapping

I'm not really deep into all the memory management code and even less
into how the IOMMU and bounce buffers work, so I need some help here.
Can anyone elaborate what would be the best option? I really want to
get this fixed now, and provide a driver that works for everyone.

What really puzzles me is that this doesn't hit a lot more people with
other drivers.


Thanks,
Daniel

Re: Allocating buffers for USB transfers (again)

[Clemens Ladisch]

Daniel Mack wrote:
> I have to revisit an issue we've discussed in length around a year ago
> and which still remains unsolved. I've been getting feedback from more
> users of my driver snd-usb-caiaq who report the issue found in bug
> #15580 in the kernel bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=15580
> 
> Let me quickly summarize the current state of this issue as I see it.
> 
> The problem seems to be that certain 64bit chipsets can't deal with
> the fact that an URB's transfer_buffer is allocated with
> kmalloc(GFP_KERNEL). The effect is that kmalloc() is very likely to
> hand out memory which is not addressable by devices that are connected
> via 32bit PCI busses, such as EHCI controllers. In theory, DMA bounce
> buffers should be installed in such cases, or the IOMMU would be in
> charge to re-map these buffers to suitable locations, but for at least
> two people who have reported the issue, this obviously fails.

The problem is that snd-usb-caiaq modifies the URB buffer after
submission.

USB drivers must always be prepared to work with host controllers that
use double buffering.  (IIRC there are some exotic ones where the
hardware works only with internal SRAM.)  DMA mapping, if available, is
only an optimization.

> The question now is how to proceed. I see three possible ways.
> ...
> 3. re-activate the currently disabled functions
> usb_buffer_{map,unmap,sync} functions and let the USB stack do the
> memory mapping

This is the only way that allows bounce buffers to be copied at the
correct time.

> What really puzzles me is that this doesn't hit a lot more people with
> other drivers.

I don't think there is any other driver that relies on the buffer being
coherently DMA-mapped.


Regards,
Clemens

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Thu, Jul 7, 2011 at 2:14 PM, Clemens Ladisch <clemens@ladisch.de> wrote:
> Daniel Mack wrote:
>> I have to revisit an issue we've discussed in length around a year ago
>> and which still remains unsolved. I've been getting feedback from more
>> users of my driver snd-usb-caiaq who report the issue found in bug
>> #15580 in the kernel bugzilla:
>> https://bugzilla.kernel.org/show_bug.cgi?id=15580
>>
>> Let me quickly summarize the current state of this issue as I see it.
>>
>> The problem seems to be that certain 64bit chipsets can't deal with
>> the fact that an URB's transfer_buffer is allocated with
>> kmalloc(GFP_KERNEL). The effect is that kmalloc() is very likely to
>> hand out memory which is not addressable by devices that are connected
>> via 32bit PCI busses, such as EHCI controllers. In theory, DMA bounce
>> buffers should be installed in such cases, or the IOMMU would be in
>> charge to re-map these buffers to suitable locations, but for at least
>> two people who have reported the issue, this obviously fails.
>
> The problem is that snd-usb-caiaq modifies the URB buffer after
> submission.

Hmm, no, it doesn't. I know I stated that once, but I confused the
implementation of the Linux driver with the approach I chose for the
Mac OS X driver code. Sorry about that.

The driver simply copies audio material to its output URB and calls
usb_submit_urb(). The URB buffer is not touched after that call.


Daniel

Re: Allocating buffers for USB transfers (again)

[Oliver Neukum]

Am Donnerstag, 7. Juli 2011, 13:53:46 schrieb Daniel Mack:
> Takashi recently posted a patch to the bugzilla entry which uses a
> different approach: it introduces a function to determine suitable GFP
> flags for USB devices, and passes __DMA32 to kmalloc() eventually.
> However, using this flags directly with the SLUB allocator is illegal
> and causes a BUG() in mm/slub.c, cache_grow().
> 
> The question now is how to proceed. I see three possible ways.
> 
> 1. Find a way to allocate 32bit-aware memory with kmalloc(), following
> Takashi's idea

That is not really a good idea, because many drivers have to deal with buffers
another subsystem has allocated.

> 2. Find out exactly why these machines fail to install bounce buffers
> or set up their IOMMU correctly

This is still the correct approach. Given the cause this cannot be a problem
only for audio. We need the root cause.

> 3. re-activate the currently disabled functions
> usb_buffer_{map,unmap,sync} functions and let the USB stack do the
> memory mapping

The USB stack does call the DMA mapping operations.

	Regards
		Oliver

PS:  Do you still see this if you enable 64bit DMA for EHCI?

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@neukum.org> wrote:

> PS:  Do you still see this if you enable 64bit DMA for EHCI?

The problem is that I personally don't see that issue at all. I even
installed 4GB of RAM to my development machine last year to be able to
reproduce this, but I can't, even when the memory allocator is under
heavy load. The only people who see this effect are Pedro Ribeiro and
William Light (both in Cc:), and both have been very helpful in trying
patches and reporting back in detail. Which instructions could we
probably give to these people to finally hunt this issue down?

Daniel

Re: Allocating buffers for USB transfers (again)

[Oliver Neukum]

Am Donnerstag, 7. Juli 2011, 14:38:51 schrieb Daniel Mack:
> On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@neukum.org> wrote:
> 
> > PS:  Do you still see this if you enable 64bit DMA for EHCI?
> 
> The problem is that I personally don't see that issue at all. I even
> installed 4GB of RAM to my development machine last year to be able to
> reproduce this, but I can't, even when the memory allocator is under
> heavy load. The only people who see this effect are Pedro Ribeiro and
> William Light (both in Cc:), and both have been very helpful in trying
> patches and reporting back in detail. Which instructions could we
> probably give to these people to finally hunt this issue down?

Do they have an IOMMU or are they using bounce buffers?
If the latter, you might be able to reproduce it if you disable your
IOMMU.

	Regards
		Oliver

Re: Allocating buffers for USB transfers (again)

[Johannes Stezenbach]

On Thu, Jul 07, 2011 at 01:53:46PM +0200, Daniel Mack wrote:
> So my approach was to force the driver using memory that is DMA
> coherent, which leads to 32bit addressable memory if the device's
> DMA_MASK is set accordingly. Other drivers do the same, and the patch
> for doing this is found here: http://lkml.org/lkml/2010/5/7/61. Users
> that see the bug all report that this patch fixes their problem with
> my driver. However, it was rejected back then as other developers said
> it was fixing the wrong end, and that the driver doesn't necessarily
> need coherent memory.

I think this is a valid approach, and since it avoids
using bounce buffers it is potentially better wrt performance.
BTW, usb_alloc_coherent() doc comment says you need to set the
URB_NO_TRANSFER_DMA_MAP flag, which your patch fails to do.

> Takashi recently posted a patch to the bugzilla entry which uses a
> different approach: it introduces a function to determine suitable GFP
> flags for USB devices, and passes __DMA32 to kmalloc() eventually.
> However, using this flags directly with the SLUB allocator is illegal
> and causes a BUG() in mm/slub.c, cache_grow().

from gfp.h:
  /* Do not use these with a slab allocator */
  #define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

I think it means you can only use __GFP_DMA32 for
get_free_pages(), not for kmalloc().

> 2. Find out exactly why these machines fail to install bounce buffers
> or set up their IOMMU correctly

I think this needs to be done.


Johannes

Re: Allocating buffers for USB transfers (again)

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1265 bytes --]

On Thu, 7 Jul 2011, Daniel Mack wrote:

> On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@neukum.org> wrote:
> 
> > PS:  Do you still see this if you enable 64bit DMA for EHCI?
> 
> The problem is that I personally don't see that issue at all. I even
> installed 4GB of RAM to my development machine last year to be able to
> reproduce this, but I can't, even when the memory allocator is under
> heavy load. The only people who see this effect are Pedro Ribeiro and
> William Light (both in Cc:), and both have been very helpful in trying
> patches and reporting back in detail. Which instructions could we
> probably give to these people to finally hunt this issue down?

Oliver is perfectly right; this problem needs to be solved at the 
source.

One thing you might be able to do is put together a debugging patch
that would print out the bus address corresponding to the DMA handle
after the DMA mapping takes place, i.e., after the second
dma_map_single() call in usb_hcd_map_urb_for_dma() returns.  Or at
least, check the bus address to see if it is below the 4 GB limit and
print out an error message if it isn't.

(I don't know how one translates a DMA address to a bus address.  You'd 
have to ask somebody who knows or figure it out.)

Alan Stern

Re: Allocating buffers for USB transfers (again)

[Florian Mickler]

[adding Robert Hancock]
see here for the whole thread: https://lkml.org/lkml/2011/7/7/150

2011/7/7 Daniel Mack <zonque@gmail.com>:
> On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@neukum.org> wrote:
>
>> PS:  Do you still see this if you enable 64bit DMA for EHCI?
>
> The problem is that I personally don't see that issue at all. I even
> installed 4GB of RAM to my development machine last year to be able to
> reproduce this, but I can't, even when the memory allocator is under
> heavy load. The only people who see this effect are Pedro Ribeiro and
> William Light (both in Cc:), and both have been very helpful in trying
> patches and reporting back in detail. Which instructions could we
> probably give to these people to finally hunt this issue down?
>
> Daniel
>
>

Did someone affected already try the suggestions from Robert :
http://lkml.org/lkml/2010/4/10/66

That is checking if it really is a 64bit vs 32bit issue by booting
with mem=4096M ?


Regards,
Flo

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

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

On Thu, Jul 7, 2011 at 3:08 PM, Oliver Neukum <oliver@neukum.org> wrote:
> Am Donnerstag, 7. Juli 2011, 14:38:51 schrieb Daniel Mack:
>> On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@neukum.org> wrote:
>>
>> > PS:  Do you still see this if you enable 64bit DMA for EHCI?
>>
>> The problem is that I personally don't see that issue at all. I even
>> installed 4GB of RAM to my development machine last year to be able to
>> reproduce this, but I can't, even when the memory allocator is under
>> heavy load. The only people who see this effect are Pedro Ribeiro and
>> William Light (both in Cc:), and both have been very helpful in trying
>> patches and reporting back in detail. Which instructions could we
>> probably give to these people to finally hunt this issue down?
>
> Do they have an IOMMU or are they using bounce buffers?
> If the latter, you might be able to reproduce it if you disable your
> IOMMU.

Ok, Pedro and William - can I ask you to dump the following
information to some paste bin:

- your .config
- the dmesg output of a booted kernel that shows the effect
- the output of "sudo lspci -v"

Also, the attached patch might give you at least one warning to your
kernel logs once the audio stream starts and outputs noise. I'm
curious whether it really does.

Thanks,
Daniel

[-- Attachment #2: ehci-32bit-debug.diff --]
[-- Type: text/x-patch, Size: 605 bytes --]

diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 6c9fbe3..4260f78 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -1188,6 +1188,9 @@ itd_sched_init(
 		trans |= length << 16;
 		uframe->transaction = cpu_to_hc32(ehci, trans);
 
+		WARN_ONCE((u64)buf & (u64) 0xfff, "XXXXX URB DMA %016x is not 4k-aligned", (u64) buf);
+		WARN_ONCE((u64)buf & 0xffffffff00000000ULL, "XXXXX URB DMA %016x has upper 32bit set", (u64) buf);
+
 		/* might need to cross a buffer page within a uframe */
 		uframe->bufp = (buf & ~(u64)0x0fff);
 		buf += length;

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

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

On Thu, Jul 7, 2011 at 5:16 PM, Florian Mickler <florian@mickler.org> wrote:
> [adding Robert Hancock]
> see here for the whole thread: https://lkml.org/lkml/2011/7/7/150
>
> 2011/7/7 Daniel Mack <zonque@gmail.com>:
>> On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@neukum.org> wrote:
>>
>>> PS:  Do you still see this if you enable 64bit DMA for EHCI?
>>
>> The problem is that I personally don't see that issue at all. I even
>> installed 4GB of RAM to my development machine last year to be able to
>> reproduce this, but I can't, even when the memory allocator is under
>> heavy load. The only people who see this effect are Pedro Ribeiro and
>> William Light (both in Cc:), and both have been very helpful in trying
>> patches and reporting back in detail. Which instructions could we
>> probably give to these people to finally hunt this issue down?
>>
>> Daniel
>>
>>
>
> Did someone affected already try the suggestions from Robert :
> http://lkml.org/lkml/2010/4/10/66
>
> That is checking if it really is a 64bit vs 32bit issue by booting
> with mem=4096M ?

So it seems we can finally close this issue. I got myself a new laptop
recently, and was actually happy to see that this machine showed the
bug as well. So I started investigating on what's going on and after
hours of debugging in the wrong area, I stumbled over a bug in my own
code. The patch attached fixes the problem for me, and William (one of
the very few people who saw it, too) also reported green light.

However, it remains a puzzle to me where the acutal root cause is for
this is, and I would much like to know. Let me briefly explain what
I'm seeing.

The driver operates in a mode so that it copies the layout of its
inbound stream to the output stream, thus guaranteeing the data rate
of outbound data is equal to that of the input stream. For that, I
walk the isochronous frames of each received urb and prepare an urb
for sending that has the same iso packet structure in terms of lengths
and offsets, jumping over and ignoring frames that are actually
invalid (status != 0). So here was the problem: if there were frames
with any different status field, the driver would calculate the wrong
offset and the playback stream will have artefacts. So, a classic bug
you might think, and the fix is trivial.

However, what I really don't understand is why this wasn't observed
any earlier by any of the many users, and why iit makes a difference
which type of memory I use for this. Recall that my original patch
that allocates DMA coherent memory for the transfer buffers also fixes
the problem just fine. Or put it that way: when allocating "normal"
memory using kmalloc(), the stream appears to have "holes" in the iso
packet structure, while with DMA coherent memory, the layout is
different. And all this only happens on fairly new 64bit-enabled
machines.

Can anyone explain this?


Many thanks to everyone involved in this - and especially to William
and Pedro who spent a lot of time in trying my patches and sending me
debug information.



Daniel

[-- Attachment #2: 0001-ALSA-snd-usb-caiaq-Correct-offset-fields-of-outbound.patch --]
[-- Type: text/x-patch, Size: 1548 bytes --]

From 0b95c8b9402aaa39c0096df00209eb73450d08a8 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@tamtam.(none)>
Date: Fri, 5 Aug 2011 13:49:52 +0200
Subject: [PATCH] ALSA: snd-usb-caiaq: Correct offset fields of outbound
 iso_frame_desc

This fixes faulty outbount packets in case the inbound packets
received from the hardware are fragmented and contain bogus input
iso frames. The bug has been there for ages, but for some strange
reasons, it was only triggered by newer machines in 64bit mode.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-and-tested-by: William Light <wrl@illest.net>
Reported-by: Pedro Ribeiro <pedrib@gmail.com>
Cc: stable@kernel.org
---
 sound/usb/caiaq/audio.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index d0d493c..4c8249e 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -614,6 +614,7 @@ static void read_completed(struct urb *urb)
 	struct snd_usb_caiaqdev *dev;
 	struct urb *out;
 	int frame, len, send_it = 0, outframe = 0;
+	size_t offset = 0;
 
 	if (urb->status || !info)
 		return;
@@ -634,7 +635,8 @@ static void read_completed(struct urb *urb)
 		len = urb->iso_frame_desc[outframe].actual_length;
 		out->iso_frame_desc[outframe].length = len;
 		out->iso_frame_desc[outframe].actual_length = 0;
-		out->iso_frame_desc[outframe].offset = BYTES_PER_FRAME * frame;
+		out->iso_frame_desc[outframe].offset = offset;
+		offset += len;
 
 		if (len > 0) {
 			spin_lock(&dev->spinlock);
-- 
1.7.6

Re: Allocating buffers for USB transfers (again)

[Alan Stern]

On Wed, 10 Aug 2011, Daniel Mack wrote:

> So it seems we can finally close this issue. I got myself a new laptop
> recently, and was actually happy to see that this machine showed the
> bug as well. So I started investigating on what's going on and after
> hours of debugging in the wrong area, I stumbled over a bug in my own
> code. The patch attached fixes the problem for me, and William (one of
> the very few people who saw it, too) also reported green light.
> 
> However, it remains a puzzle to me where the acutal root cause is for
> this is, and I would much like to know. Let me briefly explain what
> I'm seeing.
> 
> The driver operates in a mode so that it copies the layout of its
> inbound stream to the output stream, thus guaranteeing the data rate
> of outbound data is equal to that of the input stream. For that, I
> walk the isochronous frames of each received urb and prepare an urb
> for sending that has the same iso packet structure in terms of lengths
> and offsets, jumping over and ignoring frames that are actually
> invalid (status != 0). So here was the problem: if there were frames
> with any different status field, the driver would calculate the wrong
> offset and the playback stream will have artefacts. So, a classic bug
> you might think, and the fix is trivial.
> 
> However, what I really don't understand is why this wasn't observed
> any earlier by any of the many users, and why iit makes a difference
> which type of memory I use for this. Recall that my original patch
> that allocates DMA coherent memory for the transfer buffers also fixes
> the problem just fine. Or put it that way: when allocating "normal"
> memory using kmalloc(), the stream appears to have "holes" in the iso
> packet structure, while with DMA coherent memory, the layout is
> different. And all this only happens on fairly new 64bit-enabled
> machines.
> 
> Can anyone explain this?

Looking at the driver's current code, it appears that your patch does 
not fix the bug properly.  Using discontiguous regions in the transfer 
buffer is perfectly okay.  The real problem is later on, where you do:

	if (send_it) {
		out->number_of_packets = FRAMES_PER_URB;

This should be

		out->number_of_packets = outframe;

The way it is now, the USB stack will try to use data from all the 
frame descriptors, and the last few will be stale because the loop 
doesn't set them.

I don't know why this would cause the strange symptoms, though.

Alan Stern

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

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

On 08/10/2011 04:32 PM, Alan Stern wrote:
> Looking at the driver's current code, it appears that your patch
> does not fix the bug properly.  Using discontiguous regions in the
> transfer buffer is perfectly okay.  The real problem is later on,
> where you do:
>
> if (send_it) { out->number_of_packets = FRAMES_PER_URB;
>
> This should be
>
> out->number_of_packets = outframe;
>
> The way it is now, the USB stack will try to use data from all the
> frame descriptors, and the last few will be stale because the loop
> doesn't set them.

That's actually true, even though it doesn't seem to cause any trouble.
I tested everything here of course, and the output URBs return back from
the USB stack with their length fields zeroed out, which then
causes the stack to send packets with zero-length fields at the end.

However, doing it right doesn't harm either. Thanks for spotting.

Takashi, can you apply this version?


Thanks,
Daniel


[-- Attachment #2: 0001-ALSA-snd-usb-caiaq-Correct-offset-fields-of-outbound.patch --]
[-- Type: text/plain, Size: 1795 bytes --]

>From 57e3283c950e2b783a8f6afc001cc0e16aaaaea2 Mon Sep 17 00:00:00 2001
From: Daniel Mack <zonque@gmail.com>
Date: Fri, 5 Aug 2011 13:49:52 +0200
Subject: [PATCH] ALSA: snd-usb-caiaq: Correct offset fields of outbound
 iso_frame_desc

This fixes faulty outbount packets in case the inbound packets
received from the hardware are fragmented and contain bogus input
iso frames. The bug has been there for ages, but for some strange
reasons, it was only triggered by newer machines in 64bit mode.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-and-tested-by: William Light <wrl@illest.net>
Reported-by: Pedro Ribeiro <pedrib@gmail.com>
Cc: stable@kernel.org
---
 sound/usb/caiaq/audio.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index d0d493c..aa52b3e 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -614,6 +614,7 @@ static void read_completed(struct urb *urb)
 	struct snd_usb_caiaqdev *dev;
 	struct urb *out;
 	int frame, len, send_it = 0, outframe = 0;
+	size_t offset = 0;
 
 	if (urb->status || !info)
 		return;
@@ -634,7 +635,8 @@ static void read_completed(struct urb *urb)
 		len = urb->iso_frame_desc[outframe].actual_length;
 		out->iso_frame_desc[outframe].length = len;
 		out->iso_frame_desc[outframe].actual_length = 0;
-		out->iso_frame_desc[outframe].offset = BYTES_PER_FRAME * frame;
+		out->iso_frame_desc[outframe].offset = offset;
+		offset += len;
 
 		if (len > 0) {
 			spin_lock(&dev->spinlock);
@@ -650,7 +652,7 @@ static void read_completed(struct urb *urb)
 	}
 
 	if (send_it) {
-		out->number_of_packets = FRAMES_PER_URB;
+		out->number_of_packets = outframe;
 		out->transfer_flags = URB_ISO_ASAP;
 		usb_submit_urb(out, GFP_ATOMIC);
 	}
-- 
1.7.6

Re: Allocating buffers for USB transfers (again)

[Takashi Iwai]

At Wed, 10 Aug 2011 17:33:02 +0200,
Daniel Mack wrote:
> 
> On 08/10/2011 04:32 PM, Alan Stern wrote:
> > Looking at the driver's current code, it appears that your patch
> > does not fix the bug properly.  Using discontiguous regions in the
> > transfer buffer is perfectly okay.  The real problem is later on,
> > where you do:
> >
> > if (send_it) { out->number_of_packets = FRAMES_PER_URB;
> >
> > This should be
> >
> > out->number_of_packets = outframe;
> >
> > The way it is now, the USB stack will try to use data from all the
> > frame descriptors, and the last few will be stale because the loop
> > doesn't set them.
> 
> That's actually true, even though it doesn't seem to cause any trouble.
> I tested everything here of course, and the output URBs return back from
> the USB stack with their length fields zeroed out, which then
> causes the stack to send packets with zero-length fields at the end.
> 
> However, doing it right doesn't harm either. Thanks for spotting.
> 
> Takashi, can you apply this version?

Yep, applied now.  Thanks.


Takashi

Re: Allocating buffers for USB transfers (again)

[Sarah Sharp]

On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
> On 08/10/2011 04:32 PM, Alan Stern wrote:
> >Looking at the driver's current code, it appears that your patch
> >does not fix the bug properly.  Using discontiguous regions in the
> >transfer buffer is perfectly okay.  The real problem is later on,
> >where you do:
> >
> >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
> >
> >This should be
> >
> >out->number_of_packets = outframe;
> >
> >The way it is now, the USB stack will try to use data from all the
> >frame descriptors, and the last few will be stale because the loop
> >doesn't set them.
> 
> That's actually true, even though it doesn't seem to cause any trouble.
> I tested everything here of course, and the output URBs return back from
> the USB stack with their length fields zeroed out, which then
> causes the stack to send packets with zero-length fields at the end.

Actually, it causes system hangs when the driver is loaded on a device
attached to a USB 3.0 port, as Alan Stern pointed out:

https://bugzilla.kernel.org/show_bug.cgi?id=40702

Please don't submit zero-length transfers.  The xHCI driver just isn't
able to handle it.  Arguably, it probably should have just rejected your
URB when it found a zero length buffer, so I'll probably be submitting a
patch to fix that.

Sarah Sharp

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Thu, Aug 11, 2011 at 1:15 AM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
>> On 08/10/2011 04:32 PM, Alan Stern wrote:
>> >Looking at the driver's current code, it appears that your patch
>> >does not fix the bug properly.  Using discontiguous regions in the
>> >transfer buffer is perfectly okay.  The real problem is later on,
>> >where you do:
>> >
>> >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
>> >
>> >This should be
>> >
>> >out->number_of_packets = outframe;
>> >
>> >The way it is now, the USB stack will try to use data from all the
>> >frame descriptors, and the last few will be stale because the loop
>> >doesn't set them.
>>
>> That's actually true, even though it doesn't seem to cause any trouble.
>> I tested everything here of course, and the output URBs return back from
>> the USB stack with their length fields zeroed out, which then
>> causes the stack to send packets with zero-length fields at the end.
>
> Actually, it causes system hangs when the driver is loaded on a device
> attached to a USB 3.0 port, as Alan Stern pointed out:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=40702

Yes, I've noticed this.

> Please don't submit zero-length transfers.  The xHCI driver just isn't
> able to handle it.  Arguably, it probably should have just rejected your
> URB when it found a zero length buffer, so I'll probably be submitting a
> patch to fix that.

According to the spec, sending zero-length frames should be fine, no?
Is there any particular reason why XCHI can't handle this while EHCI
can? And does my patch fix the driver for XHCI?


Daniel

Re: Allocating buffers for USB transfers (again)

[Andiry Xu]

On Wed, 2011-08-10 at 16:15 -0700, Sarah Sharp wrote:
> On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
> > On 08/10/2011 04:32 PM, Alan Stern wrote:
> > >Looking at the driver's current code, it appears that your patch
> > >does not fix the bug properly.  Using discontiguous regions in the
> > >transfer buffer is perfectly okay.  The real problem is later on,
> > >where you do:
> > >
> > >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
> > >
> > >This should be
> > >
> > >out->number_of_packets = outframe;
> > >
> > >The way it is now, the USB stack will try to use data from all the
> > >frame descriptors, and the last few will be stale because the loop
> > >doesn't set them.
> > 
> > That's actually true, even though it doesn't seem to cause any trouble.
> > I tested everything here of course, and the output URBs return back from
> > the USB stack with their length fields zeroed out, which then
> > causes the stack to send packets with zero-length fields at the end.
> 
> Actually, it causes system hangs when the driver is loaded on a device
> attached to a USB 3.0 port, as Alan Stern pointed out:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=40702
> 
> Please don't submit zero-length transfers.  The xHCI driver just isn't
> able to handle it.  Arguably, it probably should have just rejected your
> URB when it found a zero length buffer, so I'll probably be submitting a
> patch to fix that.
> 

I think queue a zero-length TRB to xhci host is OK. I've not tested it,
but the issue here seems is caused by td->last_trb = NULL. Check
count_isoc_trbs_needed(), num_trbs will be 0 if the packet length is
zero and (addr & (TRB_MAS_BUFF_SIZE - 1)) is zero. We can not return
num_trbs as 0 to xhci_queue_isoc_tx(), which caused a td added to ep's
td list, while it's not actually queued to ep ring and last_trb is not
set.

In order to avoid this, we just make sure count_isoc_trbs_needed()
always return 1 or larger numbers, instead of reject the urb. Is that
feasible?

Thanks,
Andiry

Re: Allocating buffers for USB transfers (again)

[Alan Stern]

On Thu, 11 Aug 2011, Andiry Xu wrote:

> > Please don't submit zero-length transfers.  The xHCI driver just isn't
> > able to handle it.  Arguably, it probably should have just rejected your
> > URB when it found a zero length buffer, so I'll probably be submitting a
> > patch to fix that.
> > 
> 
> I think queue a zero-length TRB to xhci host is OK. I've not tested it,
> but the issue here seems is caused by td->last_trb = NULL. Check
> count_isoc_trbs_needed(), num_trbs will be 0 if the packet length is
> zero and (addr & (TRB_MAS_BUFF_SIZE - 1)) is zero. We can not return
> num_trbs as 0 to xhci_queue_isoc_tx(), which caused a td added to ep's
> td list, while it's not actually queued to ep ring and last_trb is not
> set.
> 
> In order to avoid this, we just make sure count_isoc_trbs_needed()
> always return 1 or larger numbers, instead of reject the urb. Is that
> feasible?

I just looked for the first time at the code in 
count_isoc_trbs_needed().  It does seem rather sub-optimal.

The basic idea is that you need to know how many TRB buffers can cover 
the memory area spanned by the packet data, where a TRB buffer's size 
cannot be larger than TRB_MAX_BUFF_SIZE and all but the first buffer 
must be aligned on a TRB_MAX_BUFF_SIZE boundary, right?  With a slight 
adjustment in the case of a zero-length packet, since you always need 
at least one TRB.

Given that the packet data starts at addr and has length td_len, the 
number of TRBs should be calculated as follows:

	num = DIV_ROUND_UP(td_len + (addr & (TRB_MAX_BUFF_SIZE - 1))), 
			TRB_MAX_BUFF_SIZE);
	num += (num == 0);

No need for a running_total or a loop.

Alan Stern

Re: Allocating buffers for USB transfers (again)

[Sarah Sharp]

On Thu, Aug 11, 2011 at 02:57:41AM +0200, Daniel Mack wrote:
> On Thu, Aug 11, 2011 at 1:15 AM, Sarah Sharp
> <sarah.a.sharp@linux.intel.com> wrote:
> > On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
> >> On 08/10/2011 04:32 PM, Alan Stern wrote:
> >> >Looking at the driver's current code, it appears that your patch
> >> >does not fix the bug properly.  Using discontiguous regions in the
> >> >transfer buffer is perfectly okay.  The real problem is later on,
> >> >where you do:
> >> >
> >> >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
> >> >
> >> >This should be
> >> >
> >> >out->number_of_packets = outframe;
> >> >
> >> >The way it is now, the USB stack will try to use data from all the
> >> >frame descriptors, and the last few will be stale because the loop
> >> >doesn't set them.
> >>
> >> That's actually true, even though it doesn't seem to cause any trouble.
> >> I tested everything here of course, and the output URBs return back from
> >> the USB stack with their length fields zeroed out, which then
> >> causes the stack to send packets with zero-length fields at the end.
> >
> > Actually, it causes system hangs when the driver is loaded on a device
> > attached to a USB 3.0 port, as Alan Stern pointed out:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=40702
> 
> Yes, I've noticed this.
> 
> > Please don't submit zero-length transfers.  The xHCI driver just isn't
> > able to handle it.  Arguably, it probably should have just rejected your
> > URB when it found a zero length buffer, so I'll probably be submitting a
> > patch to fix that.
> 
> According to the spec, sending zero-length frames should be fine, no?
> Is there any particular reason why XCHI can't handle this while EHCI
> can? And does my patch fix the driver for XHCI?

Ok, yes, you're correct that the xHCI spec allows the transfer length to
be set to zero.  In the case where the frame buffer is zero-length, is
the buffer pointer still valid?  It's not clear from the spec whether it
needs to be.

Sarah Sharp

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Thu, Aug 11, 2011 at 6:45 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Thu, Aug 11, 2011 at 02:57:41AM +0200, Daniel Mack wrote:
>> On Thu, Aug 11, 2011 at 1:15 AM, Sarah Sharp
>> <sarah.a.sharp@linux.intel.com> wrote:
>> > On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
>> >> On 08/10/2011 04:32 PM, Alan Stern wrote:
>> >> >Looking at the driver's current code, it appears that your patch
>> >> >does not fix the bug properly.  Using discontiguous regions in the
>> >> >transfer buffer is perfectly okay.  The real problem is later on,
>> >> >where you do:
>> >> >
>> >> >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
>> >> >
>> >> >This should be
>> >> >
>> >> >out->number_of_packets = outframe;
>> >> >
>> >> >The way it is now, the USB stack will try to use data from all the
>> >> >frame descriptors, and the last few will be stale because the loop
>> >> >doesn't set them.
>> >>
>> >> That's actually true, even though it doesn't seem to cause any trouble.
>> >> I tested everything here of course, and the output URBs return back from
>> >> the USB stack with their length fields zeroed out, which then
>> >> causes the stack to send packets with zero-length fields at the end.
>> >
>> > Actually, it causes system hangs when the driver is loaded on a device
>> > attached to a USB 3.0 port, as Alan Stern pointed out:
>> >
>> > https://bugzilla.kernel.org/show_bug.cgi?id=40702
>>
>> Yes, I've noticed this.
>>
>> > Please don't submit zero-length transfers.  The xHCI driver just isn't
>> > able to handle it.  Arguably, it probably should have just rejected your
>> > URB when it found a zero length buffer, so I'll probably be submitting a
>> > patch to fix that.
>>
>> According to the spec, sending zero-length frames should be fine, no?
>> Is there any particular reason why XCHI can't handle this while EHCI
>> can? And does my patch fix the driver for XHCI?
>
> Ok, yes, you're correct that the xHCI spec allows the transfer length to
> be set to zero.  In the case where the frame buffer is zero-length, is
> the buffer pointer still valid?  It's not clear from the spec whether it
> needs to be.

Well, the buffer pointer is set in the URB, not in its individual iso
subframes which just denotes them via the offset field. So yes, it is
valid in my case. But it doesn't matter anymore, as the code which
does that is now gone :)


Daniel

Re: Allocating buffers for USB transfers (again)

[Sarah Sharp]

On Thu, Aug 11, 2011 at 07:27:15PM +0200, Daniel Mack wrote:
> On Thu, Aug 11, 2011 at 6:45 PM, Sarah Sharp
> <sarah.a.sharp@linux.intel.com> wrote:
> > On Thu, Aug 11, 2011 at 02:57:41AM +0200, Daniel Mack wrote:
> >> On Thu, Aug 11, 2011 at 1:15 AM, Sarah Sharp
> >> <sarah.a.sharp@linux.intel.com> wrote:
> >> > On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
> >> >> On 08/10/2011 04:32 PM, Alan Stern wrote:
> >> >> >Looking at the driver's current code, it appears that your patch
> >> >> >does not fix the bug properly.  Using discontiguous regions in the
> >> >> >transfer buffer is perfectly okay.  The real problem is later on,
> >> >> >where you do:
> >> >> >
> >> >> >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
> >> >> >
> >> >> >This should be
> >> >> >
> >> >> >out->number_of_packets = outframe;
> >> >> >
> >> >> >The way it is now, the USB stack will try to use data from all the
> >> >> >frame descriptors, and the last few will be stale because the loop
> >> >> >doesn't set them.
> >> >>
> >> >> That's actually true, even though it doesn't seem to cause any trouble.
> >> >> I tested everything here of course, and the output URBs return back from
> >> >> the USB stack with their length fields zeroed out, which then
> >> >> causes the stack to send packets with zero-length fields at the end.
> >> >
> >> > Actually, it causes system hangs when the driver is loaded on a device
> >> > attached to a USB 3.0 port, as Alan Stern pointed out:
> >> >
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=40702
> >>
> >> Yes, I've noticed this.
> >>
> >> > Please don't submit zero-length transfers.  The xHCI driver just isn't
> >> > able to handle it.  Arguably, it probably should have just rejected your
> >> > URB when it found a zero length buffer, so I'll probably be submitting a
> >> > patch to fix that.
> >>
> >> According to the spec, sending zero-length frames should be fine, no?
> >> Is there any particular reason why XCHI can't handle this while EHCI
> >> can? And does my patch fix the driver for XHCI?
> >
> > Ok, yes, you're correct that the xHCI spec allows the transfer length to
> > be set to zero.  In the case where the frame buffer is zero-length, is
> > the buffer pointer still valid?  It's not clear from the spec whether it
> > needs to be.
> 
> Well, the buffer pointer is set in the URB, not in its individual iso
> subframes which just denotes them via the offset field. So yes, it is
> valid in my case. But it doesn't matter anymore, as the code which
> does that is now gone :)

Do you mean it was removed with this patch:

http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=commitdiff;h=15439b

Because according to Matej, he applied that patch, plus my patch to
reject zero-length buffers[1], and he saw debugging that indicated he
*did* see zero-length buffers.  Is there any chance your driver might
submit a zero-length buffer in the middle of the isochronous URB
transfer array?

Sarah Sharp

[1]
http://git.kernel.org/?p=linux/kernel/git/sarah/xhci.git;a=commitdiff;h=e70d79bda63050729fcef654167d22eb59380aa6

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Thu, Aug 11, 2011 at 8:05 PM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Thu, Aug 11, 2011 at 07:27:15PM +0200, Daniel Mack wrote:
>> On Thu, Aug 11, 2011 at 6:45 PM, Sarah Sharp
>> <sarah.a.sharp@linux.intel.com> wrote:
>> > On Thu, Aug 11, 2011 at 02:57:41AM +0200, Daniel Mack wrote:
>> >> On Thu, Aug 11, 2011 at 1:15 AM, Sarah Sharp
>> >> <sarah.a.sharp@linux.intel.com> wrote:
>> >> > On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote:
>> >> >> On 08/10/2011 04:32 PM, Alan Stern wrote:
>> >> >> >Looking at the driver's current code, it appears that your patch
>> >> >> >does not fix the bug properly.  Using discontiguous regions in the
>> >> >> >transfer buffer is perfectly okay.  The real problem is later on,
>> >> >> >where you do:
>> >> >> >
>> >> >> >if (send_it) { out->number_of_packets = FRAMES_PER_URB;
>> >> >> >
>> >> >> >This should be
>> >> >> >
>> >> >> >out->number_of_packets = outframe;
>> >> >> >
>> >> >> >The way it is now, the USB stack will try to use data from all the
>> >> >> >frame descriptors, and the last few will be stale because the loop
>> >> >> >doesn't set them.
>> >> >>
>> >> >> That's actually true, even though it doesn't seem to cause any trouble.
>> >> >> I tested everything here of course, and the output URBs return back from
>> >> >> the USB stack with their length fields zeroed out, which then
>> >> >> causes the stack to send packets with zero-length fields at the end.
>> >> >
>> >> > Actually, it causes system hangs when the driver is loaded on a device
>> >> > attached to a USB 3.0 port, as Alan Stern pointed out:
>> >> >
>> >> > https://bugzilla.kernel.org/show_bug.cgi?id=40702
>> >>
>> >> Yes, I've noticed this.
>> >>
>> >> > Please don't submit zero-length transfers.  The xHCI driver just isn't
>> >> > able to handle it.  Arguably, it probably should have just rejected your
>> >> > URB when it found a zero length buffer, so I'll probably be submitting a
>> >> > patch to fix that.
>> >>
>> >> According to the spec, sending zero-length frames should be fine, no?
>> >> Is there any particular reason why XCHI can't handle this while EHCI
>> >> can? And does my patch fix the driver for XHCI?
>> >
>> > Ok, yes, you're correct that the xHCI spec allows the transfer length to
>> > be set to zero.  In the case where the frame buffer is zero-length, is
>> > the buffer pointer still valid?  It's not clear from the spec whether it
>> > needs to be.
>>
>> Well, the buffer pointer is set in the URB, not in its individual iso
>> subframes which just denotes them via the offset field. So yes, it is
>> valid in my case. But it doesn't matter anymore, as the code which
>> does that is now gone :)
>
> Do you mean it was removed with this patch:
>
> http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=commitdiff;h=15439b
>
> Because according to Matej, he applied that patch, plus my patch to
> reject zero-length buffers[1], and he saw debugging that indicated he
> *did* see zero-length buffers.  Is there any chance your driver might
> submit a zero-length buffer in the middle of the isochronous URB
> transfer array?

Hmm, judging from the code, this can only ever happen if we receive an
inbound iso frame which has a valid status and an actual_length of
zero. Also, it was not neccessary to catch this case for EHCI.

Maetj, does this patch make any difference?


Daniel



diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index aa52b3e..b48adb9 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -633,6 +634,10 @@ static void read_completed(struct urb *urb)
                        continue;

                len = urb->iso_frame_desc[outframe].actual_length;
+
+               if (len == 0)
+                       continue;
+
                out->iso_frame_desc[outframe].length = len;
                out->iso_frame_desc[outframe].actual_length = 0;
                out->iso_frame_desc[outframe].offset = offset;

Re: Allocating buffers for USB transfers (again)

[Matěj Laitl]

On 11. 8. 2011 Daniel Mack wrote:
> On Thu, Aug 11, 2011 at 8:05 PM, Sarah Sharp
> > Because according to Matej, he applied that patch, plus my patch to
> > reject zero-length buffers[1], and he saw debugging that indicated he
> > *did* see zero-length buffers.  Is there any chance your driver might
> > submit a zero-length buffer in the middle of the isochronous URB
> > transfer array?
> 
> Hmm, judging from the code, this can only ever happen if we receive an
> inbound iso frame which has a valid status and an actual_length of
> zero. Also, it was not neccessary to catch this case for EHCI.
> 
> Maetj, does this patch make any difference?

This patch actually makes the sound playback _worse_. Now I get strange 
squawks where previously at least first seconds of a song sounded normally.

However, I no longer get "zero length buffer submitted" or that "... Weird." 
debug messages, only several megabytes of: (should I post these somewhere?)

xhci_hcd 0000:05:00.0: Giveback URB ffff880114cec000, len = 880, expected = 
1000, status = -115
xhci_hcd 0000:05:00.0: Giveback URB ffff880114740000, len = 352, expected = 
1000, status = -115
xhci_hcd 0000:05:00.0: underrun event on endpoint

Regards,
                 Matej

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On 08/12/2011 01:29 AM, Matěj Laitl wrote:
> On 11. 8. 2011 Daniel Mack wrote:
>> On Thu, Aug 11, 2011 at 8:05 PM, Sarah Sharp
>>> Because according to Matej, he applied that patch, plus my patch to
>>> reject zero-length buffers[1], and he saw debugging that indicated he
>>> *did* see zero-length buffers.  Is there any chance your driver might
>>> submit a zero-length buffer in the middle of the isochronous URB
>>> transfer array?
>>
>> Hmm, judging from the code, this can only ever happen if we receive an
>> inbound iso frame which has a valid status and an actual_length of
>> zero. Also, it was not neccessary to catch this case for EHCI.
>>
>> Maetj, does this patch make any difference?
>
> This patch actually makes the sound playback _worse_. Now I get strange
> squawks where previously at least first seconds of a song sounded normally.
>
> However, I no longer get "zero length buffer submitted" or that "... Weird."
> debug messages, only several megabytes of: (should I post these somewhere?)
>
> xhci_hcd 0000:05:00.0: Giveback URB ffff880114cec000, len = 880, expected =
> 1000, status = -115
> xhci_hcd 0000:05:00.0: Giveback URB ffff880114740000, len = 352, expected =
> 1000, status = -115
> xhci_hcd 0000:05:00.0: underrun event on endpoint

Might be the hardware doesn't like this. As I said, the patch is blindly 
written and I couldn't test it. Did you test this on a EHCI port as well?

Daniel

Re: Allocating buffers for USB transfers (again)

[Matěj Laitl]

On 12. 8. 2011 Daniel Mack wrote:
> > This patch actually makes the sound playback _worse_. Now I get strange
> > squawks where previously at least first seconds of a song sounded
> > normally.
> > 
> > However, I no longer get "zero length buffer submitted" or that "...
> > Weird." debug messages, only several megabytes of: (should I post these
> > somewhere?)
> > 
> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114cec000, len = 880,
> > expected = 1000, status = -115
> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114740000, len = 352,
> > expected = 1000, status = -115
> > xhci_hcd 0000:05:00.0: underrun event on endpoint
> 
> Might be the hardware doesn't like this. As I said, the patch is blindly 
> written and I couldn't test it. Did you test this on a EHCI port as well?

Good point, this patch distorts the playback also on the EHCI port. (the 
playback seems to be 10 times slower than it should be)

Now I have to go, but I hope I'll get another patch from you tomorrow. :-)

Thanks,
               Matej

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Fri, Aug 12, 2011 at 1:50 AM, Matěj Laitl <matej@laitl.cz> wrote:
> On 12. 8. 2011 Daniel Mack wrote:
>> > This patch actually makes the sound playback _worse_. Now I get strange
>> > squawks where previously at least first seconds of a song sounded
>> > normally.
>> >
>> > However, I no longer get "zero length buffer submitted" or that "...
>> > Weird." debug messages, only several megabytes of: (should I post these
>> > somewhere?)
>> >
>> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114cec000, len = 880,
>> > expected = 1000, status = -115
>> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114740000, len = 352,
>> > expected = 1000, status = -115
>> > xhci_hcd 0000:05:00.0: underrun event on endpoint
>>
>> Might be the hardware doesn't like this. As I said, the patch is blindly
>> written and I couldn't test it. Did you test this on a EHCI port as well?
>
> Good point, this patch distorts the playback also on the EHCI port. (the
> playback seems to be 10 times slower than it should be)

And it is only this *last* patch that causes it, yes?

If that is the case, we need empty subframes in the middle of urbs,
and we need to figure a way how to teach the XHCI driver to cope with
that, too.


Daniel

Re: Allocating buffers for USB transfers (again)

[Sarah Sharp]

On Fri, Aug 12, 2011 at 03:28:42AM +0200, Daniel Mack wrote:
> On Fri, Aug 12, 2011 at 1:50 AM, Matěj Laitl <matej@laitl.cz> wrote:
> > On 12. 8. 2011 Daniel Mack wrote:
> >> > This patch actually makes the sound playback _worse_. Now I get strange
> >> > squawks where previously at least first seconds of a song sounded
> >> > normally.
> >> >
> >> > However, I no longer get "zero length buffer submitted" or that "...
> >> > Weird." debug messages, only several megabytes of: (should I post these
> >> > somewhere?)
> >> >
> >> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114cec000, len = 880,
> >> > expected = 1000, status = -115
> >> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114740000, len = 352,
> >> > expected = 1000, status = -115
> >> > xhci_hcd 0000:05:00.0: underrun event on endpoint
> >>
> >> Might be the hardware doesn't like this. As I said, the patch is blindly
> >> written and I couldn't test it. Did you test this on a EHCI port as well?
> >
> > Good point, this patch distorts the playback also on the EHCI port. (the
> > playback seems to be 10 times slower than it should be)
> 
> And it is only this *last* patch that causes it, yes?
> 
> If that is the case, we need empty subframes in the middle of urbs,
> and we need to figure a way how to teach the XHCI driver to cope with
> that, too.

By empty subframes, you mean zero-length frame buffers in the middle of
the URB, correct?  Andiry and Alan had some suggestions for how to fix
that, so I'll try to cook up a patch tomorrow.  Then we'll have to
"unpeel" the debug patches and make sure the system is still usable.

Sarah Sharp

Re: Allocating buffers for USB transfers (again)

[Daniel Mack]

On Fri, Aug 12, 2011 at 6:46 AM, Sarah Sharp
<sarah.a.sharp@linux.intel.com> wrote:
> On Fri, Aug 12, 2011 at 03:28:42AM +0200, Daniel Mack wrote:
>> On Fri, Aug 12, 2011 at 1:50 AM, Matěj Laitl <matej@laitl.cz> wrote:
>> > On 12. 8. 2011 Daniel Mack wrote:
>> >> > This patch actually makes the sound playback _worse_. Now I get strange
>> >> > squawks where previously at least first seconds of a song sounded
>> >> > normally.
>> >> >
>> >> > However, I no longer get "zero length buffer submitted" or that "...
>> >> > Weird." debug messages, only several megabytes of: (should I post these
>> >> > somewhere?)
>> >> >
>> >> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114cec000, len = 880,
>> >> > expected = 1000, status = -115
>> >> > xhci_hcd 0000:05:00.0: Giveback URB ffff880114740000, len = 352,
>> >> > expected = 1000, status = -115
>> >> > xhci_hcd 0000:05:00.0: underrun event on endpoint
>> >>
>> >> Might be the hardware doesn't like this. As I said, the patch is blindly
>> >> written and I couldn't test it. Did you test this on a EHCI port as well?
>> >
>> > Good point, this patch distorts the playback also on the EHCI port. (the
>> > playback seems to be 10 times slower than it should be)
>>
>> And it is only this *last* patch that causes it, yes?
>>
>> If that is the case, we need empty subframes in the middle of urbs,
>> and we need to figure a way how to teach the XHCI driver to cope with
>> that, too.
>
> By empty subframes, you mean zero-length frame buffers in the middle of
> the URB, correct?

Yep.

> Andiry and Alan had some suggestions for how to fix
> that, so I'll try to cook up a patch tomorrow.  Then we'll have to
> "unpeel" the debug patches and make sure the system is still usable.

Good :) And I'll make a patch to make double-submission impossible in
a nice way.


Daniel