USB transfer_buffer allocations on 64bit systems
in [Kernel]
|
Prev: mfd: Initialise WM831x IRQ masks on chip even if interrupts not in use
Next: INFO: task apache2:7601 blocked for more than 120 seconds.
From: Alan Stern on 7 Apr 2010 11:50 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 7 Apr 2010 12:00 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 7 Apr 2010 12:00 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 7 Apr 2010 12:10 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 7 Apr 2010 12:20
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/ |