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;
 
 ...
[ message continues ]

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

- Leo
--

[ message continues ]

From: Li Yang <leoli@freescale.com>

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.
--

[ message continues ]

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
--

[ message continues ]

From: Li Yang <leoli@freescale.com>

yeah, happens all the time
--

[ message continues ]

On Fri, 20 Mar 2009 17:04:29 +0800

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.
--

[ message continues ]

From: Stephen Hemminger <shemminger@linux-foundation.org>

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 + ...
[ message continues ]

On Mon, 23 Mar 2009 15:20:28 -0700 (PDT)

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
--

[ message continues ]

From: Stephen Hemminger <shemminger@linux-foundation.org>


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.
--

[ message continues ]

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
--

[ message continues ]

From: Li Yang <leoli@freescale.com>


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

[ message continues ]

From: 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.
--

[ message continues ]

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

- Leo
--

[ message continues ]

From: Li Yang <leoli@freescale.com>

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

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

[ message continues ]

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

- Leo
--

[ message continues ]

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

--

[ message continues ]

On Wed, 25 Mar 2009 17:15:33 +0800

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 */

--

[ message continues ]

From: Stephen Hemminger <shemminger@vyatta.com>

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.
--

[ message continues ]