[PATCH] tg3: Do not include vlan acceleration features in vlan_features

[PATCH] tg3: Do not include vlan acceleration features in vlan_features

[Vlad Yasevich]

Including hardware acceleration features in vlan_features breaks
stacked vlans (Q-in-Q) by marking the bottom vlan interface as
capable of acceleration.  This causes one of the tags to be lost
and the packets are sent with a sing vlan header.

CC: Nithin Nayak Sujir <nsujir@broadcom.com>
CC: Michael Chan <mchan@broadcom.com>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 3b6d0ba..70a225c8 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -17649,8 +17649,6 @@ static int tg3_init_one(struct pci_dev *pdev,
 
 	tg3_init_bufmgr_config(tp);
 
-	features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
-
 	/* 5700 B0 chips do not support checksumming correctly due
 	 * to hardware bugs.
 	 */
@@ -17682,7 +17680,8 @@ static int tg3_init_one(struct pci_dev *pdev,
 			features |= NETIF_F_TSO_ECN;
 	}
 
-	dev->features |= features;
+	dev->features |= features | NETIF_F_HW_VLAN_CTAG_TX |
+			 NETIF_F_HW_VLAN_CTAG_RX;
 	dev->vlan_features |= features;
 
 	/*
-- 
1.8.5.3

Re: [PATCH] tg3: Do not include vlan acceleration features in vlan_features

[David Miller]

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Mon, 24 Mar 2014 17:52:12 -0400

> Including hardware acceleration features in vlan_features breaks
> stacked vlans (Q-in-Q) by marking the bottom vlan interface as
> capable of acceleration.  This causes one of the tags to be lost
> and the packets are sent with a sing vlan header.
> 
> CC: Nithin Nayak Sujir <nsujir@broadcom.com>
> CC: Michael Chan <mchan@broadcom.com>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Applied and queued up for -stable, thanks.

This is at least the second time I've seen a fix exactly like this
one, it certainly therefore deserves a tree-wide audit if anyone has
time for that.

[PATCH net] vlan: Mask off vlan acceleration features on vlan device.

[Vlad Yasevich]

Some drivers incorrectly assign vlan acceleration features to
vlan_features thus causing issues for Q-in-Q vlan configurations.
Prevent this once and for all by masking off acceleration features
for vlan devices until such time as we support stacked acceleration.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---

Hi David

> This is at least the second time I've seen a fix exactly like this
> one, it certainly therefore deserves a tree-wide audit if anyone has
> time for that.

How about this approach instead?

 include/linux/netdev_features.h | 7 +++++++
 net/8021q/vlan_dev.c            | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 1005ebf..84a4e5f 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -163,4 +163,11 @@ enum {
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
 
+#define NETIF_F_VLAN_FEATURES	(NETIF_F_HW_VLAN_CTAG_FILTER | \
+				 NETIF_F_HW_VLAN_STAG_RX | \
+				 NETIF_F_HW_VLAN_STAG_TX | \
+				 NETIF_F_HW_VLAN_STAG_FILTER | \
+				 NETIF_F_HW_VLAN_STAG_RX | \
+				 NETIF_F_HW_VLAN_STAG_TX)
+
 #endif	/* _LINUX_NETDEV_FEATURES_H */
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index a9591ff..d99e8df 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -576,7 +576,8 @@ static int vlan_dev_init(struct net_device *dev)
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CSUM |
 			   NETIF_F_ALL_FCOE;
 
-	dev->features |= real_dev->vlan_features | NETIF_F_LLTX;
+	dev->features |= (real_dev->vlan_features & ~NETIF_F_VLAN_FEATURES) |
+			 NETIF_F_LLTX;
 	dev->gso_max_size = real_dev->gso_max_size;
 
 	/* ipv6 shared card related stuff */
@@ -651,6 +652,7 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
 	features &= real_dev->features;
 
 	features |= old_features & NETIF_F_SOFT_FEATURES;
+	features &= ~NETIF_F_VLAN_FEATURES;
 	features |= NETIF_F_LLTX;
 
 	return features;
-- 
1.8.5.3

Re: [PATCH net] vlan: Mask off vlan acceleration features on vlan device.

[Vlad Yasevich]

On 03/26/2014 04:25 PM, Vlad Yasevich wrote:
> Some drivers incorrectly assign vlan acceleration features to
> vlan_features thus causing issues for Q-in-Q vlan configurations.
> Prevent this once and for all by masking off acceleration features
> for vlan devices until such time as we support stacked acceleration.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
> 
> Hi David
> 
>> This is at least the second time I've seen a fix exactly like this
>> one, it certainly therefore deserves a tree-wide audit if anyone has
>> time for that.
> 
> How about this approach instead?
> 
>  include/linux/netdev_features.h | 7 +++++++
>  net/8021q/vlan_dev.c            | 4 +++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 1005ebf..84a4e5f 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -163,4 +163,11 @@ enum {
>  /* changeable features with no special hardware requirements */
>  #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
>  
> +#define NETIF_F_VLAN_FEATURES	(NETIF_F_HW_VLAN_CTAG_FILTER | \
> +				 NETIF_F_HW_VLAN_STAG_RX | \
> +				 NETIF_F_HW_VLAN_STAG_TX | \
> +				 NETIF_F_HW_VLAN_STAG_FILTER | \
> +				 NETIF_F_HW_VLAN_STAG_RX | \
> +				 NETIF_F_HW_VLAN_STAG_TX)
> +

sorry, cut/paste error.  Will fix.

-vlad

>  #endif	/* _LINUX_NETDEV_FEATURES_H */
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index a9591ff..d99e8df 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -576,7 +576,8 @@ static int vlan_dev_init(struct net_device *dev)
>  			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CSUM |
>  			   NETIF_F_ALL_FCOE;
>  
> -	dev->features |= real_dev->vlan_features | NETIF_F_LLTX;
> +	dev->features |= (real_dev->vlan_features & ~NETIF_F_VLAN_FEATURES) |
> +			 NETIF_F_LLTX;
>  	dev->gso_max_size = real_dev->gso_max_size;
>  
>  	/* ipv6 shared card related stuff */
> @@ -651,6 +652,7 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
>  	features &= real_dev->features;
>  
>  	features |= old_features & NETIF_F_SOFT_FEATURES;
> +	features &= ~NETIF_F_VLAN_FEATURES;
>  	features |= NETIF_F_LLTX;
>  
>  	return features;
> 

[PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[Vlad Yasevich]

Some drivers incorrectly assign vlan acceleration features to
vlan_features thus causing issues for Q-in-Q vlan configurations.
Prevent this once and for all by masking off acceleration features
for vlan devices until such time as we support stacked acceleration.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/netdev_features.h | 7 +++++++
 net/8021q/vlan_dev.c            | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 1005ebf..5a09a48 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -163,4 +163,11 @@ enum {
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
 
+#define NETIF_F_VLAN_FEATURES	(NETIF_F_HW_VLAN_CTAG_FILTER | \
+				 NETIF_F_HW_VLAN_CTAG_RX | \
+				 NETIF_F_HW_VLAN_CTAG_TX | \
+				 NETIF_F_HW_VLAN_STAG_FILTER | \
+				 NETIF_F_HW_VLAN_STAG_RX | \
+				 NETIF_F_HW_VLAN_STAG_TX)
+
 #endif	/* _LINUX_NETDEV_FEATURES_H */
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index a9591ff..d99e8df 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -576,7 +576,8 @@ static int vlan_dev_init(struct net_device *dev)
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CSUM |
 			   NETIF_F_ALL_FCOE;
 
-	dev->features |= real_dev->vlan_features | NETIF_F_LLTX;
+	dev->features |= (real_dev->vlan_features & ~NETIF_F_VLAN_FEATURES) |
+			 NETIF_F_LLTX;
 	dev->gso_max_size = real_dev->gso_max_size;
 
 	/* ipv6 shared card related stuff */
@@ -651,6 +652,7 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
 	features &= real_dev->features;
 
 	features |= old_features & NETIF_F_SOFT_FEATURES;
+	features &= ~NETIF_F_VLAN_FEATURES;
 	features |= NETIF_F_LLTX;
 
 	return features;
-- 
1.8.5.3

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[David Miller]

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Wed, 26 Mar 2014 16:28:31 -0400

> Some drivers incorrectly assign vlan acceleration features to
> vlan_features thus causing issues for Q-in-Q vlan configurations.
> Prevent this once and for all by masking off acceleration features
> for vlan devices until such time as we support stacked acceleration.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

So what if we do have chips that can offload Q-in-Q?  Will we use
a new flag?

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[Patrick McHardy]

On Wed, Mar 26, 2014 at 05:10:38PM -0400, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Wed, 26 Mar 2014 16:28:31 -0400
> 
> > Some drivers incorrectly assign vlan acceleration features to
> > vlan_features thus causing issues for Q-in-Q vlan configurations.
> > Prevent this once and for all by masking off acceleration features
> > for vlan devices until such time as we support stacked acceleration.
> > 
> > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> So what if we do have chips that can offload Q-in-Q?  Will we use
> a new flag?

For 802.1ad that was the intention, using the NETIF_F_HW_VLAN_STAG_* flags.

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[Vlad Yasevich]

On 03/26/2014 05:15 PM, Patrick McHardy wrote:
> On Wed, Mar 26, 2014 at 05:10:38PM -0400, David Miller wrote:
>> From: Vlad Yasevich <vyasevic@redhat.com>
>> Date: Wed, 26 Mar 2014 16:28:31 -0400
>>
>>> Some drivers incorrectly assign vlan acceleration features to
>>> vlan_features thus causing issues for Q-in-Q vlan configurations.
>>> Prevent this once and for all by masking off acceleration features
>>> for vlan devices until such time as we support stacked acceleration.
>>>
>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>
>> So what if we do have chips that can offload Q-in-Q?  Will we use
>> a new flag?
> 
> For 802.1ad that was the intention, using the NETIF_F_HW_VLAN_STAG_* flags.
> 

But we can't really do that as we don't carry multiple vlan_tci in the
skb.  If the card advertises VLAN_STAG_*, then we'll offload that, but
CTAG will have to be computed by software.  More infrastructure is
needed to support nested vlan offloads.

-vlad

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[Vlad Yasevich]

On 03/26/2014 05:10 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Wed, 26 Mar 2014 16:28:31 -0400
> 
>> Some drivers incorrectly assign vlan acceleration features to
>> vlan_features thus causing issues for Q-in-Q vlan configurations.
>> Prevent this once and for all by masking off acceleration features
>> for vlan devices until such time as we support stacked acceleration.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> So what if we do have chips that can offload Q-in-Q?  Will we use
> a new flag?
> 

If there are chips that can take both the inner and outer tags an put
both vlan headers on the packet, then we'll need a lot more then just
a new flag to support that since the skbs will need to carry 2 vlan_tci
values in that case.

With this patch you can still offload the outer tag, whether STAG and
or CTAG (old Q-in-Q), but the inner tag can't be offloaded.

-vlad

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[Patrick McHardy]

On Wed, Mar 26, 2014 at 08:53:12PM -0400, Vlad Yasevich wrote:
> On 03/26/2014 05:15 PM, Patrick McHardy wrote:
> > On Wed, Mar 26, 2014 at 05:10:38PM -0400, David Miller wrote:
> >> From: Vlad Yasevich <vyasevic@redhat.com>
> >> Date: Wed, 26 Mar 2014 16:28:31 -0400
> >>
> >>> Some drivers incorrectly assign vlan acceleration features to
> >>> vlan_features thus causing issues for Q-in-Q vlan configurations.
> >>> Prevent this once and for all by masking off acceleration features
> >>> for vlan devices until such time as we support stacked acceleration.
> >>>
> >>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>
> >> So what if we do have chips that can offload Q-in-Q?  Will we use
> >> a new flag?
> > 
> > For 802.1ad that was the intention, using the NETIF_F_HW_VLAN_STAG_* flags.
> > 
> 
> But we can't really do that as we don't carry multiple vlan_tci in the
> skb.  If the card advertises VLAN_STAG_*, then we'll offload that, but
> CTAG will have to be computed by software.  More infrastructure is
> needed to support nested vlan offloads.

True, we'd need more room in the skb to store both tags for Q-in-Q.

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[David Miller]

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Wed, 26 Mar 2014 16:28:31 -0400

> Some drivers incorrectly assign vlan acceleration features to
> vlan_features thus causing issues for Q-in-Q vlan configurations.
> Prevent this once and for all by masking off acceleration features
> for vlan devices until such time as we support stacked acceleration.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

Vlad, I've thought more about this, I'd rather we emit a warning so
we can fix the drivers.

If they are setting the flags wrong, we probably want to go take a
look to see if they are doing anything else related wrong too.

Re: [PATCH v2 net] vlan: Mask off vlan acceleration features on vlan device.

[Vlad Yasevich]

On 03/27/2014 03:12 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Wed, 26 Mar 2014 16:28:31 -0400
> 
>> Some drivers incorrectly assign vlan acceleration features to
>> vlan_features thus causing issues for Q-in-Q vlan configurations.
>> Prevent this once and for all by masking off acceleration features
>> for vlan devices until such time as we support stacked acceleration.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> Vlad, I've thought more about this, I'd rather we emit a warning so
> we can fix the drivers.
> 
> If they are setting the flags wrong, we probably want to go take a
> look to see if they are doing anything else related wrong too.
> 

OK.  I'll rework the patch to emit a warning.  I took a quick glance
through all the devices that set vlan_features and there are 3 more
the have vlan_features wrong.  I'll include those when I re-submit.

Thanks
-vlad