Skip to contentSkip to conversation

tg3: fix VLAN tagging regression

PATCH Superseded Kasper Pedersen 5 participants 8 messages Sep 20, 2011 – Sep 21, 2011

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Kasper Pedersen

author
#0 lore raw

PATCH 1/1 tg3: fix VLAN tagging regression

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

Change to use the CONFIG_VLAN_xxx defines instead. Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>
---
 drivers/net/tg3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/tg3.c +1 -1
index dc3fbf6..a00d21b 100644
@@ -6234,7 +6234,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 
-#ifdef BCM_KERNEL_SUPPORTS_8021Q
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if (vlan_tx_tag_present(skb)) {
 		base_flags |= TXD_FLAG_VLAN;
 		vlan = vlan_tx_tag_get(skb);
Acked-by on v1 1/1
#1 lore raw
Quoted 8 lines · Kasper Pedersen

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

Quoted 4 lines

Change to use the CONFIG_VLAN_xxx defines instead. Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>

Yes. This is correct.

Acked-by Matt Carlson <[email protected]>
Quoted 19 lines
---
 drivers/net/tg3.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/net/tg3.c +1 -1
index dc3fbf6..a00d21b 100644
@@ -6234,7 +6234,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 
-#ifdef BCM_KERNEL_SUPPORTS_8021Q
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if (vlan_tx_tag_present(skb)) {
 		base_flags |= TXD_FLAG_VLAN;
 		vlan = vlan_tx_tag_get(skb);
#2 lore raw
Quoted 9 lines · Matt Carlson
Quoted 8 lines · Kasper Pedersen

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
    tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

Quoted 8 lines
Quoted 4 lines

Change to use the CONFIG_VLAN_xxx defines instead. Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>

Yes.  This is correct.

Acked-by Matt Carlson <[email protected]>

Actually, please don't do this. Those config #define's refer to the 802.1q module that creates vlan devices only. The rest of the network stack has code for dealing with vlan packets that is not protected by config guards, so there's no reason that drivers should be. The correct thing to do here is just drop the test altogether and unconditionally include the code.

author
#3 lore raw
Quoted 17 lines · Jesse Gross
Quoted 16 lines · Matt Carlson
Quoted 8 lines · Kasper Pedersen

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

Quoted 4 lines

Change to use the CONFIG_VLAN_xxx defines instead. Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>

Yes. This is correct.

Quoted 9 lines
Quoted 1 line
Acked-by Matt Carlson <[email protected]>

Actually, please don't do this. Those config #define's refer to the 802.1q module that creates vlan devices only. The rest of the network stack has code for dealing with vlan packets that is not protected by config guards, so there's no reason that drivers should be. The correct thing to do here is just drop the test altogether and unconditionally include the code.

currently testing, and will post a just-remove-ifdef in a few hours, assuming it behaves as expected on a no-802.1q-module config.

tg3 has one more place where this is done, and it looks as if the code in that location doesn't quite mesh with the comment.

/Kasper Pedersen

#4 lore raw
Quoted 30 lines · Kasper Pedersen
Quoted 17 lines · Jesse Gross
Quoted 16 lines · Matt Carlson
Quoted 8 lines · Kasper Pedersen

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

Quoted 4 lines

Change to use the CONFIG_VLAN_xxx defines instead. Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>

Yes. This is correct.

Quoted 9 lines
Quoted 1 line
Acked-by Matt Carlson <[email protected]>

Actually, please don't do this. Those config #define's refer to the 802.1q module that creates vlan devices only. The rest of the network stack has code for dealing with vlan packets that is not protected by config guards, so there's no reason that drivers should be. The correct thing to do here is just drop the test altogether and unconditionally include the code.

Quoted 6 lines

currently testing, and will post a just-remove-ifdef in a few hours, assuming it behaves as expected on a no-802.1q-module config.

tg3 has one more place where this is done, and it looks as if the code in that location doesn't quite mesh with the comment.

Thanks for finding this bug, this hit me last day on a BCM5755M but I had no time to investigate.

#5 lore raw
Quoted 3 lines · Eric Dumazet

Thanks for finding this bug, this hit me last day on a BCM5755M but I had no time to investigate.

I meant a BCM5715S, but its probably a generic bug anyway ;)

author
#6 lore raw

PATCH v2 1/1 tg3: fix VLAN tagging regression

In reply to Kasper Pedersen

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

v2: We can just remove the test. vlan_tx_tag_present is valid regardless of whether the 802.1q module is built.

Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>
---
 drivers/net/tg3.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/net/tg3.c +0 -2
index dc3fbf6..4a1374d 100644
@@ -6234,12 +6234,10 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 
-#ifdef BCM_KERNEL_SUPPORTS_8021Q
 	if (vlan_tx_tag_present(skb)) {
 		base_flags |= TXD_FLAG_VLAN;
 		vlan = vlan_tx_tag_get(skb);
 	}
-#endif
 
 	if (tg3_flag(tp, USE_JUMBO_BDFLAG) &&
 	    !mss && skb->len > VLAN_ETH_FRAME_LEN)
maintainer Applied
#7 lore raw

From: Kasper Pedersen <[email protected]>
Date: Wed, 21 Sep 2011 00:41:17 +0200

Quoted 8 lines

commit 92cd3a17ce9c719abb4c28dee3438e0c641f8de4
tg3: Simplify tx bd assignments

broke VLAN tagging on outbound packets.
It ifdef'ed BCM_KERNEL_SUPPORTS_8021Q, but this is not set anywhere. So vlan never gets set, and all packets are sent with vlan=0.

Quoted 7 lines

v2: We can just remove the test. vlan_tx_tag_present is valid regardless of whether the 802.1q module is built.

Tested on BCM5721 rev 11.

Signed-off-by Kasper Pedersen <[email protected]>

Applied.