[PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

The bridging device used a constant hard_header_len.  This will cause
headroom shortage for ports with additional hardware header.  The patch
makes bridging device to use the maximum value of all ports.

Signed-off-by: Li Yang <leoli@freescale.com>
---
Fixes the following BUG when using bridging with gianfar driver:

skb_under_panic: text:c0224b84 len:122 put:8 head:dfb81800 data:dfb817fa tail:0xdfb81874 end:0xdfb818a0 dev:eth1
------------[ cut here ]------------
Kernel BUG at c02d9444 [verbose debug info unavailable]
Oops: Exception in kernel mode, sig: 5 [#1]
Call Trace:
[df2dbb20] [c02d9444] skb_under_panic+0x48/0x5c (unreliable)
[df2dbb30] [c0224b94] gfar_start_xmit+0x384/0x400
[df2dbb60] [c02e1c8c] dev_hard_start_xmit+0x258/0x2cc
[df2dbba0] [c02f264c] __qdisc_run+0x5c/0x1f8
[df2dbbd0] [c02e4bf4] dev_queue_xmit+0x264/0x2d0
[df2dbbf0] [c036fdc8] br_dev_queue_push_xmit+0x90/0xf8
[df2dbc00] [c036fcc8] br_flood+0xc8/0x120
[df2dbc30] [c036ebe0] br_dev_xmit+0xbc/0xc0
[df2dbc40] [c02e1c8c] dev_hard_start_xmit+0x258/0x2cc
[df2dbc80] [c02e4c04] dev_queue_xmit+0x274/0x2d0
[df2dbca0] [c02ebaa8] neigh_resolve_output+0xfc/0x25c
............

 net/bridge/br_if.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 727c5c5..d34303d 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -348,6 +348,7 @@ void br_features_recompute(struct net_bridge *br)
 {
 	struct net_bridge_port *p;
 	unsigned long features, mask;
+	unsigned short max_hard_header_len = ETH_HLEN;
 
 	features = mask = br->feature_mask;
 	if (list_empty(&br->port_list))
@@ -358,7 +359,10 @@ void br_features_recompute(struct net_bridge *br)
 	list_for_each_entry(p, &br->port_list, list) {
 		features = netdev_increment_features(features,
 						     p->dev->features, mask);
+		if (p->dev->hard_header_len > max_hard_header_len)
+			max_hard_header_len = p->dev->hard_header_len;
 	}
+	br->dev->hard_header_len = max_hard_header_len;
 
 done:
 	br->dev->features = netdev_fix_features(features, NULL);
-- 
1.5.5.1.248.g4b17

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

On Fri, Mar 20, 2009 at 5:04 PM, Li Yang <leoli@freescale.com> wrote:
> The bridging device used a constant hard_header_len.  This will cause
> headroom shortage for ports with additional hardware header.  The patch
> makes bridging device to use the maximum value of all ports.
>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
> Fixes the following BUG when using bridging with gianfar driver:
>
> skb_under_panic: text:c0224b84 len:122 put:8 head:dfb81800 data:dfb817fa tail:0xdfb81874 end:0xdfb818a0 dev:eth1
> ------------[ cut here ]------------
> Kernel BUG at c02d9444 [verbose debug info unavailable]
> Oops: Exception in kernel mode, sig: 5 [#1]
> Call Trace:
> [df2dbb20] [c02d9444] skb_under_panic+0x48/0x5c (unreliable)
> [df2dbb30] [c0224b94] gfar_start_xmit+0x384/0x400
> [df2dbb60] [c02e1c8c] dev_hard_start_xmit+0x258/0x2cc
> [df2dbba0] [c02f264c] __qdisc_run+0x5c/0x1f8
> [df2dbbd0] [c02e4bf4] dev_queue_xmit+0x264/0x2d0
> [df2dbbf0] [c036fdc8] br_dev_queue_push_xmit+0x90/0xf8
> [df2dbc00] [c036fcc8] br_flood+0xc8/0x120
> [df2dbc30] [c036ebe0] br_dev_xmit+0xbc/0xc0
> [df2dbc40] [c02e1c8c] dev_hard_start_xmit+0x258/0x2cc
> [df2dbc80] [c02e4c04] dev_queue_xmit+0x274/0x2d0
> [df2dbca0] [c02ebaa8] neigh_resolve_output+0xfc/0x25c
> ............

Any comment about this?  Is it possible to be included in 2.6.29?

- Leo

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Li Yang <leoli@freescale.com>
Date: Mon, 23 Mar 2009 15:59:24 +0800

> Any comment about this?  Is it possible to be included in 2.6.29?

Patience please?  I reviewed and applied more than 80 patches
yesterday, maybe I'll get to your's after I recover from that.

Your patch is in the queue at:

	http://patchwork.ozlabs.org/project/netdev/list/

You can monitor it's state and what's happening to it here.

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

On Mon, Mar 23, 2009 at 4:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Li Yang <leoli@freescale.com>
> Date: Mon, 23 Mar 2009 15:59:24 +0800
>
>> Any comment about this?  Is it possible to be included in 2.6.29?
>
> Patience please?  I reviewed and applied more than 80 patches
> yesterday, maybe I'll get to your's after I recover from that.
>
> Your patch is in the queue at:
>
>        http://patchwork.ozlabs.org/project/netdev/list/
>
> You can monitor it's state and what's happening to it here.

Good to know it is being tracked.  After you have applied all those
patches, I was wondering if this one failed to get your attention.
Thanks.

- Leo

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Li Yang <leoli@freescale.com>
Date: Mon, 23 Mar 2009 16:15:34 +0800

> I was wondering if this one failed to get your attention.

yeah, happens all the time

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Stephen Hemminger]

On Fri, 20 Mar 2009 17:04:29 +0800
Li Yang <leoli@freescale.com> wrote:

> The bridging device used a constant hard_header_len.  This will cause
> headroom shortage for ports with additional hardware header.  The patch
> makes bridging device to use the maximum value of all ports.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---

That ensures big enough header for locally generated packets, but
any drivers that need bigger headroom still must handle bridged packets
that come in with smaller space. When bridging packets, the skb comes
from the allocation by the receiving driver. Almost all drivers will
use dev_alloc_skb() which will allocate NET_SKB_PAD (16) bytes of
additional headroom. This is used to hold copy of ethernet header for
the bridge/netfilter code.

So your patch is fine as an optimization but a driver can not safely
depend on any additional headroom. The driver must check if there
is space, and if no space is available, reallocate and copy.

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Mon, 23 Mar 2009 08:51:22 -0700

> That ensures big enough header for locally generated packets, but
> any drivers that need bigger headroom still must handle bridged packets
> that come in with smaller space. When bridging packets, the skb comes
> from the allocation by the receiving driver. Almost all drivers will
> use dev_alloc_skb() which will allocate NET_SKB_PAD (16) bytes of
> additional headroom. This is used to hold copy of ethernet header for
> the bridge/netfilter code.
> 
> So your patch is fine as an optimization but a driver can not safely
> depend on any additional headroom. The driver must check if there
> is space, and if no space is available, reallocate and copy.

We had some plans to deal with this kind of issue for wireless
too.  Let me see if I can find the RFC patch from that discussion...

Here it is, similar code would be added to the ipv4/ipv6 forwarding
paths:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7c1d446..6c06fba 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -600,6 +600,7 @@ struct net_device
  * Cache line mostly used on receive path (including eth_type_trans())
  */
 	unsigned long		last_rx;	/* Time of last Rx	*/
+	unsigned int		rx_alloc_extra;
 	/* Interface address info used in eth_type_trans() */
 	unsigned char		dev_addr[MAX_ADDR_LEN];	/* hw address, (before bcast 
 							because most packets are unicast) */
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index bdd7c35..531e483 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -42,6 +42,22 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
 		if (nf_bridge_maybe_copy_header(skb))
 			kfree_skb(skb);
 		else {
+			unsigned int headroom = skb_headroom(skb);
+			unsigned int hh_len = LL_RESERVED_SPACE(skb->dev);
+
+			if (headroom < hh_len) {
+				struct net_device *in_dev;
+				unsigned int extra;
+
+				in_dev = __dev_get_by_index(dev_net(skb->dev),
+							    skb->iif);
+				BUG_ON(!in_dev);
+
+				extra = hh_len - headroom;
+				if (extra >= in_dev->rx_alloc_extra)
+					in_dev->rx_alloc_extra = extra;
+			}
+
 			skb_push(skb, ETH_HLEN);
 
 			dev_queue_xmit(skb);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5c459f2..74a2515 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -255,11 +255,12 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
 		unsigned int length, gfp_t gfp_mask)
 {
 	int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
+	unsigned int extra = dev->rx_alloc_extra + NET_SKB_PAD;
 	struct sk_buff *skb;
 
-	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, 0, node);
+	skb = __alloc_skb(length + extra, gfp_mask, 0, node);
 	if (likely(skb)) {
-		skb_reserve(skb, NET_SKB_PAD);
+		skb_reserve(skb, extra);
 		skb->dev = dev;
 	}
 	return skb;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index f35eaea..86f0e36 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1562,13 +1562,13 @@ int ieee80211_subif_start_xmit(struct sk_buff *skb,
 	 * be cloned. This could happen, e.g., with Linux bridge code passing
 	 * us broadcast frames. */
 
-	if (head_need > 0 || skb_cloned(skb)) {
+	if (head_need > 0 || skb_header_cloned(skb)) {
 #if 0
 		printk(KERN_DEBUG "%s: need to reallocate buffer for %d bytes "
 		       "of headroom\n", dev->name, head_need);
 #endif
 
-		if (skb_cloned(skb))
+		if (skb_header_cloned(skb))
 			I802_DEBUG_INC(local->tx_expand_skb_head_cloned);
 		else
 			I802_DEBUG_INC(local->tx_expand_skb_head);

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Stephen Hemminger]

On Mon, 23 Mar 2009 15:20:28 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Mon, 23 Mar 2009 08:51:22 -0700
> 
> > That ensures big enough header for locally generated packets, but
> > any drivers that need bigger headroom still must handle bridged packets
> > that come in with smaller space. When bridging packets, the skb comes
> > from the allocation by the receiving driver. Almost all drivers will
> > use dev_alloc_skb() which will allocate NET_SKB_PAD (16) bytes of
> > additional headroom. This is used to hold copy of ethernet header for
> > the bridge/netfilter code.
> > 
> > So your patch is fine as an optimization but a driver can not safely
> > depend on any additional headroom. The driver must check if there
> > is space, and if no space is available, reallocate and copy.
> 
> We had some plans to deal with this kind of issue for wireless
> too.  Let me see if I can find the RFC patch from that discussion...
> 
> Here it is, similar code would be added to the ipv4/ipv6 forwarding
> paths:
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7c1d446..6c06fba 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -600,6 +600,7 @@ struct net_device
>   * Cache line mostly used on receive path (including eth_type_trans())
>   */
>  	unsigned long		last_rx;	/* Time of last Rx	*/
> +	unsigned int		rx_alloc_extra;
>  	/* Interface address info used in eth_type_trans() */
>  	unsigned char		dev_addr[MAX_ADDR_LEN];	/* hw address, (before bcast 
>  							because most packets are unicast) */
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index bdd7c35..531e483 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -42,6 +42,22 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
>  		if (nf_bridge_maybe_copy_header(skb))
>  			kfree_skb(skb);
>  		else {
> +			unsigned int headroom = skb_headroom(skb);
> +			unsigned int hh_len = LL_RESERVED_SPACE(skb->dev);
> +
> +			if (headroom < hh_len) {
> +				struct net_device *in_dev;
> +				unsigned int extra;
> +
> +				in_dev = __dev_get_by_index(dev_net(skb->dev),
> +							    skb->iif);
> +				BUG_ON(!in_dev);
> +
> +				extra = hh_len - headroom;
> +				if (extra >= in_dev->rx_alloc_extra)
> +					in_dev->rx_alloc_extra = extra;
> +			}

So you dynamically compute the additional space but if the space was
an awkward size, could it cause driver to breaks alignment assumptions?

And you didn't fixup the skb that is about to gag in the skb to make
more space, so transmitting device driver (gfar) is going to overwrite or die.

In summary, good idea, but may not solve the problem

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Mon, 23 Mar 2009 15:45:17 -0700

> So you dynamically compute the additional space but if the space was
> an awkward size, could it cause driver to breaks alignment assumptions?

Yes, you'd need to 16-byte align or something like that.

> And you didn't fixup the skb that is about to gag in the skb to make
> more space, so transmitting device driver (gfar) is going to overwrite or die.

This particular instance will do the headroom reallocation,
that's unavoidable during the size transition event.

The headroom checks can't ever be removed, but we won't hit
them in the fast path after the adjustment is made by my
patch.

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Li Yang <leoli@freescale.com>
Date: Fri, 20 Mar 2009 17:04:29 +0800

> The bridging device used a constant hard_header_len.  This will cause
> headroom shortage for ports with additional hardware header.  The patch
> makes bridging device to use the maximum value of all ports.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>

Your driver must be able to cope with any amount of
available headroom, no matter what hacks we put into
the bridging layer.

Please fix your driver, I'm not applying this patch.

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

On Wed, Mar 25, 2009 at 2:40 PM, David Miller <davem@davemloft.net> wrote:
> From: Li Yang <leoli@freescale.com>
> Date: Fri, 20 Mar 2009 17:04:29 +0800
>
>> The bridging device used a constant hard_header_len.  This will cause
>> headroom shortage for ports with additional hardware header.  The patch
>> makes bridging device to use the maximum value of all ports.
>>
>> Signed-off-by: Li Yang <leoli@freescale.com>
>
> Your driver must be able to cope with any amount of
> available headroom, no matter what hacks we put into
> the bridging layer.
>
> Please fix your driver, I'm not applying this patch.

Ok.  But it's not good to reallocate every packet generated locally.
Why not take this patch too?

- Leo

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Li Yang <leoli@freescale.com>
Date: Wed, 25 Mar 2009 15:05:20 +0800

> On Wed, Mar 25, 2009 at 2:40 PM, David Miller <davem@davemloft.net> wrote:
> > From: Li Yang <leoli@freescale.com>
> > Date: Fri, 20 Mar 2009 17:04:29 +0800
> >
> >> The bridging device used a constant hard_header_len.  This will cause
> >> headroom shortage for ports with additional hardware header.  The patch
> >> makes bridging device to use the maximum value of all ports.
> >>
> >> Signed-off-by: Li Yang <leoli@freescale.com>
> >
> > Your driver must be able to cope with any amount of
> > available headroom, no matter what hacks we put into
> > the bridging layer.
> >
> > Please fix your driver, I'm not applying this patch.
> 
> Ok.  But it's not good to reallocate every packet generated locally.
> Why not take this patch too?

Because as Stephen showed it didn't handle all cases.

Look at the patch I posted, that's the way to go.

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

On Wed, Mar 25, 2009 at 3:06 PM, David Miller <davem@davemloft.net> wrote:
> From: Li Yang <leoli@freescale.com>
> Date: Wed, 25 Mar 2009 15:05:20 +0800
>
>> On Wed, Mar 25, 2009 at 2:40 PM, David Miller <davem@davemloft.net> wrote:
>> > From: Li Yang <leoli@freescale.com>
>> > Date: Fri, 20 Mar 2009 17:04:29 +0800
>> >
>> >> The bridging device used a constant hard_header_len.  This will cause
>> >> headroom shortage for ports with additional hardware header.  The patch
>> >> makes bridging device to use the maximum value of all ports.
>> >>
>> >> Signed-off-by: Li Yang <leoli@freescale.com>
>> >
>> > Your driver must be able to cope with any amount of
>> > available headroom, no matter what hacks we put into
>> > the bridging layer.
>> >
>> > Please fix your driver, I'm not applying this patch.
>>
>> Ok.  But it's not good to reallocate every packet generated locally.
>> Why not take this patch too?
>
> Because as Stephen showed it didn't handle all cases.
>
> Look at the patch I posted, that's the way to go.

Patch coming right away.  However I have some comment about your way.
The choice is yours.

- Leo

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

On Tue, Mar 24, 2009 at 6:20 AM, David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Mon, 23 Mar 2009 08:51:22 -0700
>
>> That ensures big enough header for locally generated packets, but
>> any drivers that need bigger headroom still must handle bridged packets
>> that come in with smaller space. When bridging packets, the skb comes
>> from the allocation by the receiving driver. Almost all drivers will
>> use dev_alloc_skb() which will allocate NET_SKB_PAD (16) bytes of
>> additional headroom. This is used to hold copy of ethernet header for
>> the bridge/netfilter code.
>>
>> So your patch is fine as an optimization but a driver can not safely
>> depend on any additional headroom. The driver must check if there
>> is space, and if no space is available, reallocate and copy.
>
> We had some plans to deal with this kind of issue for wireless
> too.  Let me see if I can find the RFC patch from that discussion...
>
> Here it is, similar code would be added to the ipv4/ipv6 forwarding
> paths:
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 7c1d446..6c06fba 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -600,6 +600,7 @@ struct net_device
>  * Cache line mostly used on receive path (including eth_type_trans())
>  */
>        unsigned long           last_rx;        /* Time of last Rx      */
> +       unsigned int            rx_alloc_extra;
>        /* Interface address info used in eth_type_trans() */
>        unsigned char           dev_addr[MAX_ADDR_LEN]; /* hw address, (before bcast
>                                                        because most packets are unicast) */
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index bdd7c35..531e483 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -42,6 +42,22 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
>                if (nf_bridge_maybe_copy_header(skb))
>                        kfree_skb(skb);
>                else {
> +                       unsigned int headroom = skb_headroom(skb);
> +                       unsigned int hh_len = LL_RESERVED_SPACE(skb->dev);
> +
> +                       if (headroom < hh_len) {
> +                               struct net_device *in_dev;
> +                               unsigned int extra;
> +
> +                               in_dev = __dev_get_by_index(dev_net(skb->dev),
> +                                                           skb->iif);
> +                               BUG_ON(!in_dev);
> +
> +                               extra = hh_len - headroom;
> +                               if (extra >= in_dev->rx_alloc_extra)
> +                                       in_dev->rx_alloc_extra = extra;
> +                       }
> +
>                        skb_push(skb, ETH_HLEN);
>
>                        dev_queue_xmit(skb);

Dynamically adjusting is a good idea, but the rx_alloc_extra can only
go up not the other way down in your code.  Another thought is that if
you re-allocate skb here the driver would be saved from checking the
headroom in the fastpath, am I right?

- Leo

[PATCH] gianfar: reallocate skb when headroom is not enough for fcb

[Li Yang]

Gianfar uses a hardware header FCB for offloading.  However when used
with bridging or IP forwarding, TX skb might not have enough headroom
for the FCB.  Reallocate skb for such cases.

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

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 9831b3f..29dc4ee 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1206,10 +1206,19 @@ static int gfar_enet_open(struct net_device *dev)
 	return err;
 }
 
-static inline struct txfcb *gfar_add_fcb(struct sk_buff *skb)
+static inline struct txfcb *gfar_add_fcb(struct sk_buff **skbp)
 {
-	struct txfcb *fcb = (struct txfcb *)skb_push (skb, GMAC_FCB_LEN);
-
+	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);
 	cacheable_memzero(fcb, GMAC_FCB_LEN);
 
 	return fcb;
@@ -1330,18 +1339,20 @@ static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Set up checksumming */
 	if (CHECKSUM_PARTIAL == skb->ip_summed) {
-		fcb = gfar_add_fcb(skb);
-		lstatus |= BD_LFLAG(TXBD_TOE);
-		gfar_tx_checksum(skb, fcb);
+		fcb = gfar_add_fcb(&skb);
+		if (likely(fcb != NULL)) {
+			lstatus |= BD_LFLAG(TXBD_TOE);
+			gfar_tx_checksum(skb, fcb);
+		}
 	}
 
 	if (priv->vlgrp && vlan_tx_tag_present(skb)) {
-		if (unlikely(NULL == fcb)) {
-			fcb = gfar_add_fcb(skb);
+		if (unlikely(NULL == fcb))
+			fcb = gfar_add_fcb(&skb);
+		if (likely(fcb != NULL)) {
 			lstatus |= BD_LFLAG(TXBD_TOE);
+			gfar_tx_vlan(skb, fcb);
 		}
-
-		gfar_tx_vlan(skb, fcb);
 	}
 
 	/* setup the TxBD length and buffer pointer for the first BD */
-- 
1.5.4

Re: [PATCH] gianfar: reallocate skb when headroom is not enough for fcb

[Stephen Hemminger]

On Wed, 25 Mar 2009 17:15:33 +0800
Li Yang <leoli@freescale.com> wrote:

> Gianfar uses a hardware header FCB for offloading.  However when used
> with bridging or IP forwarding, TX skb might not have enough headroom
> for the FCB.  Reallocate skb for such cases.
> 
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  drivers/net/gianfar.c |   31 +++++++++++++++++++++----------
>  1 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index 9831b3f..29dc4ee 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -1206,10 +1206,19 @@ static int gfar_enet_open(struct net_device *dev)
>  	return err;
>  }
>  
> -static inline struct txfcb *gfar_add_fcb(struct sk_buff *skb)
> +static inline struct txfcb *gfar_add_fcb(struct sk_buff **skbp)
>  {
> -	struct txfcb *fcb = (struct txfcb *)skb_push (skb, GMAC_FCB_LEN);
> -
> +	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);
>  	cacheable_memzero(fcb, GMAC_FCB_LEN);
> 

Overall, this looks like the wrong code (not copying back new skb)
and in awkward place. Also your version doesn't handle case where
skb headroom is not writable.

Why not:

--- a/drivers/net/gianfar.c	2009-03-25 08:39:03.890718197 -0700
+++ b/drivers/net/gianfar.c	2009-03-25 08:51:39.733279404 -0700
@@ -1309,6 +1309,17 @@ static int gfar_start_xmit(struct sk_buf
 	unsigned long flags;
 	unsigned int nr_frags, length;
 
+	/* make sure there is space and can write FCB */
+	if (!skb_clone_writeable(skb, GMAC_FCB_LEN)) {
+		struct sk_buff *skb2;
+
+		skb2 = skb_realloc_headroom(skb, GMAC_FCB_LEN);
+		kfree_skb(skb);
+		if (!skb2)
+			return NETDEV_TX_OK;
+		skb = skb2;
+	}
+
 	base = priv->tx_bd_base;
 
 	/* total number of fragments in the SKB */

Re: [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[David Miller]

From: Li Yang <leoli@freescale.com>
Date: Wed, 25 Mar 2009 16:43:53 +0800

> Dynamically adjusting is a good idea, but the rx_alloc_extra can only
> go up not the other way down in your code.

That's not a problem.

> Another thought is that if you re-allocate skb here the driver would
> be saved from checking the headroom in the fastpath, am I right?

You're going to need it for other paths.  There are other kinds of
tunnels et al. that eat that headroom space on you.

Re: [PATCH] gianfar: reallocate skb when headroom is not enough for fcb

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 25 Mar 2009 08:53:33 -0700

> Overall, this looks like the wrong code (not copying back new skb)
> and in awkward place. Also your version doesn't handle case where
> skb headroom is not writable.

headroom is always writable and usable by the driver, this
is the exact same code sequence we use in drivers/net/niu.c
for this purpose:

	len = sizeof(struct tx_pkt_hdr) + 15;
	if (skb_headroom(skb) < len) {
		struct sk_buff *skb_new;

		skb_new = skb_realloc_headroom(skb, len);
		if (!skb_new) {
			rp->tx_errors++;
			goto out_drop;
		}
		kfree_skb(skb);
		skb = skb_new;

otherwise the code we just came from wouldn't be able
to push even the ethernet header there.