From: Mika Westerberg <hidden> Date: 2011-05-29 09:03:57
Commit a197b59ae6e8 (mm: fail GFP_DMA allocations when ZONE_DMA is not
configured) made page allocator to return NULL if GFP_DMA is set but
CONFIG_ZONE_DMA is disabled.
This causes ep93xx_eth to fail:
WARNING: at mm/page_alloc.c:2251 __alloc_pages_nodemask+0x11c/0x638()
Modules linked in:
[<c0035498>] (unwind_backtrace+0x0/0xf4) from [<c0043da4>] (warn_slowpath_common+0x48/0x60)
[<c0043da4>] (warn_slowpath_common+0x48/0x60) from [<c0043dd8>] (warn_slowpath_null+0x1c/0x24)
[<c0043dd8>] (warn_slowpath_null+0x1c/0x24) from [<c0083b6c>] (__alloc_pages_nodemask+0x11c/0x638)
[<c0083b6c>] (__alloc_pages_nodemask+0x11c/0x638) from [<c00366fc>] (__dma_alloc+0x8c/0x3ec)
[<c00366fc>] (__dma_alloc+0x8c/0x3ec) from [<c0036adc>] (dma_alloc_coherent+0x54/0x60)
[<c0036adc>] (dma_alloc_coherent+0x54/0x60) from [<c0227808>] (ep93xx_open+0x20/0x864)
[<c0227808>] (ep93xx_open+0x20/0x864) from [<c0283144>] (__dev_open+0xb8/0x108)
[<c0283144>] (__dev_open+0xb8/0x108) from [<c0280528>] (__dev_change_flags+0x70/0x128)
[<c0280528>] (__dev_change_flags+0x70/0x128) from [<c0283054>] (dev_change_flags+0x10/0x48)
[<c0283054>] (dev_change_flags+0x10/0x48) from [<c001a720>] (ip_auto_config+0x190/0xf68)
[<c001a720>] (ip_auto_config+0x190/0xf68) from [<c00233b0>] (do_one_initcall+0x34/0x18c)
[<c00233b0>] (do_one_initcall+0x34/0x18c) from [<c0008400>] (kernel_init+0x94/0x134)
[<c0008400>] (kernel_init+0x94/0x134) from [<c0030858>] (kernel_thread_exit+0x0/0x8)
Since there is no restrictions for DMA on ep93xx, we can fix this by just
removing the GFP_DMA flag from the allocations.
Signed-off-by: Mika Westerberg <redacted>
---
drivers/net/arm/ep93xx_eth.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
@@ -494,7 +494,7 @@ static int ep93xx_alloc_buffers(struct ep93xx_priv *ep)inti;ep->descs=dma_alloc_coherent(NULL,sizeof(structep93xx_descs),-&ep->descs_dma_addr,GFP_KERNEL|GFP_DMA);+&ep->descs_dma_addr,GFP_KERNEL);
This one is correct anyway - whether the allocation comes from the DMA
zone or not is something which should be controlled by the DMA mask and
not the driver passing random GFP flags to dma_alloc_coherent.
However, because it is passing a NULL device to dma_alloc_coherent(), it
assumes that it is for an old ISA device, and so will continue to perform
a GFP_DMA allocation. The solution - pass a struct device and set the
DMA masks correctly.
Wow, just looked at what this driver is doing with the DMA buffer handling,
and I'm wondering how come its soo broken.
So, to summarize what its doing:
1. It allocates buffers for rx and tx.
2. It maps them with dma_map_single().
This transfers ownership of the buffer to the DMA device.
3. In ep93xx_xmit,
3a. It copies the data into the buffer with skb_copy_and_csum_dev()
This violates the DMA buffer ownership rules - the CPU should
not be writing to this buffer while it is (in principle) owned
by the DMA device.
3b. It then calls dma_sync_single_for_cpu() for the buffer.
This transfers ownership of the buffer to the CPU, which surely
is the wrong direction.
4. In ep93xx_rx,
4a. It calls dma_sync_single_for_cpu() for the buffer.
This at least transfers the DMA buffer ownership to the CPU
before the CPU reads the buffer
4b. It then uses skb_copy_to_linear_data() to copy the data out.
At no point does it transfer ownership back to the DMA device.
5. When the driver is removed, it dma_unmap_single()'s the buffer.
This transfers ownership of the buffer to the CPU.
6. It frees the buffer.
While it may work on ep93xx, it's not respecting the DMA API rules,
and with DMA debugging enabled it will probably encounter quite a few
warnings.
@@ -494,7 +494,7 @@ static int ep93xx_alloc_buffers(struct ep93xx_priv *ep)inti;ep->descs=dma_alloc_coherent(NULL,sizeof(structep93xx_descs),-&ep->descs_dma_addr,GFP_KERNEL|GFP_DMA);+&ep->descs_dma_addr,GFP_KERNEL);
This one is correct anyway - whether the allocation comes from the DMA
zone or not is something which should be controlled by the DMA mask and
not the driver passing random GFP flags to dma_alloc_coherent.
However, because it is passing a NULL device to dma_alloc_coherent(), it
assumes that it is for an old ISA device, and so will continue to perform
a GFP_DMA allocation. The solution - pass a struct device and set the
DMA masks correctly.
From: Russell King - ARM Linux <hidden> Date: 2011-05-29 11:27:42
On Sun, May 29, 2011 at 01:59:46PM +0300, Mika Westerberg wrote:
FYI, I just enabled DMA debugging and I've got:
[ 1.980000] WARNING: at lib/dma-debug.c:911 check_sync+0x460/0x510()
[ 1.980000] NULL NULL: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x00000000c5a11800] [size=78 bytes]
That's because of the 'allocate one buffer, map it once, then treat it
as two buffers' thing. DMA API debugging requires that the struct
device, and device address match:
static struct dma_debug_entry *hash_bucket_find(struct hash_bucket *bucket,
struct dma_debug_entry *ref)
{
struct dma_debug_entry *entry, *ret = NULL;
int matches = 0, match_lvl, last_lvl = 0;
list_for_each_entry(entry, &bucket->list, list) {
if ((entry->dev_addr != ref->dev_addr) ||
(entry->dev != ref->dev))
continue;
so the practice of using dma_sync_single_for_xxx() with partial buffers
is prohibited by this code (which I've always believed to be the right
answer.) I've always believed that dma_sync_single_range_for_xxx() is
the correct interface for doing this kind of thing.
Others may have a different view, in which case _something_ needs to get
fixed because their view is inconsistent with the debugging code!