From: Alan Stern on
On Wed, 7 Apr 2010, Daniel Mack wrote:

> > > The fix is to use usb_buffer_alloc() for that purpose which ensures
> > > memory that is suitable for DMA. And on x86_64, this also means that the
> > > upper 32 bits of the address returned are all 0's.
> >
> > That is not a good fix. usb_buffer_alloc() provides coherent memory,
> > which is not what we want. I believe the correct fix is to specify the
> > GFP_DMA32 flag in the kzalloc() call.
> >
> > Of course, some EHCI hardware _is_ capable of using 64-bit addresses.
> > But not all, and other controller types aren't. In principle we could
> > create a new allocation routine, which would take a pointer to the USB
> > bus as an additional argument and use it to decide whether the memory
> > needs to lie below 4 GB. I'm not sure adding this extra complexity
> > would be worthwhile.
>
> Well, I thought this is exactly what the usb_buffer_alloc() abstraction
> functions are there for. We already pass a pointer to a struct
> usb_device, so the routine knows which host controller it operates on.
> So we can safely dispatch all the magic inside this function, no?

No. It says so right in the title line of the kerneldoc:

* usb_buffer_alloc - allocate dma-consistent buffer for URB_NO_xxx_DMA_MAP

That is not what people want when they call kzalloc or kmalloc.

> If not, I would rather introduce a new function than adding GFP_ flags
> to all existing drivers.

All right. But there would have to be _two_ new functions: one acting
like kmalloc and another acting like kzalloc. I guess they should take
a pointer to struct usb_device as an argument, like usb_buffer_alloc.

> I vote for a clean solution, a fixup of existing implementations and
> a clear note about how to allocate buffers for USB drivers. I believe
> faulty allocations of this kind can explain quite a lot of problems on
> x86_64 machines.

There is one awkward aspect of usb_buffer_alloc which perhaps could
be fixed at the same time. Under some conditions (for example, if the
controller needs to use internal bounce buffers) it will return
ordinary memory obtained using kmalloc rather than consistent memory.
But it doesn't tell the caller when it has done so, and consequently
the caller will always specify URB_NO_TRANSFER_DMA_MAP when using the
buffer. The proper flag to use should be returned to the caller.

Or alternatively, instead of allocating regular memory the routine
could simply fail. Then the caller would be responsible for checking
and using regular memory instead of dma-consistent memory. Of course,
that would put an even larger burden on the caller than just forcing it
to keep track of what flag to use.

> > > If what I've stated is true, there are quite some more drivers affected
> > > by this issue.
> >
> > Practically every USB driver, I should think. And maybe a lot of
> > non-USB drivers too.
>
> I found many such problems in drivers/media/{dvb,video},
> drivers/usb/serial, sound/usb and even in the USB core. If we agree on
> how to fix that nicely, I can take some of them.

I guess we could rename usb_buffer_alloc(). A more accurate name would
be something like usb_alloc_consistent() (except for the fact that
the routine falls back to regular memory if the controller can't use
consistent memory.) Then we could have usb_malloc() and usb_zalloc()
in addition.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Wed, 7 Apr 2010, Greg KH wrote:

> Yeah, I really don't want to have to change every driver in different
> ways just depending on if someone thinks it is going to need to run on
> this wierd hardware.

It's not weird hardware, as far as I know. It's just a 64-bit system
with a 32-bit USB host controller.

(And remember, while there are 64-bit EHCI controllers, there are not
any 64-bit OHCI or UHCI controllers. So whenever somebody plugs a
full-speed or low-speed device into a 64-bit machine, they will face
this problem. It's like the old problem of ISA devices that could
only do DMA to addresses in the first 16 MB of memory -- what the
original GFP_DMA flag was intended for.)

> 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.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on
On Wed, Apr 07, 2010 at 05:35:51PM +0200, Daniel Mack wrote:
> On Wed, Apr 07, 2010 at 08:31:54AM -0700, Greg KH wrote:
> > On Wed, Apr 07, 2010 at 05:11:25PM +0200, Daniel Mack wrote:
> > > I vote for a clean solution, a fixup of existing implementations and
> > > a clear note about how to allocate buffers for USB drivers. I believe
> > > faulty allocations of this kind can explain quite a lot of problems on
> > > x86_64 machines.
> >
> > Yeah, I really don't want to have to change every driver in different
> > ways just depending on if someone thinks it is going to need to run on
> > this wierd hardware.
> >
> > Alan, any objection to just using usb_buffer_alloc() for every driver?
> > Or is that too much overhead?
>
> FWIW, most drivers I've seen in the past hours use a wild mix of
> kmalloc(), kzalloc(), kcalloc() and usb_buffer_alloc(). That should
> really be unified.

Yes, if it is necessary that we have to handle this type of crappy
hardware, then it all needs to be unified. Or at least unified into 2
types of calls, one that needs the bounce buffer fun (what
usb_buffer_alloc() does today), and one that doesn't (usb_kzalloc()
perhaps?)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on
On Wed, 7 Apr 2010, Greg KH wrote:

> > FWIW, most drivers I've seen in the past hours use a wild mix of
> > kmalloc(), kzalloc(), kcalloc() and usb_buffer_alloc(). That should
> > really be unified.

Well, kcalloc can easily be replaced by kzalloc, right? Or the
equivalent.

The extra overhead of initializing the memory to 0 isn't present in
kmalloc, so we need to maintain the distinction between kmalloc and
kzalloc.

However usb_buffer_alloc is fundmentally different from all the others.

> Yes, if it is necessary that we have to handle this type of crappy
> hardware, then it all needs to be unified. Or at least unified into 2
> types of calls, one that needs the bounce buffer fun (what
> usb_buffer_alloc() does today), and one that doesn't (usb_kzalloc()
> perhaps?)

usb_buffer_alloc has very little to do with bounce buffers. Its
purpose is to allocate dma-consistent memory, that it, memory which
does not need to be mapped for DMA before each I/O operation and
unmapped afterward.

The mapping and unmapping operations aren't extremely time consuming,
so in general it makes sense to avoid them only when the _same_ buffer
is going to be used for many I/O operations during a short period of
time. For instance, it makes sense for audio and video, where all the
data streams through a small set of buffers arranged in a ring.

But for most other uses it makes no sense. Especially since some
platforms have limited amounts of consistent memory available.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Daniel Mack on
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. And while at it,
usb_alloc_buffer() will be renamed to usb_alloc_consistent(). Then I'll
try to clean up all existing drivers to use this new interface and
follow the changes.

In a next step, we should fine-tune when GFP_DMA32 is really needed.
And I'll leave all occurances of usb_alloc_consistent() as they are now.

Does that sound ok?

Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/