Re: Allocating buffers for USB transfers (again)

From: Daniel Mack
Date: Wed Aug 10 2011 - 03:51:30 EST

On Thu, Jul 7, 2011 at 5:16 PM, Florian Mickler <florian@xxxxxxxxxxx> wrote:
> [adding Robert Hancock]
> see here for the whole thread:
> 2011/7/7 Daniel Mack <zonque@xxxxxxxxx>:
>> On Thu, Jul 7, 2011 at 2:33 PM, Oliver Neukum <oliver@xxxxxxxxxx> 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 :
> 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

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.

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

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@xxxxxxxxx>
Reported-and-tested-by: William Light <wrl@xxxxxxxxxx>
Reported-by: Pedro Ribeiro <pedrib@xxxxxxxxx>
Cc: stable@xxxxxxxxxx
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)
@@ -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) {