Do I need to skb_put() Ethernet frames to a minimum of 60 bytes?

DISCUSSION Arvid Brodin 4 participants 7 messages Aug 14, 2012 – Dec 17, 2012

Subsystems (3)

Networking Network drivers Atmel MACB Ethernet driver

lore

Share & tools

lore

Arvid Brodin

author

2012-08-14 19:04 UTC #0 lore raw

Hi,

If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get

eth0: TX underrun, resetting buffers

Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug?

Signature · 2 lines

Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com

Ben Hutchings

2012-08-14 20:36 UTC #1 lore raw

Quoted 11 lines · Arvid Brodin

Hi,
If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get
eth0: TX underrun, resetting buffers
Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug?

If the hardware doesn't pad frames automatically then it's the driver's reponsibility to do so.

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Arvid Brodin

author

2012-08-21 17:34 UTC #2 lore raw

Quoted 8 lines · Ben Hutchings

Quoted 7 lines · Arvid Brodin
Hi,
If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get
eth0: TX underrun, resetting buffers

Quoted 8 lines

Quoted 4 lines
Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug?
If the hardware doesn't pad frames automatically then it's the driver's reponsibility to do so.

Nicolas, can you take a look at this? At the moment I'm using the following change in macb.c to avoid TX underruns on short packages:

drivers/net/ethernet/cadence/macb.c +7 -0

@@ -618,6 +618,7 @@ static void macb_poll_controller(struct
 }
 #endif
 
+#define MIN_ETHFRAME_LEN	60
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);

@@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf
 	printk("\n");
 #endif
 
+	if (skb->len < MIN_ETHFRAME_LEN) {
+		/* Pad skb to minium Ethernet frame size */
+		if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len)
+			memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0,
+						MIN_ETHFRAME_LEN - skb->len);
+	}
 	len = skb->len;
 	spin_lock_irqsave(&bp->lock, flags);

... but as you can see this is limited to linear skbs which has been allocated with enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware is actually doing the padding already and the problem has to do with the way the DMA transfer is set up?)

Signature · 2 lines

Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com

Eric Dumazet

2012-08-21 17:42 UTC #3 lore raw

Quoted 27 lines · Arvid Brodin

Nicolas, can you take a look at this? At the moment I'm using the following change in macb.c to avoid TX underruns on short packages:

drivers/net/ethernet/cadence/macb.c +7 -0

@@ -618,6 +618,7 @@ static void macb_poll_controller(struct
 }
 #endif
 
+#define MIN_ETHFRAME_LEN	60
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);

@@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf
 	printk("\n");
 #endif
 
+	if (skb->len < MIN_ETHFRAME_LEN) {
+		/* Pad skb to minium Ethernet frame size */
+		if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len)
+			memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0,
+						MIN_ETHFRAME_LEN - skb->len);
+	}
 	len = skb->len;
 	spin_lock_irqsave(&bp->lock, flags);

Quoted 5 lines

... but as you can see this is limited to linear skbs which has been allocated with enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware is actually doing the padding already and the problem has to do with the way the DMA transfer is set up?)

other net drivers use skb_padto() for this ...

Ben Hutchings

2012-08-21 18:07 UTC #4 lore raw

In reply to Arvid Brodin

Quoted 24 lines · Arvid Brodin

Quoted 8 lines · Ben Hutchings
Quoted 7 lines · Arvid Brodin
Hi,
If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get
eth0: TX underrun, resetting buffers
Quoted 8 lines
Quoted 4 lines
Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug?
If the hardware doesn't pad frames automatically then it's the driver's reponsibility to do so.
Nicolas, can you take a look at this? At the moment I'm using the following change in macb.c to avoid TX underruns on short packages:

drivers/net/ethernet/cadence/macb.c static void macb_poll_controller(struct · line 621

Quoted 4 lines

drivers/net/ethernet/cadence/macb.c +1 -0

@@ -618,6 +618,7 @@ static void macb_poll_controller(struct
 }
 #endif
 
+#define MIN_ETHFRAME_LEN	60

<linux/etherdevice.h> already names this as ETH_ZLEN, by the way.

Quoted 17 lines

drivers/net/ethernet/cadence/macb.c +6 -0

 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);

@@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf
 	printk("\n");
 #endif
 
+	if (skb->len < MIN_ETHFRAME_LEN) {
+		/* Pad skb to minium Ethernet frame size */
+		if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len)
+			memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0,
+						MIN_ETHFRAME_LEN - skb->len);
+	}
 	len = skb->len;
 	spin_lock_irqsave(&bp->lock, flags);

Quoted 2 lines

... but as you can see this is limited to linear skbs which has been allocated with enough tailroom. Perhaps there are better ways to fix the problem?

skb_padto() should be all you need. Note that it frees the skb on failure, so you must just return NETDEV_TX_OK then.

Ben.

Quoted 3 lines

(Maybe the hardware
is actually doing the padding already and the problem has to do with the way the DMA transfer is set up?)

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Nicolas Ferre

2012-12-17 13:43 UTC #5 lore raw

In reply to Arvid Brodin

On 08/21/2012 07:34 PM, Arvid Brodin :

Quoted 45 lines · Ben Hutchings

Quoted 8 lines · Arvid Brodin
Quoted 7 lines
Hi,
If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get
eth0: TX underrun, resetting buffers
Quoted 8 lines
Quoted 4 lines
Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug?
If the hardware doesn't pad frames automatically then it's the driver's reponsibility to do so.
Nicolas, can you take a look at this? At the moment I'm using the following change in macb.c to avoid TX underruns on short packages:
drivers/net/ethernet/cadence/macb.c +7 -0
@@ -618,6 +618,7 @@ static void macb_poll_controller(struct
 }
 #endif
 
+#define MIN_ETHFRAME_LEN	60
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
@@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf
 	printk("\n");
 #endif
 
+	if (skb->len < MIN_ETHFRAME_LEN) {
+		/* Pad skb to minium Ethernet frame size */
+		if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len)
+			memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0,
+						MIN_ETHFRAME_LEN - skb->len);
+	}
 	len = skb->len;
 	spin_lock_irqsave(&bp->lock, flags);
 

Quoted 4 lines

... but as you can see this is limited to linear skbs which has been allocated with enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware is actually doing the padding already and the problem has to do with the way the DMA transfer is set up?)

I come back to this issue. It seems to me that the macb Cadence IP is padding automatically a too little packet. It is the usual behavior unless you specify otherwise in the CTRL register embedded in the tx descriptor. I also verified this with wireshark on both ICMP and UDP packets.

The error that you are experiencing is on at91sam9260 or at91sam9263 SoCs, am I right?

Best regards,

Signature · 1 line

Nicolas Ferre

Arvid Brodin

author

2012-12-17 15:15 UTC #6 lore raw

Quoted 57 lines · Nicolas Ferre

On 08/21/2012 07:34 PM, Arvid Brodin :
Quoted 45 lines · Ben Hutchings
Quoted 8 lines · Arvid Brodin
Quoted 7 lines
Hi,
If I create an sk_buff with a payload of less than 28 bytes (ethheader + data), and send it using the cadence/macb (Ethernet) driver, I get
eth0: TX underrun, resetting buffers
Quoted 8 lines
Quoted 4 lines
Now I know the minimum Ethernet frame size is 64 bytes (including the 4-byte FCS), but whose responsibility is it to pad the frame to this size if necessary? Mine or the driver's - i.e. should I just skb_put() to the minimum size or should I report the underrun as a driver bug?
If the hardware doesn't pad frames automatically then it's the driver's reponsibility to do so.
Nicolas, can you take a look at this? At the moment I'm using the following change in macb.c to avoid TX underruns on short packages:
drivers/net/ethernet/cadence/macb.c +7 -0
@@ -618,6 +618,7 @@ static void macb_poll_controller(struct
 }
 #endif
 
+#define MIN_ETHFRAME_LEN	60
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct macb *bp = netdev_priv(dev);
@@ -635,6 +636,12 @@ static int macb_start_xmit(struct sk_buf
 	printk("\n");
 #endif
 
+	if (skb->len < MIN_ETHFRAME_LEN) {
+		/* Pad skb to minium Ethernet frame size */
+		if (skb_tailroom(skb) >= MIN_ETHFRAME_LEN - skb->len)
+			memset(skb_put(skb, MIN_ETHFRAME_LEN - skb->len), 0,
+						MIN_ETHFRAME_LEN - skb->len);
+	}
 	len = skb->len;
 	spin_lock_irqsave(&bp->lock, flags);
 
Quoted 4 lines
... but as you can see this is limited to linear skbs which has been allocated with enough tailroom. Perhaps there are better ways to fix the problem? (Maybe the hardware is actually doing the padding already and the problem has to do with the way the DMA transfer is set up?)
I come back to this issue. It seems to me that the macb Cadence IP is padding automatically a too little packet. It is the usual behavior unless you specify otherwise in the CTRL register embedded in the tx descriptor. I also verified this with wireshark on both ICMP and UDP packets.

Quoted 2 lines

The error that you are experiencing is on at91sam9260 or at91sam9263 SoCs, am I right?

No, this was on an AVR32 AP7000 board.

I believe this is what I did to solve the issue (patch for linux-2.6.37):

diff -Nurp linux-2.6.37-001-bsa400/drivers/net//macb.c linux-2.6.37-macb-hsr/drivers/net//macb.c

drivers/net//macb.c +9 -4

@@ -376,8 +379,9 @@ static void macb_tx(struct macb *bp)
 
 			rmb();
 
-			dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
-							 DMA_TO_DEVICE);
+			dma_unmap_single(&bp->pdev->dev, rp->mapping,
+					 max(skb->len, (unsigned int) ETH_ZLEN),
+					 DMA_TO_DEVICE);
 			rp->skb = NULL;
 			dev_kfree_skb_irq(skb);
 		}

@@ -413,7 +417,8 @@ static void macb_tx(struct macb *bp)
 
 		dev_dbg(&bp->pdev->dev, "skb %u (data %p) TX complete\n",
 			tail, skb->data);
-		dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
+		dma_unmap_single(&bp->pdev->dev, rp->mapping,
+				 max(skb->len, (unsigned int) ETH_ZLEN),
 				 DMA_TO_DEVICE);
 		bp->stats.tx_packets++;
 		bp->stats.tx_bytes += skb->len;

@@ -675,7 +680,10 @@ static int macb_start_xmit(struct sk_buf
 	printk("\n");
 #endif
 
-	len = skb->len;
+	if (skb_padto(skb, ETH_ZLEN) != 0)
+		return NETDEV_TX_OK; /* There is no NETDEV_TX_FAIL... */
+
+	len = max(skb->len, (unsigned int) ETH_ZLEN);
 	spin_lock_irqsave(&bp->lock, flags);
 
 	/* This is a hard error, log it. */

Signature · 2 lines

Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarn?sgatan 7 | SE-164 40 Kista | Sweden | xdin.com