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

login
register
mail settings
Submitter Jesse Brandeburg
Date Nov. 14, 2008, 1:21 a.m.
Message ID <Pine.WNT.4.63.0811131718390.984@jbrandeb-desk.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/8683/
State Superseded
Delegated to: David Miller
Headers show

Comments

Jesse Brandeburg - Nov. 14, 2008, 1:21 a.m.
On Fri, 31 Oct 2008, Russell King - ARM Linux wrote:

> On Thu, Oct 30, 2008 at 05:27:59PM +0100, Anders Grafström wrote:
> > 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 <jesse.brandeburg@intel.com>

Russell King - ARM Linux wrote:
> On Thu, Oct 30, 2008 at 05:27:59PM +0100, Anders Grafström wrote:
>> 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 <grfstrm@users.sourceforge.net>
CC: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---

 drivers/net/e100.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)
David Miller - Nov. 14, 2008, 2:42 a.m.
From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
Date: Thu, 13 Nov 2008 17:21:50 -0800 (Pacific Standard Time)

> On Fri, 31 Oct 2008, Russell King - ARM Linux wrote:
> 
> > On Thu, Oct 30, 2008 at 05:27:59PM +0100, Anders Grafström wrote:
> > > 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 <jesse.brandeburg@intel.com>

FWIW this looks good to me.

Acked-by: David S. Miller <davem@davemloft.net>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anders Grafström - Nov. 14, 2008, 8:37 p.m.
Brandeburg, Jesse wrote:
> On Fri, 31 Oct 2008, Russell King - ARM Linux wrote:
> 
>> On Thu, Oct 30, 2008 at 05:27:59PM +0100, Anders Grafström wrote:
>>> 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 <jesse.brandeburg@intel.com>

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Brandeburg - Nov. 14, 2008, 10:30 p.m.
Anders Grafström wrote:
> Brandeburg, Jesse wrote:
>> On Fri, 31 Oct 2008, Russell King - ARM Linux wrote:
>>
>>> On Thu, Oct 30, 2008 at 05:27:59PM +0100, Anders Grafström wrote:
>>>> 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 <jesse.brandeburg@intel.com>
>
> 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!

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index 62cdefa..c9c7079 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -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))