* BUG in the PCNET32 ethernet driver
@ 2002-11-13 13:34 Carsten Langgaard
2002-11-13 16:42 ` Jeff Garzik
2002-11-13 20:20 ` Jeff Garzik
0 siblings, 2 replies; 7+ messages in thread
From: Carsten Langgaard @ 2002-11-13 13:34 UTC (permalink / raw)
To: Ralf Baechle, linux-mips, tsbogend, linux-net, kevink
[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]
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: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.patch --]
[-- Type: text/plain, Size: 2038 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 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);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: BUG in the PCNET32 ethernet driver 2002-11-13 13:34 BUG in the PCNET32 ethernet driver Carsten Langgaard @ 2002-11-13 16:42 ` Jeff Garzik 2002-11-13 20:08 ` Carsten Langgaard 2002-11-13 20:20 ` Jeff Garzik 1 sibling, 1 reply; 7+ messages in thread From: Jeff Garzik @ 2002-11-13 16:42 UTC (permalink / raw) To: Carsten Langgaard; +Cc: Ralf Baechle, linux-mips, tsbogend, linux-net, kevink 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: BUG in the PCNET32 ethernet driver 2002-11-13 16:42 ` Jeff Garzik @ 2002-11-13 20:08 ` Carsten Langgaard 2002-11-14 4:11 ` David S. Miller 0 siblings, 1 reply; 7+ messages in thread From: Carsten Langgaard @ 2002-11-13 20:08 UTC (permalink / raw) To: Jeff Garzik; +Cc: Ralf Baechle, linux-mips, tsbogend, linux-net, kevink 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. /Carsten ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: BUG in the PCNET32 ethernet driver 2002-11-13 20:08 ` Carsten Langgaard @ 2002-11-14 4:11 ` David S. Miller 0 siblings, 0 replies; 7+ messages in thread From: David S. Miller @ 2002-11-14 4:11 UTC (permalink / raw) To: carstenl; +Cc: jgarzik, ralf, linux-mips, tsbogend, linux-net, kevink From: Carsten Langgaard <carstenl@mips.com> Date: Wed, 13 Nov 2002 21:08:08 +0100 Jeff Garzik wrote: > 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. 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 davem@redhat.com ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: BUG in the PCNET32 ethernet driver 2002-11-13 13:34 BUG in the PCNET32 ethernet driver Carsten Langgaard 2002-11-13 16:42 ` Jeff Garzik @ 2002-11-13 20:20 ` Jeff Garzik 2002-11-13 20:46 ` Carsten Langgaard 1 sibling, 1 reply; 7+ messages in thread From: Jeff Garzik @ 2002-11-13 20:20 UTC (permalink / raw) To: Carsten Langgaard; +Cc: Ralf Baechle, linux-mips, tsbogend, linux-net, kevink 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. Now that that's been clarified, please fix up the patch and resubmit... with this issue fixed, it looks apply-able. Jeff ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: BUG in the PCNET32 ethernet driver 2002-11-13 20:20 ` Jeff Garzik @ 2002-11-13 20:46 ` Carsten Langgaard 2002-12-11 12:37 ` Carsten Langgaard 0 siblings, 1 reply; 7+ messages in thread From: Carsten Langgaard @ 2002-11-13 20:46 UTC (permalink / raw) To: Jeff Garzik; +Cc: Ralf Baechle, linux-mips, tsbogend, linux-net, kevink [-- 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: BUG in the PCNET32 ethernet driver 2002-11-13 20:46 ` Carsten Langgaard @ 2002-12-11 12:37 ` Carsten Langgaard 0 siblings, 0 replies; 7+ messages in thread From: Carsten Langgaard @ 2002-12-11 12:37 UTC (permalink / raw) To: Jeff Garzik, Ralf Baechle, linux-mips, tsbogend, linux-net, kevink What happen regarding this issue, did you apply my patch or ? /Carsten Carsten Langgaard wrote: > 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 > > ------------------------------------------------------------------------ > 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; -- _ _ ____ ___ 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2002-12-11 12:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2002-11-13 13:34 BUG in the PCNET32 ethernet driver Carsten Langgaard 2002-11-13 16:42 ` Jeff Garzik 2002-11-13 20:08 ` Carsten Langgaard 2002-11-14 4:11 ` David S. Miller 2002-11-13 20:20 ` Jeff Garzik 2002-11-13 20:46 ` Carsten Langgaard 2002-12-11 12:37 ` Carsten Langgaard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.