Home » Mailing list archives » linux-netdev » 2009 » March » 26

[PATCH] gianfar: fix headroom expansion code

Previous thread: 2.6.29 forcedeth hang W/O NAPI enabled by Adam Richter on Wednesday, March 25, 2009 - 5:06 pm. (2 messages)

Next thread: [PATCH] bridge: bad error handling when adding invalid ether address by Stephen Hemminger on Wednesday, March 25, 2009 - 8:57 pm. (2 messages)
[view original message]
From: David Miller
Subject: Re: gianfar: reallocate skb when headroom is not enough for fcb
Date: Wednesday, March 25, 2009 - 5:21 pm

Applied, thanks!
--


[ message continues ]
[view original message]
From: Stephen Hemminger
Subject: [PATCH] gianfar: fix headroom expansion code
Date: Thursday, March 26, 2009 - 10:08 am

The code that was added to increase headroom was wrong.
It doesn't handle the case where gfar_add_fcb() changes the skb.
Better to do check at start of transmit (outside of lock), where
error handling is better anyway.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/gianfar.c	2009-03-26 09:14:39.273669929 -0700
+++ b/drivers/net/gianfar.c	2009-03-26 09:22:46.477545004 -0700
@@ -1239,19 +1239,9 @@ static int gfar_enet_open(struct net_dev
 	return err;
 }
 
-static inline struct txfcb *gfar_add_fcb(struct sk_buff **skbp)
+static inline struct txfcb *gfar_add_fcb(struct sk_buff *skb)
 {
-	struct txfcb *fcb;
-	struct sk_buff *skb = *skbp;
-
-	if (unlikely(skb_headroom(skb) < GMAC_FCB_LEN)) {
-		struct sk_buff *old_skb = skb;
-		skb = skb_realloc_headroom(old_skb, GMAC_FCB_LEN);
-		if (!skb)
-			return NULL;
-		dev_kfree_skb_any(old_skb);
-	}
-	fcb = (struct txfcb *)skb_push(skb, GMAC_FCB_LEN);
+	struct txfcb *fcb = (struct txfcb *)skb_push(skb, GMAC_FCB_LEN);
 	cacheable_memzero(fcb, GMAC_FCB_LEN);
 
 	return fcb;
@@ -1320,6 +1310,20 @@ static int gfar_start_xmit(struct sk_buf
 
 	base = priv->tx_bd_base;
 
+	/* make space for additional header */
+	if (skb_headroom(skb) < GMAC_FCB_LEN) {
+		struct sk_buff *skb_new;
+
+		skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN);
+		if (!skb_new) {
+			dev->stats.tx_errors++;
+			kfree(skb);
+			return NETDEV_TX_OK;
+		}
+		kfree_skb(skb);
+		skb = skb_new;
+	}
+
 	/* total number of fragments in the SKB */
 	nr_frags = skb_shinfo(skb)->nr_frags;
 
@@ -1372,20 +1376,18 @@ static int gfar_start_xmit(struct sk_buf
 
 	/* Set up checksumming */
 	if (CHECKSUM_PARTIAL == skb->ip_summed) {
-		fcb = gfar_add_fcb(&skb);
-		if (likely(fcb != NULL)) {
-			lstatus |= BD_LFLAG(TXBD_TOE);
-			gfar_tx_checksum(skb, fcb);
-		}
+		fcb = gfar_add_fcb(skb);
+		lstatus |= BD_LFLAG(TXBD_TOE);
+		gfar_tx_checksum(skb, fcb);
 	}
 
 	if (priv->vlgrp && vlan_tx_tag_present(skb)) {
-		if ...
[ message continues ]
[view original message]
From: Li Yang
Subject: Re: [PATCH] gianfar: fix headroom expansion code
Date: Thursday, March 26, 2009 - 9:26 pm

On Fri, Mar 27, 2009 at 1:08 AM, Stephen Hemminger


We have legacy devices without the offloading feature.  And we can
even turn off the IP checksum offloading at runtime.  Your code will
cause unnecessary realloc for these cases.

I can propose a new patch to fix the pointer problem and add more
--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH] gianfar: fix headroom expansion code
Date: Friday, March 27, 2009 - 12:39 am

From: Li Yang <leoli@freescale.com>

I'm applying Stephen's patch for now, please send any improvements
relative to that.
--


[ message continues ]
[view original message]
From: Li Yang
Subject: Re: [PATCH] gianfar: fix headroom expansion code
Date: Friday, March 27, 2009 - 1:06 am

Ok.

However is it ok to use kfree() instead of kfree_skb() in Stephen's patch?

+               skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN);
+               if (!skb_new) {
+                       dev->stats.tx_errors++;
+                       kfree(skb);
+                       return NETDEV_TX_OK;
+               }

- Leo
--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH] gianfar: fix headroom expansion code
Date: Friday, March 27, 2009 - 1:11 am

From: Li Yang <leoli@freescale.com>

I'll fix this, thanks for noticing.
--


[ message continues ]
[view original message]
From: Li Yang
Subject: [PATCH] gianfar: only check headroom when FCB is needed
Date: Friday, March 27, 2009 - 1:01 am

Signed-off-by: Li Yang <leoli@freescale.com>
---
 drivers/net/gianfar.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 14f9b5e..6e28088 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1277,8 +1277,10 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	base = priv->tx_bd_base;
 
-	/* make space for additional header */
-	if (skb_headroom(skb) < GMAC_FCB_LEN) {
+	/* make space for additional header when fcb is needed */
+	if (((skb->ip_summed == CHECKSUM_PARTIAL) ||
+			(priv->vlgrp && vlan_tx_tag_present(skb))) &&
+			(skb_headroom(skb) < GMAC_FCB_LEN)) {
 		struct sk_buff *skb_new;
 
 		skb_new = skb_realloc_headroom(skb, GMAC_FCB_LEN);
-- 
1.5.4

--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH] gianfar: only check headroom when FCB is needed
Date: Friday, March 27, 2009 - 3:54 pm

From: Li Yang <leoli@freescale.com>

Applied, thanks.
--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [PATCH] gianfar: fix headroom expansion code
Date: Friday, March 27, 2009 - 12:38 am

From: Stephen Hemminger <shemminger@vyatta.com>

Applied, thanks Stephen.
--


[ message continues ]
Previous thread: 2.6.29 forcedeth hang W/O NAPI enabled by Adam Richter on Wednesday, March 25, 2009 - 5:06 pm. (2 messages)

Next thread: [PATCH] bridge: bad error handling when adding invalid ether address by Stephen Hemminger on Wednesday, March 25, 2009 - 8:57 pm. (2 messages)