| Message ID | 1248191263.18195.49.camel@lb-tlvb-eilong |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-) -- 1.5.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html