[PATCH] cleanup large frame handling for natsemi.c

[PATCH] cleanup large frame handling for natsemi.c

[Manfred Spraul]

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

Re: [PATCH] cleanup large frame handling for natsemi.c

[Jeff Garzik]

On Sun, Jun 20, 2004 at 02:14:59PM +0200, Manfred Spraul wrote:
> 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

Re: [PATCH] cleanup large frame handling for natsemi.c

[Manfred Spraul]

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

Jeff Garzik wrote:

>On Sun, Jun 20, 2004 at 02:14:59PM +0200, Manfred Spraul wrote:
>  
>
>>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

[-- Attachment #2: patch-natsemi-long --]
[-- Type: text/plain, Size: 2797 bytes --]

--- 2.6/drivers/net/natsemi.c	2004-06-20 11:24:02.959200463 +0200
+++ build-2.6/drivers/net/natsemi.c	2004-06-20 12:11:31.170607816 +0200
@@ -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);

Re: [PATCH] cleanup large frame handling for natsemi.c

[Jeff Garzik]

On Sun, Jun 20, 2004 at 06:43:07PM +0200, Manfred Spraul wrote:
> @@ -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;
> +

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

Re: [PATCH] cleanup large frame handling for natsemi.c

[Manfred Spraul]

Jeff Garzik wrote:

>On Sun, Jun 20, 2004 at 06:43:07PM +0200, Manfred Spraul wrote:
>  
>
>>@@ -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;
>>+
>>    
>>
>
>Double NAK:
>
>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]

>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

>	Jeff
>
>
>  
>