[net-next 6/10] bnx2x: Update vlan_features

[net-next 6/10] bnx2x: Update vlan_features

[Eilon Greenstein]

As noted by Or Gerlitz <ogerlitz@Voltaire.com>, the vlan_features was not
updated

Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/bnx2x_main.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 18c3803..b404a9b 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -9310,9 +9310,17 @@ static int bnx2x_set_tso(struct net_device *dev, u32 data)
 	if (data) {
 		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
 		dev->features |= NETIF_F_TSO6;
+#ifdef BCM_VLAN
+		dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
+		dev->vlan_features |= NETIF_F_TSO6;
+#endif
 	} else {
 		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
 		dev->features &= ~NETIF_F_TSO6;
+#ifdef BCM_VLAN
+		dev->vlan_features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
+		dev->vlan_features &= ~NETIF_F_TSO6;
+#endif
 	}

 	return 0;
@@ -11143,12 +11151,19 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
 	dev->features |= NETIF_F_HW_CSUM;
 	if (bp->flags & USING_DAC_FLAG)
 		dev->features |= NETIF_F_HIGHDMA;
+	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
+	dev->features |= NETIF_F_TSO6;
 #ifdef BCM_VLAN
 	dev->features |= (NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX);
 	bp->flags |= (HW_VLAN_RX_FLAG | HW_VLAN_TX_FLAG);
+
+	dev->vlan_features |= NETIF_F_SG;
+	dev->vlan_features |= NETIF_F_HW_CSUM;
+	if (bp->flags & USING_DAC_FLAG)
+		dev->vlan_features |= NETIF_F_HIGHDMA;
+	dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
+	dev->vlan_features |= NETIF_F_TSO6;
 #endif
-	dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
-	dev->features |= NETIF_F_TSO6;

 	return 0;

--
1.5.4.3

Re: [net-next 6/10] bnx2x: Update vlan_features

[Patrick McHardy]

Eilon Greenstein wrote:
> As noted by Or Gerlitz <ogerlitz@Voltaire.com>, the vlan_features was not
> updated
> 
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
>  drivers/net/bnx2x_main.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 18c3803..b404a9b 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -9310,9 +9310,17 @@ static int bnx2x_set_tso(struct net_device *dev, u32 data)
>  	if (data) {
>  		dev->features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
>  		dev->features |= NETIF_F_TSO6;
> +#ifdef BCM_VLAN
> +		dev->vlan_features |= (NETIF_F_TSO | NETIF_F_TSO_ECN);
> +		dev->vlan_features |= NETIF_F_TSO6;
> +#endif
>  	} else {
>  		dev->features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
>  		dev->features &= ~NETIF_F_TSO6;
> +#ifdef BCM_VLAN
> +		dev->vlan_features &= ~(NETIF_F_TSO | NETIF_F_TSO_ECN);
> +		dev->vlan_features &= ~NETIF_F_TSO6;
> +#endif

vlan_features doesn't need to be updated, the resulting dev->features
of the VLAN device is computed as the intersection of dev->features
and dev->vlan_features.

Re: [net-next 6/10] bnx2x: Update vlan_features

[Or Gerlitz]

Patrick McHardy wrote:
> vlan_features doesn't need to be updated, the resulting dev->features
> of the VLAN device is computed as the intersection of dev->features
> and dev->vlan_features.

I'm not sure to follow, do you claim that the patches to bnx2x and bonding aren't needed to make vlans set on top of such devices to support these features?

For example, on two 2.6.30 systems I have here, where one uses Intel/igb and the second uses Broadcom/tg3 I can see that vlan devices on top of igb have features while those on top of tg3 has none


2.6.30/igb/8021q
# cat /sys/class/net/eth1/features
0x114bb3
# cat /sys/class/net/eth1.4004/features
0x110803


2.6.30/tg3/8021q
# cat /sys/class/net/eth1/features
0x109a3
# cat /sys/class/net/eth1.4001/features
0x0



Or.

Re: [net-next 6/10] bnx2x: Update vlan_features

[Patrick McHardy]

Or Gerlitz wrote:
> Patrick McHardy wrote:
>> vlan_features doesn't need to be updated, the resulting dev->features
>> of the VLAN device is computed as the intersection of dev->features
>> and dev->vlan_features.
> 
> I'm not sure to follow, do you claim that the patches to bnx2x and bonding aren't needed to make vlans set on top of such devices to support these features?

In case of bnx2x, its enough to initialize dev->vlan_features once
to a static set and update only dev->features when appropriately.
vlan_features is meant to contain the hardware supported features
for VLANs, which are not necessarily active.

In case of bonding, its necessary to update vlan_features so it
contains the intersection of all underlying devices. But a
change will only take effect for existing VLANs (f.i. when
enslaving a new device) if you call netdev_features_change().

Re: [net-next 6/10] bnx2x: Update vlan_features

[Or Gerlitz]

okay, understood. So the patches to bnx2 and bnx2x can/should be made 
simpler and the patch to bonding has to change and include a call to 
netdev_features_change(), Jay, I assume you prefer to do that, if not, 
let me know and I will.

Or.

Re: [net-next 6/10] bnx2x: Update vlan_features

[Jay Vosburgh]

Or Gerlitz <ogerlitz@voltaire.com> wrote:

>okay, understood. So the patches to bnx2 and bnx2x can/should be made
>simpler and the patch to bonding has to change and include a call to
>netdev_features_change(), Jay, I assume you prefer to do that, if not, let
>me know and I will.

	I'm working on the new bonding patch; adding the
netdev_features_change call has locking implications, so the calls to
bond_compute_features have to move around a bit.

	Should have something later today.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [net-next 6/10] bnx2x: Update vlan_features

[Jay Vosburgh]

Patrick McHardy <kaber@trash.net> wrote:

>In case of bonding, its necessary to update vlan_features so it
>contains the intersection of all underlying devices. But a
>change will only take effect for existing VLANs (f.i. when
>enslaving a new device) if you call netdev_features_change().

	Patrick, can you clarify one bit about your above statment?  You
say the bonding features should be an "intersection"; is that a strict
intersection (i.e., slave1->vlan_features | slave2->vlan_features), or
does the NETIF_F_ONE_FOR_ALL logic apply for vlan_features as it does
for regular dev->features (using netdev_incrmenet_features() to combine
the feature sets)?

	In other words, if a bond has two slaves, one with, e.g.,
NETIF_F_SG in its vlan_features, and the other slave has 0 in
vlan_features, should the bond's vlan_features be NETIF_F_SG, or 0?

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [net-next 6/10] bnx2x: Update vlan_features

[Or Gerlitz]

Jay Vosburgh wrote:
> 	Patrick, can you clarify one bit about your above statment?  You
> say the bonding features should be an "intersection"; is that a strict
> intersection (i.e., slave1->vlan_features | slave2->vlan_features), or
> does the NETIF_F_ONE_FOR_ALL logic apply for vlan_features as it does
> for regular dev->features (using netdev_incrmenet_features() to combine
> the feature sets)?
>
> 	In other words, if a bond has two slaves, one with, e.g.,
> NETIF_F_SG in its vlan_features, and the other slave has 0 in
> vlan_features, should the bond's vlan_features be NETIF_F_SG, or 0?
>   
Hi Jay,

So we're @ rc7 again and the "bonding: propogate vlan_features to 
bonding master" patch isn't queued yet... can you look into that so we 
have this for 2.6.32?

thanks,

Or.