Re: BUG in the PCNET32 ethernet driver

[Carsten Langgaard <carstenl@mips.com>]

[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]

Jeff Garzik wrote:

> Carsten Langgaard wrote:
>
> > Jeff Garzik wrote:
> >
> >
> > >Carsten Langgaard wrote:
> > >
> > >
> > >>@@ -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;
> > >
> > >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.
>
> 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.

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


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

I have made the change and attached a new patch.

>
>         Jeff

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



[-- Attachment #2: pcnet32.2.patch --]
[-- Type: text/plain, Size: 2444 bytes --]

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 20:38:39 -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_BUF_SZ, 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);
@@ -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;