[EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Rask Ingemann Lambertsen]

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

Attached is a patch which enables jumbo frames on tulip based boards. It  
makes it possible to set an MTU of up to 2025 bytes. However, testing
against an NE3200 board using 10base-2 shows assorted problems when going
above 2018 bytes (for frames without VLAN tags). The problems show up as
framing errors, bad CRCs and such (in both directions).

The patch is against 2.4.22-bk45.

--
Regards,
Rask Ingemann Lambertsen

[-- Attachment #2: tulip-mtu.patch --]
[-- Type: text/plain, Size: 6905 bytes --]

--- linux-2.4.22/drivers/net/tulip/tulip.h-orig	Mon Dec  1 21:58:11 2003
+++ linux-2.4.22/drivers/net/tulip/tulip.h	Mon Dec  1 22:09:14 2003
@@ -190,6 +190,12 @@ enum desc_status_bits {
 	DescOwned = 0x80000000,
 	RxDescFatalErr = 0x8000,
 	RxWholePkt = 0x0300,
+	RxError = (1 << 15),
+	RxErrLong = (1 << 7),
+	RxErrCRC = (1 << 1),
+	RxErrFIFO = (1 << 0),
+	RxErrRunt = (1 << 11),
+	RxErrFrame = (1 << 14),
 };
 
 
@@ -347,6 +353,7 @@ struct tulip_private {
 	struct ring_info tx_buffers[TX_RING_SIZE];
 	/* The addresses of receive-in-place skbuffs. */
 	struct ring_info rx_buffers[RX_RING_SIZE];
+	uint rx_buf_sz;
 	u16 setup_frame[96];	/* Pseudo-Tx frame to init address table. */
 	int chip_id;
 	int revision;
--- linux-2.4.22/drivers/net/tulip/interrupt.c-orig	Mon Dec  1 21:52:10 2003
+++ linux-2.4.22/drivers/net/tulip/interrupt.c	Mon Dec  1 22:03:50 2003
@@ -74,11 +74,11 @@ int tulip_refill_rx(struct net_device *d
 			struct sk_buff *skb;
 			dma_addr_t mapping;
 
-			skb = tp->rx_buffers[entry].skb = dev_alloc_skb(PKT_BUF_SZ);
+			skb = tp->rx_buffers[entry].skb = dev_alloc_skb(tp->rx_buf_sz);
 			if (skb == NULL)
 				break;
 
-			mapping = pci_map_single(tp->pdev, skb->tail, PKT_BUF_SZ,
+			mapping = pci_map_single(tp->pdev, skb->tail, tp->rx_buf_sz,
 						 PCI_DMA_FROMDEVICE);
 			tp->rx_buffers[entry].mapping = mapping;
 
@@ -122,13 +122,21 @@ static int tulip_rx(struct net_device *d
 	/* If we own the next entry, it is a new packet. Send it up. */
 	while ( ! (tp->rx_ring[entry].status & cpu_to_le32(DescOwned))) {
 		s32 status = le32_to_cpu(tp->rx_ring[entry].status);
+		s32 status_hacked;
 
 		if (tulip_debug > 5)
 			printk(KERN_DEBUG "%s: In tulip_rx(), entry %d %8.8x.\n",
 				   dev->name, entry, status);
 		if (--rx_work_limit < 0)
 			break;
-		if ((status & 0x38008300) != 0x0300) {
+
+		/* Ugly Jumbo frame hack. Remove error flag on long frames. */
+		if ((status & (RxErrLong | RxErrCRC | RxErrFIFO | RxErrRunt | RxErrFrame)) == RxErrLong)
+			status_hacked = status & ~(RxError | RxErrLong);
+		else
+			status_hacked = status;
+
+		if (unlikely((status_hacked & 0x38008300) != 0x0300)) {
 			if ((status & 0x38000300) != 0x0300) {
 				/* Ingore earlier buffers. */
 				if ((status & 0xffff) != 0x7fff) {
@@ -154,15 +162,6 @@ static int tulip_rx(struct net_device *d
 			short pkt_len = ((status >> 16) & 0x7ff) - 4;
 			struct sk_buff *skb;
 
-#ifndef final_version
-			if (pkt_len > 1518) {
-				printk(KERN_WARNING "%s: Bogus packet size of %d (%#x).\n",
-					   dev->name, pkt_len, pkt_len);
-				pkt_len = 1518;
-				tp->stats.rx_length_errors++;
-			}
-#endif
-
 #ifdef CONFIG_NET_HW_FLOWCONTROL
                         drop = atomic_read(&netdev_dropping);
                         if (drop)
@@ -203,7 +202,7 @@ static int tulip_rx(struct net_device *d
 #endif
 
 				pci_unmap_single(tp->pdev, tp->rx_buffers[entry].mapping,
-						 PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
+						 tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
 
 				tp->rx_buffers[entry].skb = NULL;
 				tp->rx_buffers[entry].mapping = 0;
--- linux-2.4.22/drivers/net/tulip/tulip_core.c-orig	Mon Dec  1 21:34:32 2003
+++ linux-2.4.22/drivers/net/tulip/tulip_core.c	Mon Dec  1 22:10:53 2003
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/delay.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
@@ -60,11 +61,13 @@ const char * const medianame[32] = {
 	"","","","", "","","","",  "","","","Transceiver reset",
 };
 
+#define PKT_BUF_SZ_MAX		2047	/* Maximum Rx buffer size. */
+
 /* Set the copy breakpoint for the copy-only-tiny-buffer Rx structure. */
 #if defined(__alpha__) || defined(__arm__) || defined(__hppa__) \
 	|| defined(__sparc_) || defined(__ia64__) \
 	|| defined(__sh__) || defined(__mips__) || defined(__SH5__)
-static int rx_copybreak = 1518;
+static int rx_copybreak = PKT_BUF_SZ_MAX;
 #else
 static int rx_copybreak = 100;
 #endif
@@ -518,9 +521,7 @@ void tulip_xon(struct net_device *dev)
 static int
 tulip_open(struct net_device *dev)
 {
-#ifdef CONFIG_NET_HW_FLOWCONTROL
         struct tulip_private *tp = (struct tulip_private *)dev->priv;
-#endif
 	int retval;
 	MOD_INC_USE_COUNT;
 
@@ -529,6 +530,10 @@ tulip_open(struct net_device *dev)
 		return retval;
 	}
 
+ 	tp->rx_buf_sz = (dev->mtu <= 1500 ? PKT_BUF_SZ : dev->mtu + 32);
+	if (tp->rx_buf_sz > PKT_BUF_SZ_MAX)
+		tp->rx_buf_sz = PKT_BUF_SZ_MAX;
+
 	tulip_init_ring (dev);
 
 	tulip_up (dev);
@@ -673,13 +678,13 @@ static void tulip_init_ring(struct net_d
 
 	for (i = 0; i < RX_RING_SIZE; i++) {
 		tp->rx_ring[i].status = 0x00000000;
-		tp->rx_ring[i].length = cpu_to_le32(PKT_BUF_SZ);
+		tp->rx_ring[i].length = cpu_to_le32(tp->rx_buf_sz);
 		tp->rx_ring[i].buffer2 = cpu_to_le32(tp->rx_ring_dma + sizeof(struct tulip_rx_desc) * (i + 1));
 		tp->rx_buffers[i].skb = NULL;
 		tp->rx_buffers[i].mapping = 0;
 	}
 	/* Mark the last entry as wrapping the ring. */
-	tp->rx_ring[i-1].length = cpu_to_le32(PKT_BUF_SZ | DESC_RING_WRAP);
+	tp->rx_ring[i-1].length = cpu_to_le32(tp->rx_buf_sz | DESC_RING_WRAP);
 	tp->rx_ring[i-1].buffer2 = cpu_to_le32(tp->rx_ring_dma);
 
 	for (i = 0; i < RX_RING_SIZE; i++) {
@@ -688,12 +693,12 @@ static void tulip_init_ring(struct net_d
 		/* Note the receive buffer must be longword aligned.
 		   dev_alloc_skb() provides 16 byte alignment.  But do *not*
 		   use skb_reserve() to align the IP header! */
-		struct sk_buff *skb = dev_alloc_skb(PKT_BUF_SZ);
+		struct sk_buff *skb = dev_alloc_skb(tp->rx_buf_sz);
 		tp->rx_buffers[i].skb = skb;
 		if (skb == NULL)
 			break;
 		mapping = pci_map_single(tp->pdev, skb->tail,
-					 PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
+					 tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
 		tp->rx_buffers[i].mapping = mapping;
 		skb->dev = dev;			/* Mark as being used by this device. */
 		tp->rx_ring[i].status = cpu_to_le32(DescOwned);	/* Owned by Tulip chip */
@@ -876,7 +881,7 @@ static int tulip_close (struct net_devic
 		tp->rx_ring[i].length = 0;
 		tp->rx_ring[i].buffer1 = 0xBADF00D0;	/* An invalid address. */
 		if (skb) {
-			pci_unmap_single(tp->pdev, mapping, PKT_BUF_SZ,
+			pci_unmap_single(tp->pdev, mapping, tp->rx_buf_sz,
 					 PCI_DMA_FROMDEVICE);
 			dev_kfree_skb (skb);
 		}
@@ -1321,6 +1326,18 @@ out:
 }
 #endif
 
+static int tulip_change_mtu (struct net_device *dev, int mtu)
+{
+	if (netif_running (dev))
+		return (-EBUSY);
+
+	if (mtu < 0 || mtu > PKT_BUF_SZ_MAX - VLAN_ETH_HLEN - 4)
+		return (-EINVAL);
+
+	dev->mtu = mtu;
+	return (0);
+}
+
 static int __devinit tulip_init_one (struct pci_dev *pdev,
 				     const struct pci_device_id *ent)
 {
@@ -1727,6 +1744,7 @@ static int __devinit tulip_init_one (str
 	dev->get_stats = tulip_get_stats;
 	dev->do_ioctl = private_ioctl;
 	dev->set_multicast_list = set_rx_mode;
+	dev->change_mtu = tulip_change_mtu;
 
 	if (register_netdev(dev))
 		goto err_out_free_ring;

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Jeff Garzik]

Two questions and a comment...

Would you split this into two patches?  The first simply adds, and uses, 
tp->rx_buf_sz.  The second adds PKT_BUF_SZ_MAX and mtu-related changes.

Have you looked at Donald Becker's changes to tulip.c?  He went through 
most of his drivers and made the changes necessary to support larger 
MTUs.  IIRC his tulip.c changes (which should be easily translate-able 
to 2.6.x tulip) were a bit more minimal than your patch, but still 
served the purpose.

For the comment:  I am curious why a VLAN_xxx constant is included in 
the calculation of max MTU, in the ->change_mtu hook?  IMO ->change_mtu 
simply needs to bind the MTU to the min and max h/w limits.  If 
VLAN_ETH_HLEN ever figures into the calculations, those calculations 
should occur elsewhere, not in ->change_mtu.

Thanks!

	Jeff

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Ben Greear]

Rask Ingemann Lambertsen wrote:
> Attached is a patch which enables jumbo frames on tulip based boards. It  
> makes it possible to set an MTU of up to 2025 bytes. However, testing
> against an NE3200 board using 10base-2 shows assorted problems when going
> above 2018 bytes (for frames without VLAN tags). The problems show up as
> framing errors, bad CRCs and such (in both directions).
> 
> The patch is against 2.4.22-bk45.

> +static int tulip_change_mtu (struct net_device *dev, int mtu)
> +{
> +	if (netif_running (dev))
> +		return (-EBUSY);
> +
> +	if (mtu < 0 || mtu > PKT_BUF_SZ_MAX - VLAN_ETH_HLEN - 4)
> +		return (-EINVAL);

I suppose you could let them ignore the VLAN_ETH_HLEN if you
really wanted to (in case they are not running VLANs).

But, this should work fine too.

--Ben

> +
> +	dev->mtu = mtu;
> +	return (0);
> +}
> +

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Rask Ingemann Lambertsen]

On Tue, Dec 09, 2003 at 11:45:42AM -0500, Jeff Garzik wrote:
> Two questions and a comment...
> 
> Would you split this into two patches?  The first simply adds, and uses, 
> tp->rx_buf_sz.  The second adds PKT_BUF_SZ_MAX and mtu-related changes.

That sounds like a good idea. I'll do that.
I tried to find a good place in the private structure to add the rx_buf_sz
field. By good, I mean using the cache efficiently. Any comments about that?

> Have you looked at Donald Becker's changes to tulip.c?  He went through 
> most of his drivers and made the changes necessary to support larger 
> MTUs.  IIRC his tulip.c changes (which should be easily translate-able 
> to 2.6.x tulip) were a bit more minimal than your patch, but still 
> served the purpose.

I have not looked at this particular part of Becker's tulip.c. I'll have a
look at it.

> For the comment:  I am curious why a VLAN_xxx constant is included in 
> the calculation of max MTU, in the ->change_mtu hook?

This is to ensure that the receive buffers are large enough to hold a frame
of the requested MTU also when using VLAN tags.

> IMO ->change_mtu 
> simply needs to bind the MTU to the min and max h/w limits.  If 
> VLAN_ETH_HLEN ever figures into the calculations, those calculations 
> should occur elsewhere, not in ->change_mtu.

What do you propose? Do we need something like

int vlan_adjust_mtu (int mtu)
{
#ifdef CONFIG_VLANN_8021Q
	return (mtu - VLAN_HLEN);
#else
	return (mtu);
#endif
}

and

int foobar_change_mtu (struct net_device *dev, int mtu)
{
	mtu = vlan_adjust_mtu (mtu);
	/* check hardware limits. */
	...
	dev->mtu = mtu;
	return (0);
}

? Ben, this would also keep you happy, right?

-- 
Regards,
Rask Ingemann Lambertsen

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Ben Greear]

Rask Ingemann Lambertsen wrote:

> What do you propose? Do we need something like
> 
> int vlan_adjust_mtu (int mtu)
> {
> #ifdef CONFIG_VLANN_8021Q
> 	return (mtu - VLAN_HLEN);
> #else
> 	return (mtu);
> #endif
> }
> 
> and
> 
> int foobar_change_mtu (struct net_device *dev, int mtu)
> {
> 	mtu = vlan_adjust_mtu (mtu);
> 	/* check hardware limits. */
> 	...
> 	dev->mtu = mtu;
> 	return (0);
> }
> 
> ? Ben, this would also keep you happy, right?

I was thinking the check could be made run-time, but in reality, this is
a very minor detail.  It may be better to just hard-code it like you
had it originally.  I don't like the patch above, I'd rather see the
#ifdef when checking for the maximum hardware limit, if anywhere.

Thanks,
Ben

> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Rask Ingemann Lambertsen]

On Tue, 09 Dec 2003 14:38:01 -0800, Ben Greear wrote
> Rask Ingemann Lambertsen wrote:
> 
> > What do you propose? Do we need something like
> > 
> > int vlan_adjust_mtu (int mtu)
> > {
> > #ifdef CONFIG_VLANN_8021Q
> > 	return (mtu - VLAN_HLEN);
> > #else
> > 	return (mtu);
> > #endif
> > }
> I was thinking the check could be made run-time, but in reality, 
> this is a very minor detail.

You could use something like

        if (dev->priv_flags & IFF_802_1Q_VLAN)
                return (mtu - VLAN_HLEN);
        else
                return (mtu);

but then you have a problem if the MTU is set to the maximum with VLAN 
disabled and someone decides to enable VLAN on the device afterwards. A 
possible solution would be to set NETIF_F_VLAN_CHALLENGED when VLAN_HLEN
extra bytes are not available.

That said, even checking CONFIG_VLAN_8021Q is probably flawed too, because 
ideally, even when building a kernel without VLAN support, you should be able 
to use the bridging support in a VLAN environment. IMHO. I mean, if this is 
not the case, please remind me why we need VLAN patches in the first place 
since setting an MTU of 1496 bytes works with every Ethernet board and driver.

> I don't like the patch above, I'd rather see the
> #ifdef when checking for the maximum hardware limit, if anywhere.

I was thinking that there could be other reasons than VLAN for reducing the 
MTU and that vlan_adjust_mtu() could take these into account as well. PPPoE 
comes to my mind, but I have no clue how that is implemented in Linux (or any 
other system).

-- 
Regards,
Rask Ingemann Lambertsen

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Rask Ingemann Lambertsen]

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

On Tue, Dec 09, 2003 at 11:45:42AM -0500, Jeff Garzik wrote:
> Two questions and a comment...
> 
> Would you split this into two patches?  The first simply adds, and uses, 
> tp->rx_buf_sz.  The second adds PKT_BUF_SZ_MAX and mtu-related changes.

The first patch is attached to this message. I'm not happy with the second
part yet because it is too hackish and I would prefer to have a look at the
tulip documentation first.

> Have you looked at Donald Becker's changes to tulip.c?  He went through 
> most of his drivers and made the changes necessary to support larger 
> MTUs.  IIRC his tulip.c changes (which should be easily translate-able 
> to 2.6.x tulip) were a bit more minimal than your patch, but still 
> served the purpose.

I've just tested Becker's tulip.c v0.97 (7/22/2003) and it fails to receive
packets larger than the standard 1514 bytes. "ping -s 1472" works while
"ping -s 1473" doesn't (when pinging from another box).

-- 
Regards,
Rask Ingemann Lambertsen

[-- Attachment #2: tulip-rxbufsz.patch --]
[-- Type: text/plain, Size: 3704 bytes --]

--- linux-2.4.22/drivers/net/tulip/tulip.h-orig	Mon Dec  1 21:58:11 2003
+++ linux-2.4.22/drivers/net/tulip/tulip.h	Mon Dec  1 22:09:14 2003
@@ -347,6 +353,7 @@ struct tulip_private {
 	struct ring_info tx_buffers[TX_RING_SIZE];
 	/* The addresses of receive-in-place skbuffs. */
 	struct ring_info rx_buffers[RX_RING_SIZE];
+	uint rx_buf_sz;
 	u16 setup_frame[96];	/* Pseudo-Tx frame to init address table. */
 	int chip_id;
 	int revision;
--- linux-2.4.22/drivers/net/tulip/interrupt.c-orig	Mon Dec  1 21:52:10 2003
+++ linux-2.4.22/drivers/net/tulip/interrupt.c	Mon Dec  1 22:03:50 2003
@@ -74,11 +74,11 @@ int tulip_refill_rx(struct net_device *d
 			struct sk_buff *skb;
 			dma_addr_t mapping;
 
-			skb = tp->rx_buffers[entry].skb = dev_alloc_skb(PKT_BUF_SZ);
+			skb = tp->rx_buffers[entry].skb = dev_alloc_skb(tp->rx_buf_sz);
 			if (skb == NULL)
 				break;
 
-			mapping = pci_map_single(tp->pdev, skb->tail, PKT_BUF_SZ,
+			mapping = pci_map_single(tp->pdev, skb->tail, tp->rx_buf_sz,
 						 PCI_DMA_FROMDEVICE);
 			tp->rx_buffers[entry].mapping = mapping;
 
@@ -203,7 +202,7 @@ static int tulip_rx(struct net_device *d
 #endif
 
 				pci_unmap_single(tp->pdev, tp->rx_buffers[entry].mapping,
-						 PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
+						 tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
 
 				tp->rx_buffers[entry].skb = NULL;
 				tp->rx_buffers[entry].mapping = 0;
--- linux-2.4.22/drivers/net/tulip/tulip_core.c-orig	Mon Dec  1 21:34:32 2003
+++ linux-2.4.22/drivers/net/tulip/tulip_core.c	Mon Dec  1 22:10:53 2003
@@ -518,9 +521,7 @@ void tulip_xon(struct net_device *dev)
 static int
 tulip_open(struct net_device *dev)
 {
-#ifdef CONFIG_NET_HW_FLOWCONTROL
         struct tulip_private *tp = (struct tulip_private *)dev->priv;
-#endif
 	int retval;
 	MOD_INC_USE_COUNT;
 
@@ -529,6 +530,7 @@ tulip_open(struct net_device *dev)
 		return retval;
 	}
 
+ 	tp->rx_buf_sz = PKT_BUF_SZ;
 	tulip_init_ring (dev);
 
 	tulip_up (dev);
@@ -673,13 +678,13 @@ static void tulip_init_ring(struct net_d
 
 	for (i = 0; i < RX_RING_SIZE; i++) {
 		tp->rx_ring[i].status = 0x00000000;
-		tp->rx_ring[i].length = cpu_to_le32(PKT_BUF_SZ);
+		tp->rx_ring[i].length = cpu_to_le32(tp->rx_buf_sz);
 		tp->rx_ring[i].buffer2 = cpu_to_le32(tp->rx_ring_dma + sizeof(struct tulip_rx_desc) * (i + 1));
 		tp->rx_buffers[i].skb = NULL;
 		tp->rx_buffers[i].mapping = 0;
 	}
 	/* Mark the last entry as wrapping the ring. */
-	tp->rx_ring[i-1].length = cpu_to_le32(PKT_BUF_SZ | DESC_RING_WRAP);
+	tp->rx_ring[i-1].length = cpu_to_le32(tp->rx_buf_sz | DESC_RING_WRAP);
 	tp->rx_ring[i-1].buffer2 = cpu_to_le32(tp->rx_ring_dma);
 
 	for (i = 0; i < RX_RING_SIZE; i++) {
@@ -688,12 +693,12 @@ static void tulip_init_ring(struct net_d
 		/* Note the receive buffer must be longword aligned.
 		   dev_alloc_skb() provides 16 byte alignment.  But do *not*
 		   use skb_reserve() to align the IP header! */
-		struct sk_buff *skb = dev_alloc_skb(PKT_BUF_SZ);
+		struct sk_buff *skb = dev_alloc_skb(tp->rx_buf_sz);
 		tp->rx_buffers[i].skb = skb;
 		if (skb == NULL)
 			break;
 		mapping = pci_map_single(tp->pdev, skb->tail,
-					 PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
+					 tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
 		tp->rx_buffers[i].mapping = mapping;
 		skb->dev = dev;			/* Mark as being used by this device. */
 		tp->rx_ring[i].status = cpu_to_le32(DescOwned);	/* Owned by Tulip chip */
@@ -876,7 +881,7 @@ static int tulip_close (struct net_devic
 		tp->rx_ring[i].length = 0;
 		tp->rx_ring[i].buffer1 = 0xBADF00D0;	/* An invalid address. */
 		if (skb) {
-			pci_unmap_single(tp->pdev, mapping, PKT_BUF_SZ,
+			pci_unmap_single(tp->pdev, mapping, tp->rx_buf_sz,
 					 PCI_DMA_FROMDEVICE);
 			dev_kfree_skb (skb);
 		}

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Rask Ingemann Lambertsen]

On Wed, Dec 10, 2003 at 12:40:02AM +0100, Rask Ingemann Lambertsen wrote:

> That said, even checking CONFIG_VLAN_8021Q is probably flawed too, because 
> ideally, even when building a kernel without VLAN support, you should be able 
> to use the bridging support in a VLAN environment. IMHO. I mean, if this is 
> not the case, please remind me why we need VLAN patches in the first place 
> since setting an MTU of 1496 bytes works with every Ethernet board and driver.

Further, why is it not the responsibility of vconfig to ensure that the MTU
of the VLAN device is 4 lower than that of the underlying, "bare" Ethernet
device?

-- 
Regards,
Rask Ingemann Lambertsen

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Ben Greear]

Rask Ingemann Lambertsen wrote:
> On Wed, Dec 10, 2003 at 12:40:02AM +0100, Rask Ingemann Lambertsen wrote:
> 
> 
>>That said, even checking CONFIG_VLAN_8021Q is probably flawed too, because 
>>ideally, even when building a kernel without VLAN support, you should be able 
>>to use the bridging support in a VLAN environment. IMHO. I mean, if this is 
>>not the case, please remind me why we need VLAN patches in the first place 
>>since setting an MTU of 1496 bytes works with every Ethernet board and driver.
> 
> 
> Further, why is it not the responsibility of vconfig to ensure that the MTU
> of the VLAN device is 4 lower than that of the underlying, "bare" Ethernet
> device?

Because correctly patched drivers can have eth0 with MTU 1500 and vlan eth0.5 with
MTU of 1500 as well.  I do not want to add artificial policy in vconfig or the
kernel,though of course users can hack up the MTU settings manually as
desired...

Ben


-- 
Ben Greear <greearb@candelatech.com>          <Ben_Greear@excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [EXPERIMENTAL PATCH] 2.4 tulip jumbo frames

[Ben Greear]

Rask Ingemann Lambertsen wrote:
 > On Tue, Dec 09, 2003 at 11:45:42AM -0500, Jeff Garzik wrote:
 >
 >>Two questions and a comment...
 >>
 >>Would you split this into two patches?  The first simply adds, and uses,
 >>tp->rx_buf_sz.  The second adds PKT_BUF_SZ_MAX and mtu-related changes.
 >
 >
 > The first patch is attached to this message. I'm not happy with the second
 > part yet because it is too hackish and I would prefer to have a look at the
 > tulip documentation first.


Whatever became of this?  For what it's worth, I have been using
a patch originally by Ben McKeegan that seems to work fine.  But,
I'd be happy for either of the patches to go in so long as we can finally
get the tulip driver to work with VLANs.


--- linux-2.4.25/drivers/net/tulip/interrupt.c	2003-06-13 07:51:35.000000000 -0700
+++ linux-2.4.25.p4/drivers/net/tulip/interrupt.c	2004-05-24 17:24:26.000000000 -0700
@@ -1,4 +1,4 @@
-/*
+/*  -*-linux-c-*-
  	drivers/net/tulip/interrupt.c

  	Maintained by Jeff Garzik <jgarzik@pobox.com>
@@ -122,14 +122,34 @@
  	/* If we own the next entry, it is a new packet. Send it up. */
  	while ( ! (tp->rx_ring[entry].status & cpu_to_le32(DescOwned))) {
  		s32 status = le32_to_cpu(tp->rx_ring[entry].status);
-
+		short pkt_len;
+
  		if (tulip_debug > 5)
  			printk(KERN_DEBUG "%s: In tulip_rx(), entry %d %8.8x.\n",
  				   dev->name, entry, status);
  		if (--rx_work_limit < 0)
  			break;
-		if ((status & 0x38008300) != 0x0300) {
-			if ((status & 0x38000300) != 0x0300) {
+		/*
+		  Omit the four octet CRC from the length.
+		  (May not be considered valid until we have
+		  checked status for RxLengthOver2047 bits)
+                */
+		pkt_len = ((status >> 16) & 0x7ff) - 4;
+
+		/*
+		  Maximum pkt_len is 1518 (1514 + vlan header)
+		  Anything higher than this is always invalid
+		  regardless of RxLengthOver2047 bits
+		*/
+
+		if (((status & (RxLengthOver2047 |
+				RxDescCRCError |
+				RxDescCollisionSeen |
+				RxDescRunt |
+				RxDescDescErr |
+				RxWholePkt)) != RxWholePkt)
+		    || (pkt_len > 1518) ) {
+			if ((status & (RxLengthOver2047 | RxWholePkt)) != RxWholePkt) {
  				/* Ingore earlier buffers. */
  				if ((status & 0xffff) != 0x7fff) {
  					if (tulip_debug > 1)
@@ -138,31 +158,21 @@
  							   dev->name, status);
  					tp->stats.rx_length_errors++;
  				}
-			} else if (status & RxDescFatalErr) {
+			} else {
  				/* There was a fatal error. */
  				if (tulip_debug > 2)
  					printk(KERN_DEBUG "%s: Receive error, Rx status %8.8x.\n",
  						   dev->name, status);
  				tp->stats.rx_errors++; /* end of a packet.*/
-				if (status & 0x0890) tp->stats.rx_length_errors++;
+				if ((pkt_len > 1518) || (status & RxDescRunt))
+					tp->stats.rx_length_errors++;
  				if (status & 0x0004) tp->stats.rx_frame_errors++;
  				if (status & 0x0002) tp->stats.rx_crc_errors++;
  				if (status & 0x0001) tp->stats.rx_fifo_errors++;
  			}
  		} else {
-			/* Omit the four octet CRC from the length. */
-			short pkt_len = ((status >> 16) & 0x7ff) - 4;
  			struct sk_buff *skb;

-#ifndef final_version
-			if (pkt_len > 1518) {
-				printk(KERN_WARNING "%s: Bogus packet size of %d (%#x).\n",
-					   dev->name, pkt_len, pkt_len);
-				pkt_len = 1518;
-				tp->stats.rx_length_errors++;
-			}
-#endif
-
  #ifdef CONFIG_NET_HW_FLOWCONTROL
                          drop = atomic_read(&netdev_dropping);
                          if (drop)
--- linux-2.4.25/drivers/net/tulip/tulip.h	2002-11-28 15:53:14.000000000 -0800
+++ linux-2.4.25.p4/drivers/net/tulip/tulip.h	2004-05-24 17:29:48.000000000 -0700
@@ -188,8 +188,40 @@

  enum desc_status_bits {
  	DescOwned = 0x80000000,
-	RxDescFatalErr = 0x8000,
+
+        /*
+	   Error summary flag is logical or of 'CRC Error',
+	   'Collision Seen', 'Frame Too Long', 'Runt' and
+	   'Descriptor Error' flags generated within tulip chip.
+	*/
+        RxDescErrorSummary = 0x8000,
+	
+	RxDescCRCError = 0x0002,
+        RxDescCollisionSeen = 0x0040,
+	
+        /*
+	   'Frame Too Long' flag is set if packet length including CRC
+	   exceeds 1518.  However, a full sized VLAN tagged frame is
+	   1522 bytes including CRC.
+	
+	   The tulip chip does not block oversized frames, and if this
+	   flag is set on a receive descriptor it does not indicate
+	   the frame has been truncated.  The receive descriptor also
+	   includes the actual length.  Therefore we can safety ignore
+	   this flag and check the length ourselves.
+        */
+        RxDescFrameTooLong = 0x0080,
+	RxDescRunt = 0x0800,
+	RxDescDescErr = 0x4000,
  	RxWholePkt = 0x0300,
+	
+	/*
+	  Top three bits of 14 bit frame length (status bits 27-29)
+          should never be set as that would make frame over 2047 bytes.
+	  The Receive Watchdog flag (bit 4) may indicate the length is
+          over 2048 and the length field is invalid.
+	*/
+	RxLengthOver2047 = 0x38000010
  };



> 
> 
>>Have you looked at Donald Becker's changes to tulip.c?  He went through 
>>most of his drivers and made the changes necessary to support larger 
>>MTUs.  IIRC his tulip.c changes (which should be easily translate-able 
>>to 2.6.x tulip) were a bit more minimal than your patch, but still 
>>served the purpose.
> 
> 
> I've just tested Becker's tulip.c v0.97 (7/22/2003) and it fails to receive
> packets larger than the standard 1514 bytes. "ping -s 1472" works while
> "ping -s 1473" doesn't (when pinging from another box).
> 
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.4.22/drivers/net/tulip/tulip.h-orig	Mon Dec  1 21:58:11 2003
> +++ linux-2.4.22/drivers/net/tulip/tulip.h	Mon Dec  1 22:09:14 2003
> @@ -347,6 +353,7 @@ struct tulip_private {
>  	struct ring_info tx_buffers[TX_RING_SIZE];
>  	/* The addresses of receive-in-place skbuffs. */
>  	struct ring_info rx_buffers[RX_RING_SIZE];
> +	uint rx_buf_sz;
>  	u16 setup_frame[96];	/* Pseudo-Tx frame to init address table. */
>  	int chip_id;
>  	int revision;
> --- linux-2.4.22/drivers/net/tulip/interrupt.c-orig	Mon Dec  1 21:52:10 2003
> +++ linux-2.4.22/drivers/net/tulip/interrupt.c	Mon Dec  1 22:03:50 2003
> @@ -74,11 +74,11 @@ int tulip_refill_rx(struct net_device *d
>  			struct sk_buff *skb;
>  			dma_addr_t mapping;
>  
> -			skb = tp->rx_buffers[entry].skb = dev_alloc_skb(PKT_BUF_SZ);
> +			skb = tp->rx_buffers[entry].skb = dev_alloc_skb(tp->rx_buf_sz);
>  			if (skb == NULL)
>  				break;
>  
> -			mapping = pci_map_single(tp->pdev, skb->tail, PKT_BUF_SZ,
> +			mapping = pci_map_single(tp->pdev, skb->tail, tp->rx_buf_sz,
>  						 PCI_DMA_FROMDEVICE);
>  			tp->rx_buffers[entry].mapping = mapping;
>  
> @@ -203,7 +202,7 @@ static int tulip_rx(struct net_device *d
>  #endif
>  
>  				pci_unmap_single(tp->pdev, tp->rx_buffers[entry].mapping,
> -						 PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
> +						 tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
>  
>  				tp->rx_buffers[entry].skb = NULL;
>  				tp->rx_buffers[entry].mapping = 0;
> --- linux-2.4.22/drivers/net/tulip/tulip_core.c-orig	Mon Dec  1 21:34:32 2003
> +++ linux-2.4.22/drivers/net/tulip/tulip_core.c	Mon Dec  1 22:10:53 2003
> @@ -518,9 +521,7 @@ void tulip_xon(struct net_device *dev)
>  static int
>  tulip_open(struct net_device *dev)
>  {
> -#ifdef CONFIG_NET_HW_FLOWCONTROL
>          struct tulip_private *tp = (struct tulip_private *)dev->priv;
> -#endif
>  	int retval;
>  	MOD_INC_USE_COUNT;
>  
> @@ -529,6 +530,7 @@ tulip_open(struct net_device *dev)
>  		return retval;
>  	}
>  
> + 	tp->rx_buf_sz = PKT_BUF_SZ;
>  	tulip_init_ring (dev);
>  
>  	tulip_up (dev);
> @@ -673,13 +678,13 @@ static void tulip_init_ring(struct net_d
>  
>  	for (i = 0; i < RX_RING_SIZE; i++) {
>  		tp->rx_ring[i].status = 0x00000000;
> -		tp->rx_ring[i].length = cpu_to_le32(PKT_BUF_SZ);
> +		tp->rx_ring[i].length = cpu_to_le32(tp->rx_buf_sz);
>  		tp->rx_ring[i].buffer2 = cpu_to_le32(tp->rx_ring_dma + sizeof(struct tulip_rx_desc) * (i + 1));
>  		tp->rx_buffers[i].skb = NULL;
>  		tp->rx_buffers[i].mapping = 0;
>  	}
>  	/* Mark the last entry as wrapping the ring. */
> -	tp->rx_ring[i-1].length = cpu_to_le32(PKT_BUF_SZ | DESC_RING_WRAP);
> +	tp->rx_ring[i-1].length = cpu_to_le32(tp->rx_buf_sz | DESC_RING_WRAP);
>  	tp->rx_ring[i-1].buffer2 = cpu_to_le32(tp->rx_ring_dma);
>  
>  	for (i = 0; i < RX_RING_SIZE; i++) {
> @@ -688,12 +693,12 @@ static void tulip_init_ring(struct net_d
>  		/* Note the receive buffer must be longword aligned.
>  		   dev_alloc_skb() provides 16 byte alignment.  But do *not*
>  		   use skb_reserve() to align the IP header! */
> -		struct sk_buff *skb = dev_alloc_skb(PKT_BUF_SZ);
> +		struct sk_buff *skb = dev_alloc_skb(tp->rx_buf_sz);
>  		tp->rx_buffers[i].skb = skb;
>  		if (skb == NULL)
>  			break;
>  		mapping = pci_map_single(tp->pdev, skb->tail,
> -					 PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
> +					 tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
>  		tp->rx_buffers[i].mapping = mapping;
>  		skb->dev = dev;			/* Mark as being used by this device. */
>  		tp->rx_ring[i].status = cpu_to_le32(DescOwned);	/* Owned by Tulip chip */
> @@ -876,7 +881,7 @@ static int tulip_close (struct net_devic
>  		tp->rx_ring[i].length = 0;
>  		tp->rx_ring[i].buffer1 = 0xBADF00D0;	/* An invalid address. */
>  		if (skb) {
> -			pci_unmap_single(tp->pdev, mapping, PKT_BUF_SZ,
> +			pci_unmap_single(tp->pdev, mapping, tp->rx_buf_sz,
>  					 PCI_DMA_FROMDEVICE);
>  			dev_kfree_skb (skb);
>  		}


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com