Skip to contentSkip to conversation

cleanup large frame handling for natsemi.c

PATCH In review Manfred Spraul 2 participants 5 messages Jun 20, 2004

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

Manfred Spraul

author
#0 lore raw

PATCH 1/1 cleanup large frame handling for natsemi.c

Hi,

The DP83815/6 by default rejects frames longer that 1518 bytes (including crc). This means that a special flag must be set for 8021q - otherwise mtu sized packets are dropped.
The current driver enables this flag only if the buffer size is above 1536 bytes - this is wrong. Additionally, the nic writes up to two bytes behind the indicated end of the buffer. This is not documented, thus I've added 64 bytes - just to be safe. The patch also removes RX_OFFSET from the rx buffer allocation: The nic can only receive to 32-bit aligned addresses, it's a left over from a skeleton driver.

Jeff, could you apply it? I've stress tested vlan for an hour with tbench and parallel kernel compiles, not obvious problems.

--
Manfred

#1 lore raw
Quoted 2 lines · Manfred Spraul

Jeff, could you apply it? I've stress tested vlan for an hour with tbench and parallel kernel compiles, not obvious problems.

I would if you had actually included a patch ;-)

Jeff

author
#2 lore raw
Quoted 11 lines · Jeff Garzik
Quoted 3 lines · Manfred Spraul

Jeff, could you apply it? I've stress tested vlan for an hour with tbench and parallel kernel compiles, not obvious problems.

I would if you had actually included a patch ;-)

Ups, sorry. Here's the patch.

--
Manfred

drivers/net/natsemi.c +20 -8
@@ -236,7 +236,14 @@
 #define NATSEMI_REGS_SIZE	(NATSEMI_NREGS * sizeof(u32))
 #define NATSEMI_EEPROM_SIZE	24 /* 12 16-bit values */
 
-#define PKT_BUF_SZ		1536 /* Size of each temporary Rx buffer. */
+/* Buffer sizes:
+ * The nic writes 32-bit values, even if the upper bytes of
+ * a 32-bit value are beyond the end of the buffer.
+ */
+#define NATSEMI_HEADERS		22	/* 2*mac,type,vlan,crc */
+#define NATSEMI_PADDING		64	/* 2 bytes should be sufficient */
+#define NATSEMI_LONGPKT		1518	/* limit for long packets */
+#define NATSEMI_RX_LIMIT	2046	/* maximum supported by hardware */
 
 /* These identify the driver base version and may not be removed. */
 static char version[] __devinitdata =

@@ -1688,7 +1695,7 @@
 	 */
 	np->rx_config = RxMxdma_256 | 0x20;
 	/* if receive ring now has bigger buffers than normal, enable jumbo */
-	if (np->rx_buf_sz > PKT_BUF_SZ)
+	if (np->rx_buf_sz > NATSEMI_LONGPKT)
 		np->rx_config |= RxAcceptLong;
 
 	writel(np->rx_config, ioaddr + RxConfig);

@@ -1870,7 +1877,7 @@
 		struct sk_buff *skb;
 		int entry = np->dirty_rx % RX_RING_SIZE;
 		if (np->rx_skbuff[entry] == NULL) {
-			unsigned int buflen = np->rx_buf_sz + RX_OFFSET;
+			unsigned int buflen = np->rx_buf_sz+NATSEMI_PADDING;
 			skb = dev_alloc_skb(buflen);
 			np->rx_skbuff[entry] = skb;
 			if (skb == NULL)

@@ -1909,9 +1916,13 @@
 	np->dirty_rx = 0;
 	np->cur_rx = RX_RING_SIZE;
 	np->oom = 0;
-	np->rx_buf_sz = PKT_BUF_SZ;
-	if (dev->mtu > ETH_DATA_LEN)
-		np->rx_buf_sz += dev->mtu - ETH_DATA_LEN;
+	if (dev->mtu <= ETH_DATA_LEN)
+		np->rx_buf_sz = ETH_DATA_LEN + NATSEMI_HEADERS;
+	else
+		np->rx_buf_sz = dev->mtu + NATSEMI_HEADERS;
+	if (np->rx_buf_sz > NATSEMI_RX_LIMIT)
+		np->rx_buf_sz = NATSEMI_RX_LIMIT;
+
 	np->rx_head_desc = &np->rx_ring[0];
 
 	/* Please be carefull before changing this loop - at least gcc-2.95.1

@@ -1949,7 +1960,7 @@
 static void drain_ring(struct net_device *dev)
 {
 	struct netdev_private *np = dev->priv;
-	unsigned int buflen = np->rx_buf_sz + RX_OFFSET;
+	unsigned int buflen = np->rx_buf_sz;
 	int i;
 
 	/* Free all the skbuffs in the Rx queue. */

@@ -2154,7 +2165,7 @@
 	int entry = np->cur_rx % RX_RING_SIZE;
 	int boguscnt = np->dirty_rx + RX_RING_SIZE - np->cur_rx;
 	s32 desc_status = le32_to_cpu(np->rx_head_desc->cmd_status);
-	unsigned int buflen = np->rx_buf_sz + RX_OFFSET;
+	unsigned int buflen = np->rx_buf_sz;
 
 	/* If the driver owns the next entry it's a new packet. Send it up. */
 	while (desc_status < 0) { /* e.g. & DescOwn */

@@ -2881,6 +2892,7 @@
 	}
 	return 0;
 }
+
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
 	struct mii_ioctl_data *data = if_mii(rq);
#3 lore raw
Quoted 4 lines · Manfred Spraul
drivers/net/natsemi.c +0 -0
@@ -1909,9 +1916,13 @@
 	np->dirty_rx = 0;
 	np->cur_rx = RX_RING_SIZE;
 	np->oom = 0;

drivers/net/natsemi.c line 1925

Quoted 10 lines
drivers/net/natsemi.c +7 -3
-	np->rx_buf_sz = PKT_BUF_SZ;
-	if (dev->mtu > ETH_DATA_LEN)
-		np->rx_buf_sz += dev->mtu - ETH_DATA_LEN;
+	if (dev->mtu <= ETH_DATA_LEN)
+		np->rx_buf_sz = ETH_DATA_LEN + NATSEMI_HEADERS;
+	else
+		np->rx_buf_sz = dev->mtu + NATSEMI_HEADERS;
+	if (np->rx_buf_sz > NATSEMI_RX_LIMIT)
+		np->rx_buf_sz = NATSEMI_RX_LIMIT;
+

Double NAK:

1) Use PKT_BUF_SZ, don't alloc smaller than that.

2) The final check should not be needed. The code should guarantee that np->rx_buf_sz never exceeds NATSEMI_RX_LIMIT.

Jeff

author
#4 lore raw
Quoted 22 lines · Jeff Garzik
Quoted 6 lines · Manfred Spraul
drivers/net/natsemi.c +0 -2
@@ -1909,9 +1916,13 @@
 	np->dirty_rx = 0;
 	np->cur_rx = RX_RING_SIZE;
 	np->oom = 0;
-	np->rx_buf_sz = PKT_BUF_SZ;
-	if (dev->mtu > ETH_DATA_LEN)

drivers/net/natsemi.c line 1927

Quoted 10 lines
drivers/net/natsemi.c +7 -1
-		np->rx_buf_sz += dev->mtu - ETH_DATA_LEN;
+	if (dev->mtu <= ETH_DATA_LEN)
+		np->rx_buf_sz = ETH_DATA_LEN + NATSEMI_HEADERS;
+	else
+		np->rx_buf_sz = dev->mtu + NATSEMI_HEADERS;
+	if (np->rx_buf_sz > NATSEMI_RX_LIMIT)
+		np->rx_buf_sz = NATSEMI_RX_LIMIT;
+

Double NAK:

Quoted 4 lines

1) Use PKT_BUF_SZ, don't alloc smaller than that.

The alloc size is never smaller than PKT_BUF_SZ: ETH_DATA_LEN+NATSEMI_HEADERS+NATSEMI_PADDING is 1586 bytes [still smaller than the 1620 byte skb kmalloc cache]

Quoted 5 lines

2) The final check should not be needed. The code should guarantee that np->rx_buf_sz never exceeds NATSEMI_RX_LIMIT.

You mean: implement a change_mtu callback and reject mtu values above 2020 byte?

--
Manfred

Quoted 5 lines

Jeff