Subject: BUG in the PCNET32 ethernet driver

From: Carsten Langgaard <[email-masked]>
Date: Wed, 13 Nov 2002 14:34:48 +0100

I finally found the problem that caused a lot of problem with an
ethernet throughput test, that we have been running.
It turned out the problem is related to a bug in the PCNET32 driver,
when you are running it on a system that doesn't support hardware
coherency.

The problem is the way the AMD ethernet driver is using the PCI DMA
mapping routines.
When the driver releases a receive DMA buffer to the controller for
later DMA transfer it call the PCI DMA flushing routine as it should,
but it calls it with a length equal to 0. The driver is assuming that
the length field in the buffer structure is equal to the actual length
of the buffer, but this field is first updated when we are receiving the
packet (and call the skb_put function).

I have attached a patch, that solve this problem.
Please note that the patch is against Ralf Baechle latest linux_2_4
tree.

/Carsten

--
_    _ ____  ___   Carsten Langgaard   Mailto:[email-masked]
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

Index: drivers/net/pcnet32.c
===================================================================
RCS file: /home/cvs/linux/drivers/net/pcnet32.c,v
retrieving revision 1.33.2.3
diff -u -r1.33.2.3 pcnet32.c
--- drivers/net/pcnet32.c	6 Oct 2002 20:49:43 -0000	1.33.2.3
+++ drivers/net/pcnet32.c	13 Nov 2002 13:32:09 -0000
@@ -981,7 +981,7 @@
 	    }
 	    skb_reserve (rx_skbuff, 2);
 	}
-        lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, rx_skbuff->len, PCI_DMA_FROMDEVICE);
+        lp->rx_dma_addr[i] = pci_map_single(lp->pci_dev, rx_skbuff->tail, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 	lp->rx_ring[i].base = (u32)le32_to_cpu(lp->rx_dma_addr[i]);
 	lp->rx_ring[i].buf_length = le16_to_cpu(-PKT_BUF_SZ);
 	lp->rx_ring[i].status = le16_to_cpu(0x8000);
@@ -1316,13 +1316,13 @@
 		    if ((newskb = dev_alloc_skb (PKT_BUF_SZ))) {
 			skb_reserve (newskb, 2);
 			skb = lp->rx_skbuff[entry];
-			pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[entry], skb->len, PCI_DMA_FROMDEVICE);
+			pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[entry], pkt_len +2, PCI_DMA_FROMDEVICE);
 			skb_put (skb, pkt_len);
 			lp->rx_skbuff[entry] = newskb;
 			newskb->dev = dev;
                         lp->rx_dma_addr[entry] = 
 				pci_map_single(lp->pci_dev, newskb->tail,
-					newskb->len, PCI_DMA_FROMDEVICE);
+					PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 			lp->rx_ring[entry].base = le32_to_cpu(lp->rx_dma_addr[entry]);
 			rx_in_place = 1;
 		    } else
@@ -1349,13 +1349,10 @@
 		if (!rx_in_place) {
 		    skb_reserve(skb,2); /* 16 byte align */
 		    skb_put(skb,pkt_len);	/* Make room */
-                    pci_dma_sync_single(lp->pci_dev, 
-				    lp->rx_dma_addr[entry],
-				    pkt_len,
-				    PCI_DMA_FROMDEVICE);
 		    eth_copy_and_sum(skb,
 				     (unsigned char *)(lp->rx_skbuff[entry]->tail),
 				     pkt_len,0);
+		    lp->rx_dma_addr[entry] = pci_map_single(lp->pci_dev, lp->rx_skbuff[entry]->tail, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 		}
 		lp->stats.rx_bytes += skb->len;
 		skb->protocol=eth_type_trans(skb,dev);

---

From: Jeff Garzik <[email-masked]>
Date: Wed, 13 Nov 2002 11:42:35 -0500

Why does this line not reference PKT_BUF_SZ when all the others do?

---

From: Carsten Langgaard <[email-masked]>
Date: Wed, 13 Nov 2002 21:08:08 +0100

> Why does this line not reference PKT_BUF_SZ when all the others do?

In this case we know the size of the packet and therefore only need to handle that.
In the other cases we don't know have big the receiving packet is going to be, so we has to
take care of the whole buffer.

/Carsten

---

From: Jeff Garzik <[email-masked]>
Date: Wed, 13 Nov 2002 15:20:18 -0500

Carsten Langgaard wrote:

> >>-                     pci_unmap_single(lp->pci_dev, 
> lp->rx_dma_addr[entry], skb->len,
> >>PCI_DMA_FROMDEVICE);
> >>+                     pci_unmap_single(lp->pci_dev, 
> lp->rx_dma_addr[entry], pkt_len +2,
> >>PCI_DMA_FROMDEVICE);
> >>                      skb_put (skb, pkt_len);

Well, it's a seriously bad idea to pass different values to map and 
unmap steps, because on some platforms you could wind up telling the 
IOMMU or some other allocator that you are allocating N bytes, but 
freeing N-M bytes.  IOW, a leak.

Now that that's been clarified, please fix up the patch and resubmit... 
  with this issue fixed, it looks apply-able.

	Jeff

---

From: Carsten Langgaard <[email-masked]>
Date: Wed, 13 Nov 2002 21:46:50 +0100

> freeing N-M bytes.  IOW, a leak.

Ok, fine.
There is actually another place in the code that also need a fix then.

>   with this issue fixed, it looks apply-able.

I have made the change and attached a new patch.

+++ drivers/net/pcnet32.c	13 Nov 2002 20:38:39 -0000
+			pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[entry], PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
@@ -1406,7 +1403,7 @@
     for (i = 0; i < RX_RING_SIZE; i++) {
 	lp->rx_ring[i].status = 0;			    
 	if (lp->rx_skbuff[i]) {
-            pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[i], lp->rx_skbuff[i]->len, PCI_DMA_FROMDEVICE);
+            pci_unmap_single(lp->pci_dev, lp->rx_dma_addr[i], PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 	    dev_kfree_skb(lp->rx_skbuff[i]);
         }
 	lp->rx_skbuff[i] = NULL;

---

From: "David S. Miller" <[email-masked]>
Date: Wed, 13 Nov 2002 20:11:55 -0800 (PST)

From: Carsten Langgaard <[email-masked]>
   Date: Wed, 13 Nov 2002 21:08:08 +0100

   

When you unmap a DMA buffer, which is a resource so this is just like
freeing memory, you must specify the exact size you used when creating
the mapping to begin with.

Franks a lot,
David S. Miller
[email-masked]