BUG_ON for DMA-bounce and bidirectional mappings (e100 on IXP4xx)

BUG_ON for DMA-bounce and bidirectional mappings (e100 on IXP4xx)

[Simon Kågström]

On Tue, 4 Mar 2014 10:45:28 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Mar 04, 2014 at 11:24:59AM +0100, Simon K?gstr?m wrote:
> > We have a IXP4xx-based board which has an e100 NIC. During bootup, we
> > trigger a BUG_ON in dmabounce.c because the mapped direction doesn't
> > match the synced one. e100.c maps buffers bidirectional, but the sync
> > is done fromdevice.
> 
> First and foremost, enable DMA API debugging and check whether that
> complains too (I think it should do, you're not allowed to change the
> direction argument until a mapping is unmapped.)

Well, you are right, but the warning I get is not for direction:

  WARNING: at /srv/lhome/simkag/devel/nc602/external/kernel/lib/dma-debug.c:986 check_sync+0x14c/0x540()
  e100 0000:00:02.0: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x0000000003c01000] [size=16 bytes]
  [...]
  [<c0134b24>] (debug_dma_sync_single_for_device+0x44/0x50) from [<c017a5e8>] (e100_rx_alloc_skb+0x1c4/0x1e8)
  [<c017a5e8>] (e100_rx_alloc_skb+0x1c4/0x1e8) from [<c017c58c>] (e100_rx_alloc_list+0x84/0x164)
  [<c017c58c>] (e100_rx_alloc_list+0x84/0x164) from [<c017e958>] (e100_up+0xc/0x14c)
  [<c017e958>] (e100_up+0xc/0x14c) from [<c017ecc8>] (e100_open+0x14/0x40)
  [<c017ecc8>] (e100_open+0x14/0x40) from [<c019b230>] (__dev_open+0x8c/0x134)

And the dma_debug code explicitly skips the check if the direction is
DMA_BIDIRECTIONAL:

  static void check_sync(struct device *dev,
		       struct dma_debug_entry *ref,
		       bool to_cpu)
  {
	[...]
	if (entry->direction == DMA_BIDIRECTIONAL)
		goto out;

	if (ref->direction != entry->direction) {
		err_printk(dev, entry, "DMA-API: device driver syncs "
				"DMA memory with different direction "
				"[device address=0x%016llx] [size=%llu bytes] "
				"[mapped with %s] [synced with %s]\n",
				(unsigned long long)ref->dev_addr, entry->size,
				dir2name[entry->direction],
				dir2name[ref->direction]);
	}
	[...]
  }

Krzysztofs patch does the same for dmabounce.c, from the dma-debug code
it seems to me that it would make sense there as well?

// Simon

BUG_ON for DMA-bounce and bidirectional mappings (e100 on IXP4xx)

[Simon Kågström]

Hi again!

On Tue, 4 Mar 2014 12:37:14 +0100
Simon K?gstr?m <simon.kagstrom@netinsight.net> wrote:

> On Tue, 4 Mar 2014 10:45:28 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Tue, Mar 04, 2014 at 11:24:59AM +0100, Simon K?gstr?m wrote:
> > > We have a IXP4xx-based board which has an e100 NIC. During bootup, we
> > > trigger a BUG_ON in dmabounce.c because the mapped direction doesn't
> > > match the synced one. e100.c maps buffers bidirectional, but the sync
> > > is done fromdevice.
> > [...]
> [...]
> And the dma_debug code explicitly skips the check if the direction is
> DMA_BIDIRECTIONAL:
>
>   static void check_sync(struct device *dev,
> 		       struct dma_debug_entry *ref,
> 		       bool to_cpu)
>   {
> 	[...]
> 	if (entry->direction == DMA_BIDIRECTIONAL)
> 		goto out;
> 
> 	if (ref->direction != entry->direction) {
> 		err_printk(dev, entry, "DMA-API: device driver syncs "
> 				"DMA memory with different direction "
> 				"[device address=0x%016llx] [size=%llu bytes] "
> 				"[mapped with %s] [synced with %s]\n",
> 				(unsigned long long)ref->dev_addr, entry->size,
> 				dir2name[entry->direction],
> 				dir2name[ref->direction]);
> 	}
> 	[...]
>   }
> 
> Krzysztofs patch does the same for dmabounce.c, from the dma-debug code
> it seems to me that it would make sense there as well?

I'd like to bring this issue up again. To me it seems Krzysztofs patch
(pasted below again) would align the ARM dmabounce behaviour with the
DMA API verifier code. Seems to make sense for my part at least.
Comments?

// Simon

--- a/arch/arm/common/dmabounce.c
+++ b/arch/arm/common/dmabounce.c
@@ -375,7 +375,7 @@ static int __dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr,
 
 	off = addr - buf->safe_dma_addr;
 
-	BUG_ON(buf->direction != dir);
+	BUG_ON(buf->direction != dir && buf->direction != DMA_BIDIRECTIONAL);
 
 	dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x off=%#lx) mapped to %p (dma=%#x)\n",
 		__func__, buf->ptr, virt_to_dma(dev, buf->ptr), off,
@@ -415,7 +415,7 @@ static int __dmabounce_sync_for_device(struct device *dev, dma_addr_t addr,
 
 	off = addr - buf->safe_dma_addr;
 
-	BUG_ON(buf->direction != dir);
+	BUG_ON(buf->direction != dir && buf->direction != DMA_BIDIRECTIONAL);
 
 	dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x off=%#lx) mapped to %p (dma=%#x)\n",
 		__func__, buf->ptr, virt_to_dma(dev, buf->ptr), off,

BUG_ON for DMA-bounce and bidirectional mappings (e100 on IXP4xx)

[Simon Kågström]

(Sorry, sent to wrong mailing list address)

Hi!

We have a IXP4xx-based board which has an e100 NIC. During bootup, we
trigger a BUG_ON in dmabounce.c because the mapped direction doesn't
match the synced one. e100.c maps buffers bidirectional, but the sync
is done fromdevice.

Krzysztof Ha?asa sent a patch 2012-09-20 which excludes bidirectional
mappings from the sync BUG_ONs, as seen below. We need that patch for
our board to boot. The patch in full is available at

  http://patchwork.ozlabs.org/patch/185486/

This was never applied, but also received no comments. Does the patch
make sense?


For further reference, the e100 BUG_ON was discussed here a few years
ago:

  http://comments.gmane.org/gmane.linux.ports.arm.kernel/48922

there is a e100 patch there from Jesse Brandeburg which changes the
mapping, however with that patch the NIC stops receiving traffic after
a while.

Thanks,
// Simon

--- a/arch/arm/common/dmabounce.c
+++ b/arch/arm/common/dmabounce.c
@@ -375,7 +375,7 @@ static int __dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr,
 
 	off = addr - buf->safe_dma_addr;
 
-	BUG_ON(buf->direction != dir);
+	BUG_ON(buf->direction != dir && buf->direction != DMA_BIDIRECTIONAL);
 
 	dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x off=%#lx) mapped to %p (dma=%#x)\n",
 		__func__, buf->ptr, virt_to_dma(dev, buf->ptr), off,
@@ -415,7 +415,7 @@ static int __dmabounce_sync_for_device(struct device *dev, dma_addr_t addr,
 
 	off = addr - buf->safe_dma_addr;
 
-	BUG_ON(buf->direction != dir);
+	BUG_ON(buf->direction != dir && buf->direction != DMA_BIDIRECTIONAL);
 
 	dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x off=%#lx) mapped to %p (dma=%#x)\n",
 		__func__, buf->ptr, virt_to_dma(dev, buf->ptr), off,