Re: USB transfer_buffer allocations on 64bit systems

Re: USB transfer_buffer allocations on 64bit systems

[Daniel Mack]

On Tue, Apr 20, 2010 at 01:16:58AM +0100, Pedro Ribeiro wrote:
> On 15 April 2010 16:20, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 15 Apr 2010, Pedro Ribeiro wrote:
> >
> >> I enabled  CONFIG_DMAR_BROKEN_GFX_WA=y and the result is the same. A
> >> delay in the boot process and usb devices don't work properly,
> >> including my USB mouse.
> >>
> >> Strange, since you have the same platform as me. The extra usb devices
> >> you were seeing are because of my docking station - but it makes no
> >> difference whether I'm docked or not for the purposes of the original
> >> bug or this situation right now. The dmesg I'm attaching is without
> >> the computer being docked.
> >
> > It's not possible to determine the reason for the timeout errors
> > between timestamps 16 and 53 from the small amount of debugging
> > information in the log.  Clearly something is going wrong with the
> > communication between the computer and the EHCI controller.  And
> > clearly the kernel config changes are responsible.
> >
> > But I don't know what to do to track it down any farther.
> >
> > Alan Stern
> >
> >
> 
> I guess this is pretty much a dead end until anyone else can reproduce it!

Hmm, I think I finally found the reason for all this confusion. No idea
why I didn't come up with the following explanation any earlier.

The problem is again (summarized):

On 64bit machines, with 4GB or more, the allocated buffers for USB
transfers might be beyond the 32bit boundary. In this case, the IOMMU
should take care and install DMA bounce buffer to copy over the buffer
before the transfer actually happens. The problem is, however, that this
copy mechanism takes place when the URB with its associated buffer is
submitted, not when the EHCI will actually do the transfer.

In the particular case of audio drivers, though, the contents of the
buffers are likely to change after the submission. What we do here
is that we map the audio stream buffers which are used by ALSA to
the output URBs, so they're filled asychronously. Once the buffer is
actually sent out on the bus, it is believed to contain proper audio
date. If it doesn't, that's due to too tight audio timing or other
problems. This breaks once buffers are magically bounced in the
background.

So - long story short: these audio buffers need to be DMA coherent.

The patch below does that, and Pedro excessively tested this patch for
weeks now :) It was just the final explanation _why_ it does the right
thing that was yet missing.

If nobody has objections, can we still squeeze it into .34?

Thanks,
Daniel


>From 5672ca44b9b4617f6c29c88409da13b1bf475547 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Wed, 7 Apr 2010 01:03:09 +0200
Subject: [PATCH] ALSA: usb/caiaq: use usb_buffer_alloc()

Use usb_buffer_alloc() and usb_buffer_free() for transfer buffers.
We need DMA-coherent memory in this case as buffer contents are likely
to be modified after the URB was submitted, because the URB buffers
are mapped to the audio streams.

On x86_64, buffers allocated with kmalloc() may be beyond the boundaries
of 32bit accessible memory, and DMA bounce buffers will live at other
locations, unaccessible by the driver, and hence outside of the audio
buffer mapping.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Tested-by: Pedro Ribeiro <pedrib@gmail.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg KH <gregkh@suse.de>
Cc: iommu@lists.linux-foundation.org
Cc: Kernel development list <linux-kernel@vger.kernel.org>
Cc: USB list <linux-usb@vger.kernel.org>
Cc: stable@kernel.org
---
 sound/usb/caiaq/audio.c |   57 ++++++++++++++++++++++++++--------------------
 1 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 4328cad..adbeefd 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -552,46 +552,47 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev *dev, int dir, int *ret)
 	}
 
 	for (i = 0; i < N_URBS; i++) {
-		urbs[i] = usb_alloc_urb(FRAMES_PER_URB, GFP_KERNEL);
-		if (!urbs[i]) {
+		struct urb *u = usb_alloc_urb(FRAMES_PER_URB, GFP_KERNEL);
+		if (!u) {
 			log("unable to usb_alloc_urb(), OOM!?\n");
 			*ret = -ENOMEM;
 			return urbs;
 		}
 
-		urbs[i]->transfer_buffer =
-			kmalloc(FRAMES_PER_URB * BYTES_PER_FRAME, GFP_KERNEL);
-		if (!urbs[i]->transfer_buffer) {
-			log("unable to kmalloc() transfer buffer, OOM!?\n");
+		urbs[i] = u;
+		u->dev = usb_dev;
+		u->pipe = pipe;
+		u->transfer_buffer_length =
+				FRAMES_PER_URB * BYTES_PER_FRAME;
+		u->context = &dev->data_cb_info[i];
+		u->interval = 1;
+		u->transfer_flags = URB_ISO_ASAP;
+		u->number_of_packets = FRAMES_PER_URB;
+		u->complete = (dir == SNDRV_PCM_STREAM_CAPTURE) ?
+					read_completed : write_completed;
+		u->transfer_buffer = usb_alloc_coherent(usb_dev,
+							u->transfer_buffer_length,
+							GFP_KERNEL, &u->transfer_dma);
+		if (!u->transfer_buffer) {
+			log("usb_alloc_coherent() failed, OOM!?\n");
 			*ret = -ENOMEM;
 			return urbs;
 		}
 
 		for (frame = 0; frame < FRAMES_PER_URB; frame++) {
 			struct usb_iso_packet_descriptor *iso =
-				&urbs[i]->iso_frame_desc[frame];
+					&u->iso_frame_desc[frame];
 
 			iso->offset = BYTES_PER_FRAME * frame;
 			iso->length = BYTES_PER_FRAME;
 		}
-
-		urbs[i]->dev = usb_dev;
-		urbs[i]->pipe = pipe;
-		urbs[i]->transfer_buffer_length = FRAMES_PER_URB
-						* BYTES_PER_FRAME;
-		urbs[i]->context = &dev->data_cb_info[i];
-		urbs[i]->interval = 1;
-		urbs[i]->transfer_flags = URB_ISO_ASAP;
-		urbs[i]->number_of_packets = FRAMES_PER_URB;
-		urbs[i]->complete = (dir == SNDRV_PCM_STREAM_CAPTURE) ?
-					read_completed : write_completed;
 	}
 
 	*ret = 0;
 	return urbs;
 }
 
-static void free_urbs(struct urb **urbs)
+static void free_urbs(struct usb_device *usb_dev, struct urb **urbs)
 {
 	int i;
 
@@ -603,7 +604,10 @@ static void free_urbs(struct urb **urbs)
 			continue;
 
 		usb_kill_urb(urbs[i]);
-		kfree(urbs[i]->transfer_buffer);
+		usb_free_coherent(usb_dev,
+				  urbs[i]->transfer_buffer_length,
+				  urbs[i]->transfer_buffer,
+				  urbs[i]->transfer_dma);
 		usb_free_urb(urbs[i]);
 	}
 
@@ -613,6 +617,7 @@ static void free_urbs(struct urb **urbs)
 int snd_usb_caiaq_audio_init(struct snd_usb_caiaqdev *dev)
 {
 	int i, ret;
+	struct usb_device *usb_dev = dev->chip.dev;
 
 	dev->n_audio_in  = max(dev->spec.num_analog_audio_in,
 			       dev->spec.num_digital_audio_in) /
@@ -689,15 +694,15 @@ int snd_usb_caiaq_audio_init(struct snd_usb_caiaqdev *dev)
 	dev->data_urbs_in = alloc_urbs(dev, SNDRV_PCM_STREAM_CAPTURE, &ret);
 	if (ret < 0) {
 		kfree(dev->data_cb_info);
-		free_urbs(dev->data_urbs_in);
+		free_urbs(usb_dev, dev->data_urbs_in);
 		return ret;
 	}
 
 	dev->data_urbs_out = alloc_urbs(dev, SNDRV_PCM_STREAM_PLAYBACK, &ret);
 	if (ret < 0) {
 		kfree(dev->data_cb_info);
-		free_urbs(dev->data_urbs_in);
-		free_urbs(dev->data_urbs_out);
+		free_urbs(usb_dev, dev->data_urbs_in);
+		free_urbs(usb_dev, dev->data_urbs_out);
 		return ret;
 	}
 
@@ -706,10 +711,12 @@ int snd_usb_caiaq_audio_init(struct snd_usb_caiaqdev *dev)
 
 void snd_usb_caiaq_audio_free(struct snd_usb_caiaqdev *dev)
 {
+	struct usb_device *usb_dev = dev->chip.dev;
+
 	debug("%s(%p)\n", __func__, dev);
 	stream_stop(dev);
-	free_urbs(dev->data_urbs_in);
-	free_urbs(dev->data_urbs_out);
+	free_urbs(usb_dev, dev->data_urbs_in);
+	free_urbs(usb_dev, dev->data_urbs_out);
 	kfree(dev->data_cb_info);
 }
 
-- 
1.7.1

Re: USB transfer_buffer allocations on 64bit systems

[Clemens Ladisch]

Daniel Mack wrote:
> The problem is again (summarized):
> 
> On 64bit machines, with 4GB or more, the allocated buffers for USB
> transfers might be beyond the 32bit boundary. In this case, the IOMMU
> should take care and install DMA bounce buffer to copy over the buffer
> before the transfer actually happens. The problem is, however, that this
> copy mechanism takes place when the URB with its associated buffer is
> submitted, not when the EHCI will actually do the transfer.
> 
> In the particular case of audio drivers, though, the contents of the
> buffers are likely to change after the submission. What we do here
> is that we map the audio stream buffers which are used by ALSA to
> the output URBs, so they're filled asychronously. Once the buffer is
> actually sent out on the bus, it is believed to contain proper audio
> date. If it doesn't, that's due to too tight audio timing or other
> problems. This breaks once buffers are magically bounced in the
> background.

At least the audio class and ua101 drivers don't do this and fill the
buffers before they are submitted.

> So - long story short: these audio buffers need to be DMA coherent.

Does the USB API actually guarantee that all controllers use DMA, i.e.,
that the buffers can be filled after submission?


Regards,
Clemens

Re: USB transfer_buffer allocations on 64bit systems

[Daniel Mack]

On Fri, May 07, 2010 at 11:47:37AM +0200, Clemens Ladisch wrote:
> Daniel Mack wrote:
> > The problem is again (summarized):
> > 
> > On 64bit machines, with 4GB or more, the allocated buffers for USB
> > transfers might be beyond the 32bit boundary. In this case, the IOMMU
> > should take care and install DMA bounce buffer to copy over the buffer
> > before the transfer actually happens. The problem is, however, that this
> > copy mechanism takes place when the URB with its associated buffer is
> > submitted, not when the EHCI will actually do the transfer.
> > 
> > In the particular case of audio drivers, though, the contents of the
> > buffers are likely to change after the submission. What we do here
> > is that we map the audio stream buffers which are used by ALSA to
> > the output URBs, so they're filled asychronously. Once the buffer is
> > actually sent out on the bus, it is believed to contain proper audio
> > date. If it doesn't, that's due to too tight audio timing or other
> > problems. This breaks once buffers are magically bounced in the
> > background.
> 
> At least the audio class and ua101 drivers don't do this and fill the
> buffers before they are submitted.

Gnaa, you're right. I _thought_ my code does it the way I described, but
what I wrote is how I _wanted_ to do it, not how it's currently done. I
have a plan to change this in the future.

So unfortunately, that doesn't explain it either. Sorry for the noise.

Daniel

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Alan Stern]

On Fri, 7 May 2010, Daniel Mack wrote:

> > At least the audio class and ua101 drivers don't do this and fill the
> > buffers before they are submitted.
> 
> Gnaa, you're right. I _thought_ my code does it the way I described, but
> what I wrote is how I _wanted_ to do it, not how it's currently done. I
> have a plan to change this in the future.
> 
> So unfortunately, that doesn't explain it either. Sorry for the noise.

At one point we tried an experiment, printing out the buffer and DMA 
addresses.  I don't recall seeing anything obviously wrong, but if an 
IOMMU was in use then that might not mean anything.  Is it possible 
that the IOMMU mappings sometimes get messed up for addresses above 4 
GB?

Alan Stern

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[FUJITA Tomonori]

On Fri, 7 May 2010 10:51:10 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Fri, 7 May 2010, Daniel Mack wrote:
> 
> > > At least the audio class and ua101 drivers don't do this and fill the
> > > buffers before they are submitted.
> > 
> > Gnaa, you're right. I _thought_ my code does it the way I described, but
> > what I wrote is how I _wanted_ to do it, not how it's currently done. I
> > have a plan to change this in the future.
> > 
> > So unfortunately, that doesn't explain it either. Sorry for the noise.
> 
> At one point we tried an experiment, printing out the buffer and DMA 
> addresses.  I don't recall seeing anything obviously wrong, but if an 
> IOMMU was in use then that might not mean anything.  Is it possible 
> that the IOMMU mappings sometimes get messed up for addresses above 4 
> GB?

You mean that an IOMMU could allocate an address above 4GB wrongly? If
so, IIRC, all the IOMMU implementations use dev->dma_mask and
dev->coherent_dma_mask properly. And the DMA address space of the
majority of IOMMUs are limited less than 4GB.

Re: USB transfer_buffer allocations on 64bit systems

[David Woodhouse]

On Mon, 2010-05-10 at 11:50 +0900, FUJITA Tomonori wrote:
> On Fri, 7 May 2010 10:51:10 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Fri, 7 May 2010, Daniel Mack wrote:
> > 
> > > > At least the audio class and ua101 drivers don't do this and fill the
> > > > buffers before they are submitted.
> > > 
> > > Gnaa, you're right. I _thought_ my code does it the way I described, but
> > > what I wrote is how I _wanted_ to do it, not how it's currently done. I
> > > have a plan to change this in the future.
> > > 
> > > So unfortunately, that doesn't explain it either. Sorry for the noise.
> > 
> > At one point we tried an experiment, printing out the buffer and DMA 
> > addresses.  I don't recall seeing anything obviously wrong, but if an 
> > IOMMU was in use then that might not mean anything.  Is it possible 
> > that the IOMMU mappings sometimes get messed up for addresses above 4 
> > GB?
> 
> You mean that an IOMMU could allocate an address above 4GB wrongly? If
> so, IIRC, all the IOMMU implementations use dev->dma_mask and
> dev->coherent_dma_mask properly. And the DMA address space of the
> majority of IOMMUs are limited less than 4GB.

The Intel IOMMU code will use dev->dma_mask and dev->coherent_dma_mask
properly. It is not limited to 4GiB, but it will tend to give virtual
DMA addresses below 4GiB even when a device is capable of more; it'll
only give out higher addresses when the address space below 4GiB is
exhausted.

-- 
dwmw2

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Alan Stern]

On Mon, 10 May 2010, David Woodhouse wrote:

> On Mon, 2010-05-10 at 11:50 +0900, FUJITA Tomonori wrote:
> > On Fri, 7 May 2010 10:51:10 -0400 (EDT)
> > Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > 
> > > On Fri, 7 May 2010, Daniel Mack wrote:
> > > 
> > > > > At least the audio class and ua101 drivers don't do this and fill the
> > > > > buffers before they are submitted.
> > > > 
> > > > Gnaa, you're right. I _thought_ my code does it the way I described, but
> > > > what I wrote is how I _wanted_ to do it, not how it's currently done. I
> > > > have a plan to change this in the future.
> > > > 
> > > > So unfortunately, that doesn't explain it either. Sorry for the noise.
> > > 
> > > At one point we tried an experiment, printing out the buffer and DMA 
> > > addresses.  I don't recall seeing anything obviously wrong, but if an 
> > > IOMMU was in use then that might not mean anything.  Is it possible 
> > > that the IOMMU mappings sometimes get messed up for addresses above 4 
> > > GB?
> > 
> > You mean that an IOMMU could allocate an address above 4GB wrongly? If
> > so, IIRC, all the IOMMU implementations use dev->dma_mask and
> > dev->coherent_dma_mask properly. And the DMA address space of the
> > majority of IOMMUs are limited less than 4GB.
> 
> The Intel IOMMU code will use dev->dma_mask and dev->coherent_dma_mask
> properly. It is not limited to 4GiB, but it will tend to give virtual
> DMA addresses below 4GiB even when a device is capable of more; it'll
> only give out higher addresses when the address space below 4GiB is
> exhausted.

What I meant was: Is it possible that the IOMMU code will return a
virtual DMA address before 4 GB but will somehow forget to actually map
that address to the data buffer?

The problem goes away when Pedro boots with mem=4G.  And the dma_mask
value is set properly (in fact, the ehci-hcd driver currently doesn't
use 64-bit DMA at all).

If anyone wants to see the debug log entries showing the buffer and DMA
addresses, they are attached to this email message:

	http://marc.info/?l=linux-kernel&m=127076841801054&w=2

Either the data isn't getting written to the buffer correctly or else
the buffer isn't getting sent to the device correctly.  Can anybody
suggest a means of determining which is the case?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[FUJITA Tomonori]

On Mon, 10 May 2010 10:58:37 -0400 (EDT)
Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:

> On Mon, 10 May 2010, David Woodhouse wrote:
> 
> > On Mon, 2010-05-10 at 11:50 +0900, FUJITA Tomonori wrote:
> > > On Fri, 7 May 2010 10:51:10 -0400 (EDT)
> > > Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > > 
> > > > On Fri, 7 May 2010, Daniel Mack wrote:
> > > > 
> > > > > > At least the audio class and ua101 drivers don't do this and fill the
> > > > > > buffers before they are submitted.
> > > > > 
> > > > > Gnaa, you're right. I _thought_ my code does it the way I described, but
> > > > > what I wrote is how I _wanted_ to do it, not how it's currently done. I
> > > > > have a plan to change this in the future.
> > > > > 
> > > > > So unfortunately, that doesn't explain it either. Sorry for the noise.
> > > > 
> > > > At one point we tried an experiment, printing out the buffer and DMA 
> > > > addresses.  I don't recall seeing anything obviously wrong, but if an 
> > > > IOMMU was in use then that might not mean anything.  Is it possible 
> > > > that the IOMMU mappings sometimes get messed up for addresses above 4 
> > > > GB?
> > > 
> > > You mean that an IOMMU could allocate an address above 4GB wrongly? If
> > > so, IIRC, all the IOMMU implementations use dev->dma_mask and
> > > dev->coherent_dma_mask properly. And the DMA address space of the
> > > majority of IOMMUs are limited less than 4GB.
> > 
> > The Intel IOMMU code will use dev->dma_mask and dev->coherent_dma_mask
> > properly. It is not limited to 4GiB, but it will tend to give virtual
> > DMA addresses below 4GiB even when a device is capable of more; it'll
> > only give out higher addresses when the address space below 4GiB is
> > exhausted.
> 
> What I meant was: Is it possible that the IOMMU code will return a
> virtual DMA address before 4 GB but will somehow forget to actually map
> that address to the data buffer?

Then, the IOMMU is completely broken. Then, we would get tons of DMA
bugs not only about USB, I guess. So I'm not sure.


> The problem goes away when Pedro boots with mem=4G.  And the dma_mask
> value is set properly (in fact, the ehci-hcd driver currently doesn't
> use 64-bit DMA at all).
> 
> If anyone wants to see the debug log entries showing the buffer and DMA
> addresses, they are attached to this email message:
> 
> 	http://marc.info/?l=linux-kernel&m=127076841801054&w=2
> 
> Either the data isn't getting written to the buffer correctly or else
> the buffer isn't getting sent to the device correctly.  Can anybody
> suggest a means of determining which is the case?

I can't say anything about this log that including only DMA addresses.
I'm not familiar with how the USB core does DMA stuff. And the USB
stack design that the USB core does DMA stuff (allocating, mappings,
etc) makes debugging DMA issues really difficult.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Alan Stern]

On Tue, 11 May 2010, FUJITA Tomonori wrote:

> > > > > At one point we tried an experiment, printing out the buffer and DMA 
> > > > > addresses.  I don't recall seeing anything obviously wrong, but if an 
> > > > > IOMMU was in use then that might not mean anything.  Is it possible 
> > > > > that the IOMMU mappings sometimes get messed up for addresses above 4 
> > > > > GB?
> > > > 
> > > > You mean that an IOMMU could allocate an address above 4GB wrongly? If
> > > > so, IIRC, all the IOMMU implementations use dev->dma_mask and
> > > > dev->coherent_dma_mask properly. And the DMA address space of the
> > > > majority of IOMMUs are limited less than 4GB.
> > > 
> > > The Intel IOMMU code will use dev->dma_mask and dev->coherent_dma_mask
> > > properly. It is not limited to 4GiB, but it will tend to give virtual
> > > DMA addresses below 4GiB even when a device is capable of more; it'll
> > > only give out higher addresses when the address space below 4GiB is
> > > exhausted.
> > 
> > What I meant was: Is it possible that the IOMMU code will return a
> > virtual DMA address before 4 GB but will somehow forget to actually map
> > that address to the data buffer?
> 
> Then, the IOMMU is completely broken. Then, we would get tons of DMA
> bugs not only about USB, I guess. So I'm not sure.

Yes, you're right about that.

> > The problem goes away when Pedro boots with mem=4G.  And the dma_mask
> > value is set properly (in fact, the ehci-hcd driver currently doesn't
> > use 64-bit DMA at all).
> > 
> > If anyone wants to see the debug log entries showing the buffer and DMA
> > addresses, they are attached to this email message:
> > 
> > 	http://marc.info/?l=linux-kernel&m=127076841801054&w=2
> > 
> > Either the data isn't getting written to the buffer correctly or else
> > the buffer isn't getting sent to the device correctly.  Can anybody
> > suggest a means of determining which is the case?
> 
> I can't say anything about this log that including only DMA addresses.
> I'm not familiar with how the USB core does DMA stuff. And the USB
> stack design that the USB core does DMA stuff (allocating, mappings,
> etc) makes debugging DMA issues really difficult.

The DMA stuff is simple enough in this case.  The urb->transfer_buffer
address is passed to dma_map_single(), and the DMA address it returns
is stored in urb->transfer_dma.  Those are the two values printed out
by the debugging patch.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[FUJITA Tomonori]

On Tue, 11 May 2010 10:00:50 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> > > The problem goes away when Pedro boots with mem=4G.  And the dma_mask
> > > value is set properly (in fact, the ehci-hcd driver currently doesn't
> > > use 64-bit DMA at all).
> > > 
> > > If anyone wants to see the debug log entries showing the buffer and DMA
> > > addresses, they are attached to this email message:
> > > 
> > > 	http://marc.info/?l=linux-kernel&m=127076841801054&w=2
> > > 
> > > Either the data isn't getting written to the buffer correctly or else
> > > the buffer isn't getting sent to the device correctly.  Can anybody
> > > suggest a means of determining which is the case?
> > 
> > I can't say anything about this log that including only DMA addresses.
> > I'm not familiar with how the USB core does DMA stuff. And the USB
> > stack design that the USB core does DMA stuff (allocating, mappings,
> > etc) makes debugging DMA issues really difficult.
> 
> The DMA stuff is simple enough in this case.  The urb->transfer_buffer
> address is passed to dma_map_single(), and the DMA address it returns
> is stored in urb->transfer_dma.  Those are the two values printed out
> by the debugging patch.

You've already confirmed that all the returned addresses
(urb->transfer_dma) in the log is less than 4GB, right?

Drivers need to take care of more things (not only about addresses) to
do DMA properly, that is, use the DMA API in the proper way (such as,
once calling dma_map_single, drivers can't touch the buffer until
calling dma_unmap_single)

Using the DMA API and performing DMA transfer in different places just
make things complicated.

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Konrad Rzeszutek Wilk]

> > > Either the data isn't getting written to the buffer correctly or else
> > > the buffer isn't getting sent to the device correctly.  Can anybody
> > > suggest a means of determining which is the case?
> > 
> > I can't say anything about this log that including only DMA addresses.
> > I'm not familiar with how the USB core does DMA stuff. And the USB
> > stack design that the USB core does DMA stuff (allocating, mappings,
> > etc) makes debugging DMA issues really difficult.
> 
> The DMA stuff is simple enough in this case.  The urb->transfer_buffer
> address is passed to dma_map_single(), and the DMA address it returns
> is stored in urb->transfer_dma.  Those are the two values printed out
> by the debugging patch.

Is that address (urb->transfer_dma) the same as 'virt_to_phys(urb->transfer_buffer)'
(if not, then SWIOTLB is being utilized) and is the dma_sync_* done on the
urb->transfer_dma (to properly sync the data from the SWIOTLB to the
transfer_buffer) before you start using the urb->transfer_buffer?

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[FUJITA Tomonori]

On Tue, 11 May 2010 10:24:40 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> > > > Either the data isn't getting written to the buffer correctly or else
> > > > the buffer isn't getting sent to the device correctly.  Can anybody
> > > > suggest a means of determining which is the case?
> > > 
> > > I can't say anything about this log that including only DMA addresses.
> > > I'm not familiar with how the USB core does DMA stuff. And the USB
> > > stack design that the USB core does DMA stuff (allocating, mappings,
> > > etc) makes debugging DMA issues really difficult.
> > 
> > The DMA stuff is simple enough in this case.  The urb->transfer_buffer
> > address is passed to dma_map_single(), and the DMA address it returns
> > is stored in urb->transfer_dma.  Those are the two values printed out
> > by the debugging patch.
> 
> Is that address (urb->transfer_dma) the same as 'virt_to_phys(urb->transfer_buffer)'
> (if not, then SWIOTLB is being utilized) and is the dma_sync_* done on the
> urb->transfer_dma (to properly sync the data from the SWIOTLB to the
> transfer_buffer) before you start using the urb->transfer_buffer?

Or calling dma_unmap_single.

Can you tell me all the exact process of DMA that the usb core and the
driver do?

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Alan Stern]

On Tue, 11 May 2010, FUJITA Tomonori wrote:

> On Tue, 11 May 2010 10:24:40 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > > > > Either the data isn't getting written to the buffer correctly or else
> > > > > the buffer isn't getting sent to the device correctly.  Can anybody
> > > > > suggest a means of determining which is the case?
> > > > 
> > > > I can't say anything about this log that including only DMA addresses.
> > > > I'm not familiar with how the USB core does DMA stuff. And the USB
> > > > stack design that the USB core does DMA stuff (allocating, mappings,
> > > > etc) makes debugging DMA issues really difficult.
> > > 
> > > The DMA stuff is simple enough in this case.  The urb->transfer_buffer
> > > address is passed to dma_map_single(), and the DMA address it returns
> > > is stored in urb->transfer_dma.  Those are the two values printed out
> > > by the debugging patch.
> > 
> > Is that address (urb->transfer_dma) the same as 'virt_to_phys(urb->transfer_buffer)'
> > (if not, then SWIOTLB is being utilized) and is the dma_sync_* done on the
> > urb->transfer_dma (to properly sync the data from the SWIOTLB to the
> > transfer_buffer) before you start using the urb->transfer_buffer?
> 
> Or calling dma_unmap_single.
> 
> Can you tell me all the exact process of DMA that the usb core and the
> driver do?

1. The audio driver stores data in urb->transfer_buffer.

2. The audio driver calls usb_submit_urb(urb).

3. The USB core does
		urb->transfer_dma = dma_map_single(controller, 
			urb->transfer_buffer,
			urb->transfer_buffer_length,
			DMA_TO_DEVICE);

4. The host controller driver tells the hardware to carry out the data 
   transfer.

5. When the hardware says the transfer is finished, the USB core does
		dma_unmap_single(controller,
			urb->transfer_dma,
			urb->transfer_buffer_length,
			DMA_TO_DEVICE);

6. The audio driver's completion handler is called:
		(urb->complete)(urb);

Alan Stern

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[FUJITA Tomonori]

On Tue, 11 May 2010 11:04:55 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 11 May 2010, FUJITA Tomonori wrote:
> 
> > On Tue, 11 May 2010 10:24:40 -0400
> > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > 
> > > > > > Either the data isn't getting written to the buffer correctly or else
> > > > > > the buffer isn't getting sent to the device correctly.  Can anybody
> > > > > > suggest a means of determining which is the case?
> > > > > 
> > > > > I can't say anything about this log that including only DMA addresses.
> > > > > I'm not familiar with how the USB core does DMA stuff. And the USB
> > > > > stack design that the USB core does DMA stuff (allocating, mappings,
> > > > > etc) makes debugging DMA issues really difficult.
> > > > 
> > > > The DMA stuff is simple enough in this case.  The urb->transfer_buffer
> > > > address is passed to dma_map_single(), and the DMA address it returns
> > > > is stored in urb->transfer_dma.  Those are the two values printed out
> > > > by the debugging patch.
> > > 
> > > Is that address (urb->transfer_dma) the same as 'virt_to_phys(urb->transfer_buffer)'
> > > (if not, then SWIOTLB is being utilized) and is the dma_sync_* done on the
> > > urb->transfer_dma (to properly sync the data from the SWIOTLB to the
> > > transfer_buffer) before you start using the urb->transfer_buffer?
> > 
> > Or calling dma_unmap_single.
> > 
> > Can you tell me all the exact process of DMA that the usb core and the
> > driver do?
> 
> 1. The audio driver stores data in urb->transfer_buffer.

How urb->transfer_buffer is allocated?


> 2. The audio driver calls usb_submit_urb(urb).
> 
> 3. The USB core does
> 		urb->transfer_dma = dma_map_single(controller, 
> 			urb->transfer_buffer,
> 			urb->transfer_buffer_length,
> 			DMA_TO_DEVICE);

The mapping error is checked via dma_mapping_error? Even if an mapping
error happens, no data corruption happens due to that (needs
something like retrying the request)?


> 4. The host controller driver tells the hardware to carry out the data 
>    transfer.
> 
> 5. When the hardware says the transfer is finished, the USB core does
> 		dma_unmap_single(controller,
> 			urb->transfer_dma,
> 			urb->transfer_buffer_length,
> 			DMA_TO_DEVICE);
> 
> 6. The audio driver's completion handler is called:
> 		(urb->complete)(urb);

The driver does only DMA_TO_DEVICE? Or you see DMA problems only with
DMA_TO_DEVICE?

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Konrad Rzeszutek Wilk]

On Fri, May 07, 2010 at 12:24:08PM +0200, Daniel Mack wrote:
> On Fri, May 07, 2010 at 11:47:37AM +0200, Clemens Ladisch wrote:
> > Daniel Mack wrote:
> > > The problem is again (summarized):
> > > 
> > > On 64bit machines, with 4GB or more, the allocated buffers for USB
> > > transfers might be beyond the 32bit boundary. In this case, the IOMMU
> > > should take care and install DMA bounce buffer to copy over the buffer
> > > before the transfer actually happens. The problem is, however, that this
> > > copy mechanism takes place when the URB with its associated buffer is
> > > submitted, not when the EHCI will actually do the transfer.
> > > 
> > > In the particular case of audio drivers, though, the contents of the
> > > buffers are likely to change after the submission. What we do here
> > > is that we map the audio stream buffers which are used by ALSA to
> > > the output URBs, so they're filled asychronously. Once the buffer is
> > > actually sent out on the bus, it is believed to contain proper audio
> > > date. If it doesn't, that's due to too tight audio timing or other
> > > problems. This breaks once buffers are magically bounced in the
> > > background.
> > 
> > At least the audio class and ua101 drivers don't do this and fill the
> > buffers before they are submitted.
> 
> Gnaa, you're right. I _thought_ my code does it the way I described, but
> what I wrote is how I _wanted_ to do it, not how it's currently done. I
> have a plan to change this in the future.
> 
> So unfortunately, that doesn't explain it either. Sorry for the noise.

Well, you might be on the right track. You see, when you do any DMA API
operation (say pci_map_page), you might end up with _two_ DMA addresses.
One that you get from doing 'virt_to_phys' for your buffer (which might
be above the 4GB mark), and another from the 'pci_map_page' (which can
be the virt_to_phys of your buffer or it can be the DMA address of the
SWIOTLB). If you don't submit the _right_ DMA address or sync after the
DMA transfer (so the SWIOTLB would do its memcpy to your allocated
buffer DMA address), you could end up having the data it the SWIOTLB buffer,
and check data in your kzalloc buffer and notice that nothing is there
(and if it hadn't called pci_dma_sync.. before the check).

But this obviously would not happen if you buffer is allocated with the
GFP_DMA32.

I am not familiar with the USB stack so it might be doing this correctly
already...
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Oliver Neukum]

Am Freitag, 7. Mai 2010 11:47:37 schrieb Clemens Ladisch:
> > In the particular case of audio drivers, though, the contents of the
> > buffers are likely to change after the submission. What we do here
> > is that we map the audio stream buffers which are used by ALSA to
> > the output URBs, so they're filled asychronously. Once the buffer is
> > actually sent out on the bus, it is believed to contain proper audio
> > date. If it doesn't, that's due to too tight audio timing or other
> > problems. This breaks once buffers are magically bounced in the
> > background.
> 
> At least the audio class and ua101 drivers don't do this and fill the
> buffers before they are submitted.
> 
> > So - long story short: these audio buffers need to be DMA coherent.
> 
> Does the USB API actually guarantee that all controllers use DMA, i.e.,
> that the buffers can be filled after submission?

No, you must not touch buffers after submission. This does not even
work if we use DMA, because on some architectures this violates
guarantees to the dma primitives. It cannot be done.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Oliver Neukum]

Am Freitag, 7. Mai 2010 13:42:34 schrieb Oliver Neukum:
> Am Freitag, 7. Mai 2010 11:47:37 schrieb Clemens Ladisch:
> > > In the particular case of audio drivers, though, the contents of the
> > > buffers are likely to change after the submission. What we do here
> > > is that we map the audio stream buffers which are used by ALSA to
> > > the output URBs, so they're filled asychronously. Once the buffer is
> > > actually sent out on the bus, it is believed to contain proper audio
> > > date. If it doesn't, that's due to too tight audio timing or other
> > > problems. This breaks once buffers are magically bounced in the
> > > background.
> > 
> > At least the audio class and ua101 drivers don't do this and fill the
> > buffers before they are submitted.
> > 
> > > So - long story short: these audio buffers need to be DMA coherent.
> > 
> > Does the USB API actually guarantee that all controllers use DMA, i.e.,
> > that the buffers can be filled after submission?
> 
> No, you must not touch buffers after submission. This does not even
> work if we use DMA, because on some architectures this violates
> guarantees to the dma primitives. It cannot be done.

Or to be precise it could be done with coherent memory, but you'd risk
transfering partially updated buffers, as you cannot know when DMA will
be done and we don't guarantee that DMA will be done right as we transfer.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB transfer_buffer allocations on 64bit systems

[Daniel Mack]

On Fri, May 07, 2010 at 01:47:56PM +0200, Oliver Neukum wrote:
> Am Freitag, 7. Mai 2010 13:42:34 schrieb Oliver Neukum:
> > Am Freitag, 7. Mai 2010 11:47:37 schrieb Clemens Ladisch:
> > > > In the particular case of audio drivers, though, the contents of the
> > > > buffers are likely to change after the submission. What we do here
> > > > is that we map the audio stream buffers which are used by ALSA to
> > > > the output URBs, so they're filled asychronously. Once the buffer is
> > > > actually sent out on the bus, it is believed to contain proper audio
> > > > date. If it doesn't, that's due to too tight audio timing or other
> > > > problems. This breaks once buffers are magically bounced in the
> > > > background.
> > > 
> > > At least the audio class and ua101 drivers don't do this and fill the
> > > buffers before they are submitted.
> > > 
> > > > So - long story short: these audio buffers need to be DMA coherent.
> > > 
> > > Does the USB API actually guarantee that all controllers use DMA, i.e.,
> > > that the buffers can be filled after submission?
> > 
> > No, you must not touch buffers after submission. This does not even
> > work if we use DMA, because on some architectures this violates
> > guarantees to the dma primitives. It cannot be done.

We do such tricks on other OS where IRQ latency as high as some tens
of milliseconds. So we queue EHCI transfers well in advance and update
access their contents (for both input and output) from audio context.
I considered implementing this idea to the Linux USB audio driver as
well.

> Or to be precise it could be done with coherent memory, but you'd risk
> transfering partially updated buffers, as you cannot know when DMA will
> be done and we don't guarantee that DMA will be done right as we transfer.

If the memory is coherent, it should be possible. And if buffers are
only partially updated, the audio buffer scheduling is too tight. IOW,
the configured buffer size of audio clients in userspace is too small.

This is OT now I believe, I will start another thread for this topic
once I have something to show.

Thanks,
Daniel

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Alan Stern]

On Fri, 7 May 2010, Daniel Mack wrote:

> > Or to be precise it could be done with coherent memory, but you'd risk
> > transfering partially updated buffers, as you cannot know when DMA will
> > be done and we don't guarantee that DMA will be done right as we transfer.
> 
> If the memory is coherent, it should be possible. And if buffers are
> only partially updated, the audio buffer scheduling is too tight. IOW,
> the configured buffer size of audio clients in userspace is too small.

Even with coherent memory, this dynamic updating of buffer contents 
isn't a good idea.  There's no guarantee that coherent memory won't be 
implemented using a bounce buffer during URB submission.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB transfer_buffer allocations on 64bit systems

[Takashi Iwai]

At Wed, 7 Apr 2010 18:16:03 +0200,
Daniel Mack wrote:
> 
> On Wed, Apr 07, 2010 at 11:55:19AM -0400, Alan Stern wrote:
> > On Wed, 7 Apr 2010, Greg KH wrote:
> > 
> > > Alan, any objection to just using usb_buffer_alloc() for every driver?
> > > Or is that too much overhead?
> > 
> > I don't know what the overhead is.  But usb_buffer_alloc() requires the 
> > caller to keep track of the buffer's DMA address, so it's not a simple 
> > plug-in replacement.  In addition, the consistent memory that 
> > usb_buffer_alloc() provides is a scarce resource on some platforms.
> > 
> > Writing new functions is the way to go.
> 
> Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
> will just call kmalloc() with GFP_DMA32 for now.

Can't we provide only zalloc() variant?  Zero'ing doesn't cost much,
and the buffer allocation shouldn't be called too often.

> And while at it,
> usb_alloc_buffer() will be renamed to usb_alloc_consistent().

Most of recent functions are named with "coherent".


thanks,

Takashi

Re: USB transfer_buffer allocations on 64bit systems

[Alan Stern]

On Wed, 7 Apr 2010, Takashi Iwai wrote:

> > Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
> > will just call kmalloc() with GFP_DMA32 for now.
> 
> Can't we provide only zalloc() variant?  Zero'ing doesn't cost much,
> and the buffer allocation shouldn't be called too often.

Linus specifically requested us to avoid using kzalloc in usbfs.  I
can't find the message in the email archives, but Greg KH should be
able to confirm it.

As long as we're imitating kmalloc for one use, we might as well make
it available to all.

> > And while at it,
> > usb_alloc_buffer() will be renamed to usb_alloc_consistent().
> 
> Most of recent functions are named with "coherent".

Yes, the terminology got a little confused between the PCI and DMA
realms.  I agree, "coherent" is better.

BTW, although some EHCI controllers may support 64-bit DMA, the driver 
contains this:

	if (HCC_64BIT_ADDR(hcc_params)) {
		ehci_writel(ehci, 0, &ehci->regs->segment);
#if 0
// this is deeply broken on almost all architectures
		if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
			ehci_info(ehci, "enabled 64bit DMA\n");
#endif
	}

I don't know if the comment is still true, but until the "#if 0" is 
removed, ehci-hcd won't make use of 64-bit DMA.

Note also that just before the lines above, ehci-hcd.c contains this
comment:

	 * pci_pool consistent memory always uses segment zero.

(Nowadays the driver uses dma_pool, not pci_pool.)  As far as I can
tell, this comment isn't true on 64-bit systems, but nevertheless,
ehci-hcd relies on it.  Of course it _is_ true for devices whose DMA
mask indicates they can't use memory above 4 GB -- but if ehci-hcd is
changed to enlarge the DMA mask then how do we force dma_pool memory to
lie below 4 GB?

[Actually it isn't _really_ necessary for the dma_pool to lie below 4 
GB.  But ehci-hcd allocates several pools, and it _is_ necessary for 
all of them to lie in the _same_ 4 GB segment.  The easiest way to do 
that is to put them all in segment 0.]

Alan Stern

Re: USB transfer_buffer allocations on 64bit systems

[Greg KH]

On Wed, Apr 07, 2010 at 03:13:11PM -0400, Alan Stern wrote:
> On Wed, 7 Apr 2010, Takashi Iwai wrote:
> 
> > > Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
> > > will just call kmalloc() with GFP_DMA32 for now.
> > 
> > Can't we provide only zalloc() variant?  Zero'ing doesn't cost much,
> > and the buffer allocation shouldn't be called too often.
> 
> Linus specifically requested us to avoid using kzalloc in usbfs.  I
> can't find the message in the email archives, but Greg KH should be
> able to confirm it.
> 
> As long as we're imitating kmalloc for one use, we might as well make
> it available to all.
> 
> > > And while at it,
> > > usb_alloc_buffer() will be renamed to usb_alloc_consistent().
> > 
> > Most of recent functions are named with "coherent".
> 
> Yes, the terminology got a little confused between the PCI and DMA
> realms.  I agree, "coherent" is better.
> 
> BTW, although some EHCI controllers may support 64-bit DMA, the driver 
> contains this:
> 
> 	if (HCC_64BIT_ADDR(hcc_params)) {
> 		ehci_writel(ehci, 0, &ehci->regs->segment);
> #if 0
> // this is deeply broken on almost all architectures
> 		if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
> 			ehci_info(ehci, "enabled 64bit DMA\n");
> #endif
> 	}
> 
> I don't know if the comment is still true, but until the "#if 0" is 
> removed, ehci-hcd won't make use of 64-bit DMA.

I think someone tried to remove it recently, but I wouldn't let them :)

What a mess, hopefully xhci will just take over and save the world from
this whole thing...

thanks,

greg k-h

Re: USB transfer_buffer allocations on 64bit systems

[Robert Hancock]

On 04/07/2010 06:33 PM, Greg KH wrote:
> On Wed, Apr 07, 2010 at 03:13:11PM -0400, Alan Stern wrote:
>> On Wed, 7 Apr 2010, Takashi Iwai wrote:
>>
>>>> Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
>>>> will just call kmalloc() with GFP_DMA32 for now.
>>>
>>> Can't we provide only zalloc() variant?  Zero'ing doesn't cost much,
>>> and the buffer allocation shouldn't be called too often.
>>
>> Linus specifically requested us to avoid using kzalloc in usbfs.  I
>> can't find the message in the email archives, but Greg KH should be
>> able to confirm it.
>>
>> As long as we're imitating kmalloc for one use, we might as well make
>> it available to all.
>>
>>>> And while at it,
>>>> usb_alloc_buffer() will be renamed to usb_alloc_consistent().
>>>
>>> Most of recent functions are named with "coherent".
>>
>> Yes, the terminology got a little confused between the PCI and DMA
>> realms.  I agree, "coherent" is better.
>>
>> BTW, although some EHCI controllers may support 64-bit DMA, the driver
>> contains this:
>>
>> 	if (HCC_64BIT_ADDR(hcc_params)) {
>> 		ehci_writel(ehci, 0,&ehci->regs->segment);
>> #if 0
>> // this is deeply broken on almost all architectures
>> 		if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
>> 			ehci_info(ehci, "enabled 64bit DMA\n");
>> #endif
>> 	}
>>
>> I don't know if the comment is still true, but until the "#if 0" is
>> removed, ehci-hcd won't make use of 64-bit DMA.
>
> I think someone tried to remove it recently, but I wouldn't let them :)
>
> What a mess, hopefully xhci will just take over and save the world from
> this whole thing...

True.. except for the fact that the xhci driver currently doesn't do 
64-bit DMA either, nor does it support MSI even though the HW supports 
it (surprisingly enough the NEC Windows driver does, MSI-X even). At 
this point only Intel likely knows how to do this properly, though, 
since AFAICS the spec isn't publicly available yet.

Re: USB transfer_buffer allocations on 64bit systems

[Sarah Sharp]

On Thu, Apr 08, 2010 at 06:01:27PM -0600, Robert Hancock wrote:
> On 04/07/2010 06:33 PM, Greg KH wrote:
> >On Wed, Apr 07, 2010 at 03:13:11PM -0400, Alan Stern wrote:
> >>On Wed, 7 Apr 2010, Takashi Iwai wrote:
> >>
> >>>>Ok, I'll write some dummies for usb_malloc() and usb_zalloc() which
> >>>>will just call kmalloc() with GFP_DMA32 for now.
> >>>
> >>>Can't we provide only zalloc() variant?  Zero'ing doesn't cost much,
> >>>and the buffer allocation shouldn't be called too often.
> >>
> >>Linus specifically requested us to avoid using kzalloc in usbfs.  I
> >>can't find the message in the email archives, but Greg KH should be
> >>able to confirm it.
> >>
> >>As long as we're imitating kmalloc for one use, we might as well make
> >>it available to all.
> >>
> >>>>And while at it,
> >>>>usb_alloc_buffer() will be renamed to usb_alloc_consistent().
> >>>
> >>>Most of recent functions are named with "coherent".
> >>
> >>Yes, the terminology got a little confused between the PCI and DMA
> >>realms.  I agree, "coherent" is better.
> >>
> >>BTW, although some EHCI controllers may support 64-bit DMA, the driver
> >>contains this:
> >>
> >>	if (HCC_64BIT_ADDR(hcc_params)) {
> >>		ehci_writel(ehci, 0,&ehci->regs->segment);
> >>#if 0
> >>// this is deeply broken on almost all architectures
> >>		if (!dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64)))
> >>			ehci_info(ehci, "enabled 64bit DMA\n");
> >>#endif
> >>	}
> >>
> >>I don't know if the comment is still true, but until the "#if 0" is
> >>removed, ehci-hcd won't make use of 64-bit DMA.
> >
> >I think someone tried to remove it recently, but I wouldn't let them :)
> >
> >What a mess, hopefully xhci will just take over and save the world from
> >this whole thing...

I hate to break it to you, but 64-bit DMA support is optional for an
xHCI implementation.  There's a bit in HCCPARAMS that tells whether the
host supports it (see the HCC_64BIT_ADDR macro in xhci.h).  The xHCI
driver currently doesn't do anything with that bit, although it should.
All the implementations I've seen do 64-bit DMA.

> True.. except for the fact that the xhci driver currently doesn't do
> 64-bit DMA either

What makes you think that?  I've seen URB buffers with 64-bit DMA
addresses.  I can tell when the debug polling loop runs and I look at
the DMA addresses the xHCI driver is feeding to the hardware:

Dev 1 endpoint ring 0:
xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841

So the TRB at address 71a49800 is pointing to a buffer at address
0x0008000001000680.

If I'm setting a DMA mask wrong somewhere, or doing something else to
limit the DMA to 32-bit, then please let me know.

> nor does it support MSI even though the HW
> supports it (surprisingly enough the NEC Windows driver does, MSI-X
> even).

There's a patch from AMD to enable MSI-X.  The code was there, just
commented out because the early prototypes didn't do MSI-X.

> At this point only Intel likely knows how to do this
> properly, though, since AFAICS the spec isn't publicly available
> yet.

I have tried very hard to fix this, and will continue to do so.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB transfer_buffer allocations on 64bit systems

[Robert Hancock]

On Fri, Apr 9, 2010 at 10:50 AM, Sarah Sharp
<sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> >>I don't know if the comment is still true, but until the "#if 0" is
>> >>removed, ehci-hcd won't make use of 64-bit DMA.
>> >
>> >I think someone tried to remove it recently, but I wouldn't let them :)
>> >
>> >What a mess, hopefully xhci will just take over and save the world from
>> >this whole thing...
>
> I hate to break it to you, but 64-bit DMA support is optional for an
> xHCI implementation.  There's a bit in HCCPARAMS that tells whether the
> host supports it (see the HCC_64BIT_ADDR macro in xhci.h).  The xHCI
> driver currently doesn't do anything with that bit, although it should.
> All the implementations I've seen do 64-bit DMA.
>
>> True.. except for the fact that the xhci driver currently doesn't do
>> 64-bit DMA either
>
> What makes you think that?  I've seen URB buffers with 64-bit DMA
> addresses.  I can tell when the debug polling loop runs and I look at
> the DMA addresses the xHCI driver is feeding to the hardware:
>
> Dev 1 endpoint ring 0:
> xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841
>
> So the TRB at address 71a49800 is pointing to a buffer at address
> 0x0008000001000680.

I'm not sure why the address would be that huge, unless it's not
actually a physical address, or there's some kind of remapping going
on?

>
> If I'm setting a DMA mask wrong somewhere, or doing something else to
> limit the DMA to 32-bit, then please let me know.

The DMA mask for the controller isn't being set anywhere (in the
version that's in Linus' current git anyway). In that case it'll
default to 32-bit and any DMA mappings above 4GB will need to be
remapped. The controller driver doesn't do the mapping itself but the
USB core does, passing in the controller device as the one doing the
DMA, so if the controller's DMA mask is set to 32-bit then the buffers
passed in will get remapped/bounced accordingly.

You should likely be setting the DMA mask for the controller device to
64-bit if the HCC_64BIT_ADDR flag is set, similar to the code in
ehci-hcd.c which is currently #if 0'ed out.

You can see the currently set mask in sysfs under
/sys/devices/pci(whatever)/dma_mask_bits.

>
>> nor does it support MSI even though the HW
>> supports it (surprisingly enough the NEC Windows driver does, MSI-X
>> even).
>
> There's a patch from AMD to enable MSI-X.  The code was there, just
> commented out because the early prototypes didn't do MSI-X.

Ah, I see it. That could presumably be tested now (the NEC D720200F1
chip on the Asus U3S6 card I have seems to support MSI-X with 8
vectors).

>
>> At this point only Intel likely knows how to do this
>> properly, though, since AFAICS the spec isn't publicly available
>> yet.
>
> I have tried very hard to fix this, and will continue to do so.
>
> Sarah Sharp
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB transfer_buffer allocations on 64bit systems

[Daniel Mack]

On Fri, Apr 09, 2010 at 05:38:13PM -0600, Robert Hancock wrote:
> On Fri, Apr 9, 2010 at 10:50 AM, Sarah Sharp
> <sarah.a.sharp@linux.intel.com> wrote:
> > What makes you think that?  I've seen URB buffers with 64-bit DMA
> > addresses.  I can tell when the debug polling loop runs and I look at
> > the DMA addresses the xHCI driver is feeding to the hardware:
> >
> > Dev 1 endpoint ring 0:
> > xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841
> >
> > So the TRB at address 71a49800 is pointing to a buffer at address
> > 0x0008000001000680.
> 
> I'm not sure why the address would be that huge, unless it's not
> actually a physical address, or there's some kind of remapping going
> on?
> 
> >
> > If I'm setting a DMA mask wrong somewhere, or doing something else to
> > limit the DMA to 32-bit, then please let me know.
> 
> The DMA mask for the controller isn't being set anywhere (in the
> version that's in Linus' current git anyway). In that case it'll
> default to 32-bit and any DMA mappings above 4GB will need to be
> remapped. The controller driver doesn't do the mapping itself but the
> USB core does, passing in the controller device as the one doing the
> DMA, so if the controller's DMA mask is set to 32-bit then the buffers
> passed in will get remapped/bounced accordingly.

So if we're seeing physical addresses in the log above, and the xHCI
driver does not explicitly allow the USB core to use addresses above
4GB, why shouldn't the same thing be true as well for EHCI?
(Which would then be exactly the case we're seeing)

Daniel

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Robert Hancock]

On Sat, Apr 10, 2010 at 2:34 AM, Daniel Mack <daniel@caiaq.de> wrote:
> On Fri, Apr 09, 2010 at 05:38:13PM -0600, Robert Hancock wrote:
>> On Fri, Apr 9, 2010 at 10:50 AM, Sarah Sharp
>> <sarah.a.sharp@linux.intel.com> wrote:
>> > What makes you think that?  I've seen URB buffers with 64-bit DMA
>> > addresses.  I can tell when the debug polling loop runs and I look at
>> > the DMA addresses the xHCI driver is feeding to the hardware:
>> >
>> > Dev 1 endpoint ring 0:
>> > xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841
>> >
>> > So the TRB at address 71a49800 is pointing to a buffer at address
>> > 0x0008000001000680.
>>
>> I'm not sure why the address would be that huge, unless it's not
>> actually a physical address, or there's some kind of remapping going
>> on?
>>
>> >
>> > If I'm setting a DMA mask wrong somewhere, or doing something else to
>> > limit the DMA to 32-bit, then please let me know.
>>
>> The DMA mask for the controller isn't being set anywhere (in the
>> version that's in Linus' current git anyway). In that case it'll
>> default to 32-bit and any DMA mappings above 4GB will need to be
>> remapped. The controller driver doesn't do the mapping itself but the
>> USB core does, passing in the controller device as the one doing the
>> DMA, so if the controller's DMA mask is set to 32-bit then the buffers
>> passed in will get remapped/bounced accordingly.
>
> So if we're seeing physical addresses in the log above, and the xHCI
> driver does not explicitly allow the USB core to use addresses above
> 4GB, why shouldn't the same thing be true as well for EHCI?
> (Which would then be exactly the case we're seeing)

That is a bit suspicious, yes. With the DMA mask at default I don't
expect that should happen. Sarah, what kind of traffic was happening
when you saw that (bulk, isochronous, etc)?

If this apparent EHCI problem is reproducible, it might be useful to
add some code that warns if anything non-zero gets written to the
hw_bufp_hi and hw_buf_hi members in the descriptors.

Also, booting with mem=4096M on an affected system would also tell you
if it's related to using memory above 4GB.

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Sarah Sharp]

On Sat, Apr 10, 2010 at 11:02:53AM -0600, Robert Hancock wrote:
> On Sat, Apr 10, 2010 at 2:34 AM, Daniel Mack <daniel@caiaq.de> wrote:
> > On Fri, Apr 09, 2010 at 05:38:13PM -0600, Robert Hancock wrote:
> >> On Fri, Apr 9, 2010 at 10:50 AM, Sarah Sharp
> >> <sarah.a.sharp@linux.intel.com> wrote:
> >> > What makes you think that?  I've seen URB buffers with 64-bit DMA
> >> > addresses.  I can tell when the debug polling loop runs and I look at
> >> > the DMA addresses the xHCI driver is feeding to the hardware:
> >> >
> >> > Dev 1 endpoint ring 0:
> >> > xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841
> >> >
> >> > So the TRB at address 71a49800 is pointing to a buffer at address
> >> > 0x0008000001000680.
> >>
> >> I'm not sure why the address would be that huge, unless it's not
> >> actually a physical address, or there's some kind of remapping going
> >> on?
> >>
> >> >
> >> > If I'm setting a DMA mask wrong somewhere, or doing something else to
> >> > limit the DMA to 32-bit, then please let me know.
> >>
> >> The DMA mask for the controller isn't being set anywhere (in the
> >> version that's in Linus' current git anyway). In that case it'll
> >> default to 32-bit and any DMA mappings above 4GB will need to be
> >> remapped. The controller driver doesn't do the mapping itself but the
> >> USB core does, passing in the controller device as the one doing the
> >> DMA, so if the controller's DMA mask is set to 32-bit then the buffers
> >> passed in will get remapped/bounced accordingly.
> >
> > So if we're seeing physical addresses in the log above, and the xHCI
> > driver does not explicitly allow the USB core to use addresses above
> > 4GB, why shouldn't the same thing be true as well for EHCI?
> > (Which would then be exactly the case we're seeing)
> 
> That is a bit suspicious, yes. With the DMA mask at default I don't
> expect that should happen. Sarah, what kind of traffic was happening
> when you saw that (bulk, isochronous, etc)?

Ring 0 is the default control ring, so it must be control transfers.
That's the first control transfer on the ring (and it didn't fill), so
it must have come from the USB core.

I was running the USB mass storage driver, and the bulk endpoint rings
don't have the high 32-bits of the address set.  It mostly uses the
scatter-gather interface, which calls usb_buffer_map_sg(), so that's not
surprising.

Sarah Sharp

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Robert Hancock]

On Mon, Apr 12, 2010 at 12:56 PM, Sarah Sharp
<sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On Sat, Apr 10, 2010 at 11:02:53AM -0600, Robert Hancock wrote:
>> On Sat, Apr 10, 2010 at 2:34 AM, Daniel Mack <daniel-rDUAYElUppE@public.gmane.org> wrote:
>> > On Fri, Apr 09, 2010 at 05:38:13PM -0600, Robert Hancock wrote:
>> >> On Fri, Apr 9, 2010 at 10:50 AM, Sarah Sharp
>> >> <sarah.a.sharp-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> >> > What makes you think that?  I've seen URB buffers with 64-bit DMA
>> >> > addresses.  I can tell when the debug polling loop runs and I look at
>> >> > the DMA addresses the xHCI driver is feeding to the hardware:
>> >> >
>> >> > Dev 1 endpoint ring 0:
>> >> > xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841
>> >> >
>> >> > So the TRB at address 71a49800 is pointing to a buffer at address
>> >> > 0x0008000001000680.
>> >>
>> >> I'm not sure why the address would be that huge, unless it's not
>> >> actually a physical address, or there's some kind of remapping going
>> >> on?
>> >>
>> >> >
>> >> > If I'm setting a DMA mask wrong somewhere, or doing something else to
>> >> > limit the DMA to 32-bit, then please let me know.
>> >>
>> >> The DMA mask for the controller isn't being set anywhere (in the
>> >> version that's in Linus' current git anyway). In that case it'll
>> >> default to 32-bit and any DMA mappings above 4GB will need to be
>> >> remapped. The controller driver doesn't do the mapping itself but the
>> >> USB core does, passing in the controller device as the one doing the
>> >> DMA, so if the controller's DMA mask is set to 32-bit then the buffers
>> >> passed in will get remapped/bounced accordingly.
>> >
>> > So if we're seeing physical addresses in the log above, and the xHCI
>> > driver does not explicitly allow the USB core to use addresses above
>> > 4GB, why shouldn't the same thing be true as well for EHCI?
>> > (Which would then be exactly the case we're seeing)
>>
>> That is a bit suspicious, yes. With the DMA mask at default I don't
>> expect that should happen. Sarah, what kind of traffic was happening
>> when you saw that (bulk, isochronous, etc)?
>
> Ring 0 is the default control ring, so it must be control transfers.
> That's the first control transfer on the ring (and it didn't fill), so
> it must have come from the USB core.
>
> I was running the USB mass storage driver, and the bulk endpoint rings
> don't have the high 32-bits of the address set.  It mostly uses the
> scatter-gather interface, which calls usb_buffer_map_sg(), so that's not
> surprising.

Is this machine using an IOMMU or something which would be remapping
physical addresses? That address 0x0008000001000680 seems huge, I
don't see how it could even be a valid bus address otherwise..
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] USB transfer_buffer allocations on 64bit systems

[Sarah Sharp]

On Mon, Apr 12, 2010 at 02:39:04PM -0600, Robert Hancock wrote:
> On Mon, Apr 12, 2010 at 12:56 PM, Sarah Sharp
> <sarah.a.sharp@linux.intel.com> wrote:
> > On Sat, Apr 10, 2010 at 11:02:53AM -0600, Robert Hancock wrote:
> >> On Sat, Apr 10, 2010 at 2:34 AM, Daniel Mack <daniel@caiaq.de> wrote:
> >> > On Fri, Apr 09, 2010 at 05:38:13PM -0600, Robert Hancock wrote:
> >> >> On Fri, Apr 9, 2010 at 10:50 AM, Sarah Sharp
> >> >> <sarah.a.sharp@linux.intel.com> wrote:
> >> >> > What makes you think that?  I've seen URB buffers with 64-bit DMA
> >> >> > addresses.  I can tell when the debug polling loop runs and I look at
> >> >> > the DMA addresses the xHCI driver is feeding to the hardware:
> >> >> >
> >> >> > Dev 1 endpoint ring 0:
> >> >> > xhci_hcd 0000:05:00.0: @71a49800 01000680 00080000 00000008 00000841
> >> >> >
> >> >> > So the TRB at address 71a49800 is pointing to a buffer at address
> >> >> > 0x0008000001000680.
> >> >>
> >> >> I'm not sure why the address would be that huge, unless it's not
> >> >> actually a physical address, or there's some kind of remapping going
> >> >> on?
> >> >>
> >> >> >
> >> >> > If I'm setting a DMA mask wrong somewhere, or doing something else to
> >> >> > limit the DMA to 32-bit, then please let me know.
> >> >>
> >> >> The DMA mask for the controller isn't being set anywhere (in the
> >> >> version that's in Linus' current git anyway). In that case it'll
> >> >> default to 32-bit and any DMA mappings above 4GB will need to be
> >> >> remapped. The controller driver doesn't do the mapping itself but the
> >> >> USB core does, passing in the controller device as the one doing the
> >> >> DMA, so if the controller's DMA mask is set to 32-bit then the buffers
> >> >> passed in will get remapped/bounced accordingly.
> >> >
> >> > So if we're seeing physical addresses in the log above, and the xHCI
> >> > driver does not explicitly allow the USB core to use addresses above
> >> > 4GB, why shouldn't the same thing be true as well for EHCI?
> >> > (Which would then be exactly the case we're seeing)
> >>
> >> That is a bit suspicious, yes. With the DMA mask at default I don't
> >> expect that should happen. Sarah, what kind of traffic was happening
> >> when you saw that (bulk, isochronous, etc)?
> >
> > Ring 0 is the default control ring, so it must be control transfers.
> > That's the first control transfer on the ring (and it didn't fill), so
> > it must have come from the USB core.
> >
> > I was running the USB mass storage driver, and the bulk endpoint rings
> > don't have the high 32-bits of the address set.  It mostly uses the
> > scatter-gather interface, which calls usb_buffer_map_sg(), so that's not
> > surprising.
> 
> Is this machine using an IOMMU or something which would be remapping
> physical addresses? That address 0x0008000001000680 seems huge, I
> don't see how it could even be a valid bus address otherwise..

Oh, shoot, nevermind.  That TRB has a slightly different format, where
the setup data for the control transfer is substituted for the DMA
buffer address.  I'll have to look through my logs to see if there's any
real 64-bit DMA addresses in there, and fix xHCI's DMA mask.

Sarah Sharp