Skip to contentSkip to conversation

Validation of DMA params breaks e100 driver (2.6.28-rc2)

DISCUSSION Russell King - ARM Linux 4 participants 5 messages Oct 31, 2008 – Nov 14, 2008 1 message not in the archive

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Russell King - ARM Linux

author
#1 lore raw
Quoted 6 lines · Anders Grafström

The e100 driver triggers BUG_ON(buf->direction != dir) by doing pci_map_single(..., PCI_DMA_BIDIRECTIONAL) and pci_dma_sync_single_for_device(..., PCI_DMA_TODEVICE).

I'm guessing it's allowed to do that and that something like the patch below is called for?

No, it is not allowed to do that - that's why it's called "BUG_ON". Changing the DMA direction, especially with dmabounce will result in unexpected behaviour.

Quoted 68 lines

-----------------

[PATCH] [ARM] dma: Fix DMA params validation

This patch makes the DMA params validation less strict and more like the generic implementation.

Signed-off-by Anders Grafström <[email protected]>
---
 arch/arm/common/dmabounce.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
arch/arm/common/dmabounce.c +6 -7
index f030f07..a0c9c7b 100644
@@ -289,7 +289,6 @@ static inline void unmap_single(struct device *dev, dma_addr_t dma_addr,
 
 	if (buf) {
 		BUG_ON(buf->size != size);
-		BUG_ON(buf->direction != dir);
 
 		dev_dbg(dev,
 			"%s: unsafe buffer %p (dma=%#x) mapped to %p (dma=%#x)\n",

@@ -382,8 +381,6 @@ int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr,
 	if (!buf)
 		return 1;
 
-	BUG_ON(buf->direction != dir);
-
 	dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x) mapped to %p (dma=%#x)\n",
 		__func__, buf->ptr, virt_to_dma(dev, buf->ptr),
 		buf->safe, buf->safe_dma_addr);

@@ -394,7 +391,9 @@ int dmabounce_sync_for_cpu(struct device *dev, dma_addr_t addr,
 		dev_dbg(dev, "%s: copy back safe %p to unsafe %p size %d\n",
 			__func__, buf->safe + off, buf->ptr + off, sz);
 		memcpy(buf->ptr + off, buf->safe + off, sz);
-	}
+	} else
+		BUG_ON(dir != DMA_TO_DEVICE);
+
 	return 0;
 }
 EXPORT_SYMBOL(dmabounce_sync_for_cpu);

@@ -411,8 +410,6 @@ int dmabounce_sync_for_device(struct device *dev, dma_addr_t addr,
 	if (!buf)
 		return 1;
 
-	BUG_ON(buf->direction != dir);
-
 	dev_dbg(dev, "%s: unsafe buffer %p (dma=%#x) mapped to %p (dma=%#x)\n",
 		__func__, buf->ptr, virt_to_dma(dev, buf->ptr),
 		buf->safe, buf->safe_dma_addr);

@@ -423,7 +420,9 @@ int dmabounce_sync_for_device(struct device *dev, dma_addr_t addr,
 		dev_dbg(dev, "%s: copy out unsafe %p to safe %p, size %d\n",
 			__func__,buf->ptr + off, buf->safe + off, sz);
 		memcpy(buf->safe + off, buf->ptr + off, sz);
-	}
+	} else
+		BUG_ON(dir != DMA_FROM_DEVICE);
+
 	return 0;
 }
 EXPORT_SYMBOL(dmabounce_sync_for_device);
Signature · 1 line
1.5.6.5
#2 lore raw
Quoted 11 lines · Russell King - ARM Linux
Quoted 6 lines · Anders Grafström

The e100 driver triggers BUG_ON(buf->direction != dir) by doing pci_map_single(..., PCI_DMA_BIDIRECTIONAL) and pci_dma_sync_single_for_device(..., PCI_DMA_TODEVICE).

I'm guessing it's allowed to do that and that something like the patch below is called for?

No, it is not allowed to do that - that's why it's called "BUG_ON". Changing the DMA direction, especially with dmabounce will result in unexpected behaviour.

okay, how about this patch... only compile tested as I couldn't get net-next-2.6 to boot on my test machine. I'll get some testing done on this, but in the meantime....

e100: fix dma error in direction for mapping

From: Jesse Brandeburg <[email protected]>

Quoted 11 lines · Russell King - ARM Linux
Quoted 6 lines · Anders Grafström

The e100 driver triggers BUG_ON(buf->direction != dir) by doing pci_map_single(..., PCI_DMA_BIDIRECTIONAL) and pci_dma_sync_single_for_device(..., PCI_DMA_TODEVICE).

I'm guessing it's allowed to do that and that something like the patch below is called for?

No, it is not allowed to do that - that's why it's called "BUG_ON". Changing the DMA direction, especially with dmabounce will result in unexpected behaviour.

This is my attempt to fix this issue:

Reported by: Anders Grafstrom <[email protected]>

CC Russell King <[email protected]>
Signed-off-by Jesse Brandeburg <[email protected]>

---

 drivers/net/e100.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
drivers/net/e100.c +10 -10
index 62cdefa..c9c7079 100644
@@ -166,7 +166,7 @@
 
 #define DRV_NAME		"e100"
 #define DRV_EXT			"-NAPI"
-#define DRV_VERSION		"3.5.23-k4"DRV_EXT
+#define DRV_VERSION		"3.5.23-k6"DRV_EXT
 #define DRV_DESCRIPTION		"Intel(R) PRO/100 Network Driver"
 #define DRV_COPYRIGHT		"Copyright(c) 1999-2006 Intel Corporation"
 #define PFX			DRV_NAME ": "

@@ -1804,7 +1804,7 @@ static int e100_rx_alloc_skb(struct nic *nic, struct rx *rx)
 		struct rfd *prev_rfd = (struct rfd *)rx->prev->skb->data;
 		put_unaligned_le32(rx->dma_addr, &prev_rfd->link);
 		pci_dma_sync_single_for_device(nic->pdev, rx->prev->dma_addr,
-			sizeof(struct rfd), PCI_DMA_TODEVICE);
+			sizeof(struct rfd), PCI_DMA_BIDIRECTIONAL);
 	}
 
 	return 0;

@@ -1823,7 +1823,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 	/* Need to sync before taking a peek at cb_complete bit */
 	pci_dma_sync_single_for_cpu(nic->pdev, rx->dma_addr,
-		sizeof(struct rfd), PCI_DMA_FROMDEVICE);
+		sizeof(struct rfd), PCI_DMA_BIDIRECTIONAL);
 	rfd_status = le16_to_cpu(rfd->status);
 
 	DPRINTK(RX_STATUS, DEBUG, "status=0x%04X\n", rfd_status);

@@ -1850,7 +1850,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 	/* Get data */
 	pci_unmap_single(nic->pdev, rx->dma_addr,
-		RFD_BUF_LEN, PCI_DMA_FROMDEVICE);
+		RFD_BUF_LEN, PCI_DMA_BIDIRECTIONAL);
 
 	/* If this buffer has the el bit, but we think the receiver
 	 * is still running, check to see if it really stopped while

@@ -1942,7 +1942,7 @@ static void e100_rx_clean(struct nic *nic, unsigned int *work_done,
 		new_before_last_rfd->command |= cpu_to_le16(cb_el);
 		pci_dma_sync_single_for_device(nic->pdev,
 			new_before_last_rx->dma_addr, sizeof(struct rfd),
-			PCI_DMA_TODEVICE);
+			PCI_DMA_BIDIRECTIONAL);
 
 		/* Now that we have a new stopping point, we can clear the old
 		 * stopping point.  We must sync twice to get the proper

@@ -1950,11 +1950,11 @@ static void e100_rx_clean(struct nic *nic, unsigned int *work_done,
 		old_before_last_rfd->command &= ~cpu_to_le16(cb_el);
 		pci_dma_sync_single_for_device(nic->pdev,
 			old_before_last_rx->dma_addr, sizeof(struct rfd),
-			PCI_DMA_TODEVICE);
+			PCI_DMA_BIDIRECTIONAL);
 		old_before_last_rfd->size = cpu_to_le16(VLAN_ETH_FRAME_LEN);
 		pci_dma_sync_single_for_device(nic->pdev,
 			old_before_last_rx->dma_addr, sizeof(struct rfd),
-			PCI_DMA_TODEVICE);
+			PCI_DMA_BIDIRECTIONAL);
 	}
 
 	if(restart_required) {

@@ -1977,7 +1977,7 @@ static void e100_rx_clean_list(struct nic *nic)
 		for(rx = nic->rxs, i = 0; i < count; rx++, i++) {
 			if(rx->skb) {
 				pci_unmap_single(nic->pdev, rx->dma_addr,
-					RFD_BUF_LEN, PCI_DMA_FROMDEVICE);
+					RFD_BUF_LEN, PCI_DMA_BIDIRECTIONAL);
 				dev_kfree_skb(rx->skb);
 			}
 		}

@@ -2020,7 +2020,7 @@ static int e100_rx_alloc_list(struct nic *nic)
 	before_last->command |= cpu_to_le16(cb_el);
 	before_last->size = 0;
 	pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
-		sizeof(struct rfd), PCI_DMA_TODEVICE);
+		sizeof(struct rfd), PCI_DMA_BIDIRECTIONAL);
 
 	nic->rx_to_use = nic->rx_to_clean = nic->rxs;
 	nic->ru_running = RU_SUSPENDED;

@@ -2221,7 +2221,7 @@ static int e100_loopback_test(struct nic *nic, enum loopback loopback_mode)
 	msleep(10);
 
 	pci_dma_sync_single_for_cpu(nic->pdev, nic->rx_to_clean->dma_addr,
-			RFD_BUF_LEN, PCI_DMA_FROMDEVICE);
+			RFD_BUF_LEN, PCI_DMA_BIDIRECTIONAL);
 
 	if(memcmp(nic->rx_to_clean->skb->data + sizeof(struct rfd),
 	   skb->data, ETH_DATA_LEN))

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel

maintainer Acked-by
#3 lore raw

From: "Brandeburg, Jesse" <[email protected]>
Date: Thu, 13 Nov 2008 17:21:50 -0800 (Pacific Standard Time)

Quoted 14 lines · Russell King - ARM Linux
Quoted 6 lines · Anders Grafström
Quoted 4 lines

The e100 driver triggers BUG_ON(buf->direction != dir) by doing pci_map_single(..., PCI_DMA_BIDIRECTIONAL) and pci_dma_sync_single_for_device(..., PCI_DMA_TODEVICE).

Quoted 6 lines
Quoted 2 lines

I'm guessing it's allowed to do that and that something like the patch below is called for?

No, it is not allowed to do that - that's why it's called "BUG_ON". Changing the DMA direction, especially with dmabounce will result in unexpected behaviour.

Quoted 7 lines

okay, how about this patch... only compile tested as I couldn't get net-next-2.6 to boot on my test machine. I'll get some testing done on this, but in the meantime....

e100: fix dma error in direction for mapping

From: Jesse Brandeburg <[email protected]>

FWIW this looks good to me.

Acked-by David S. Miller <[email protected]>
#4 lore raw
In reply to Brandeburg, Jesse
Quoted 13 lines · Jesse
Quoted 10 lines · Russell King - ARM Linux
Quoted 6 lines · Anders Grafström

The e100 driver triggers BUG_ON(buf->direction != dir) by doing pci_map_single(..., PCI_DMA_BIDIRECTIONAL) and pci_dma_sync_single_for_device(..., PCI_DMA_TODEVICE).

I'm guessing it's allowed to do that and that something like the patch below is called for?

No, it is not allowed to do that - that's why it's called "BUG_ON". Changing the DMA direction, especially with dmabounce will result in unexpected behaviour.

Quoted 7 lines

okay, how about this patch... only compile tested as I couldn't get net-next-2.6 to boot on my test machine. I'll get some testing done on this, but in the meantime....

e100: fix dma error in direction for mapping

From: Jesse Brandeburg <[email protected]>

I failed to save this patch (flowed format?) but I applied it manually to 2.6.28-rc4 and tried it. Appears to work.

-------------------------------------------------------------------

List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php
#5 lore raw
Quoted 18 lines · Anders Grafström
Quoted 17 lines · Jesse
Quoted 10 lines · Russell King - ARM Linux
Quoted 6 lines · Anders Grafström

The e100 driver triggers BUG_ON(buf->direction != dir) by doing pci_map_single(..., PCI_DMA_BIDIRECTIONAL) and pci_dma_sync_single_for_device(..., PCI_DMA_TODEVICE).

I'm guessing it's allowed to do that and that something like the patch below is called for?

No, it is not allowed to do that - that's why it's called "BUG_ON". Changing the DMA direction, especially with dmabounce will result in unexpected behaviour.

okay, how about this patch... only compile tested as I couldn't get net-next-2.6 to boot on my test machine. I'll get some testing done on this, but in the meantime....

Quoted 6 lines
Quoted 3 lines

e100: fix dma error in direction for mapping

From: Jesse Brandeburg <[email protected]>

I failed to save this patch (flowed format?) but I applied it manually to 2.6.28-rc4 and tried it. Appears to work.

Strange... sorry for the thrash on the patch, and thanks for testing!

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/