| Message ID | 20110331110135.19BE6138DD@rere.qmqm.pl |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
2011/3/31 Michał Mirosław <mirq-linux@rere.qmqm.pl>: > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index e34ea9e..b84a46b 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -704,8 +704,8 @@ static int vlan_dev_init(struct net_device *dev) > (1<<__LINK_STATE_DORMANT))) | > (1<<__LINK_STATE_PRESENT); > > - dev->features |= real_dev->features & real_dev->vlan_features; > - dev->features |= NETIF_F_LLTX; > + dev->hw_features = real_dev->vlan_features & NETIF_F_ALL_TX_OFFLOADS; > + dev->features |= real_dev->vlan_features | NETIF_F_LLTX; Shouldn't this continue to use real_dev->feaures & real_dev->vlan_features? In some places capabilities are blindly added to vlan_features on the assumption that they will later be ANDed with the real capabilities of the hardware (for example, see register_netdevice()). -- 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
On Fri, Apr 01, 2011 at 06:55:04PM -0700, Jesse Gross wrote: > 2011/3/31 Michał Mirosław <mirq-linux@rere.qmqm.pl>: > > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > > index e34ea9e..b84a46b 100644 > > --- a/net/8021q/vlan_dev.c > > +++ b/net/8021q/vlan_dev.c > > @@ -704,8 +704,8 @@ static int vlan_dev_init(struct net_device *dev) > > (1<<__LINK_STATE_DORMANT))) | > > (1<<__LINK_STATE_PRESENT); > > > > - dev->features |= real_dev->features & real_dev->vlan_features; > > - dev->features |= NETIF_F_LLTX; > > + dev->hw_features = real_dev->vlan_features & NETIF_F_ALL_TX_OFFLOADS; > > + dev->features |= real_dev->vlan_features | NETIF_F_LLTX; > Shouldn't this continue to use real_dev->feaures & > real_dev->vlan_features? In some places capabilities are blindly > added to vlan_features on the assumption that they will later be ANDed > with the real capabilities of the hardware (for example, see > register_netdevice()). real_dev->features are ANDed on every netdev_update_features() (also called by register_netdevice()) by vlan_dev_fix_features(). This sets all vlan_features as wanted to be enabled, so that if some are initially disabled on the base device and later enabled then they can be propagated back to vlan devices. The other way would be to set wanted_features based on current real_dev->features & real_dev->vlan_features. Hmm. I just noticed that I missed vlan_transfer_features() in the conversion. I'll fix that. Best Regards, Michał Mirosław -- 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/net/8021q/vlan.c b/net/8021q/vlan.c index 7850412..4b575de 100644 --- a/net/8021q/vlan.c +++ b/net/8021q/vlan.c @@ -329,8 +329,7 @@ static void vlan_transfer_features(struct net_device *dev, { u32 old_features = vlandev->features; - vlandev->features &= ~dev->vlan_features; - vlandev->features |= dev->features & dev->vlan_features; + netdev_update_features(vlandev); vlandev->gso_max_size = dev->gso_max_size; if (dev->features & NETIF_F_HW_VLAN_TX) diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c index e34ea9e..b84a46b 100644 --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -704,8 +704,8 @@ static int vlan_dev_init(struct net_device *dev) (1<<__LINK_STATE_DORMANT))) | (1<<__LINK_STATE_PRESENT); - dev->features |= real_dev->features & real_dev->vlan_features; - dev->features |= NETIF_F_LLTX; + dev->hw_features = real_dev->vlan_features & NETIF_F_ALL_TX_OFFLOADS; + dev->features |= real_dev->vlan_features | NETIF_F_LLTX; dev->gso_max_size = real_dev->gso_max_size; /* ipv6 shared card related stuff */ @@ -759,6 +759,17 @@ static void vlan_dev_uninit(struct net_device *dev) } } +static u32 vlan_dev_fix_features(struct net_device *dev, u32 features) +{ + struct net_device *real_dev = vlan_dev_info(dev)->real_dev; + + features &= (real_dev->features | NETIF_F_LLTX); + if (dev_ethtool_get_rx_csum(real_dev)) + features |= NETIF_F_RXCSUM; + + return features; +} + static int vlan_ethtool_get_settings(struct net_device *dev, struct ethtool_cmd *cmd) { @@ -774,18 +785,6 @@ static void vlan_ethtool_get_drvinfo(struct net_device *dev, strcpy(info->fw_version, "N/A"); } -static u32 vlan_ethtool_get_rx_csum(struct net_device *dev) -{ - const struct vlan_dev_info *vlan = vlan_dev_info(dev); - return dev_ethtool_get_rx_csum(vlan->real_dev); -} - -static u32 vlan_ethtool_get_flags(struct net_device *dev) -{ - const struct vlan_dev_info *vlan = vlan_dev_info(dev); - return dev_ethtool_get_flags(vlan->real_dev); -} - static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats) { @@ -823,32 +822,10 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, st return stats; } -static int vlan_ethtool_set_tso(struct net_device *dev, u32 data) -{ - if (data) { - struct net_device *real_dev = vlan_dev_info(dev)->real_dev; - - /* Underlying device must support TSO for VLAN-tagged packets - * and must have TSO enabled now. - */ - if (!(real_dev->vlan_features & NETIF_F_TSO)) - return -EOPNOTSUPP; - if (!(real_dev->features & NETIF_F_TSO)) - return -EINVAL; - dev->features |= NETIF_F_TSO; - } else { - dev->features &= ~NETIF_F_TSO; - } - return 0; -} - static const struct ethtool_ops vlan_ethtool_ops = { .get_settings = vlan_ethtool_get_settings, .get_drvinfo = vlan_ethtool_get_drvinfo, .get_link = ethtool_op_get_link, - .get_rx_csum = vlan_ethtool_get_rx_csum, - .get_flags = vlan_ethtool_get_flags, - .set_tso = vlan_ethtool_set_tso, }; static const struct net_device_ops vlan_netdev_ops = { @@ -874,6 +851,7 @@ static const struct net_device_ops vlan_netdev_ops = { .ndo_fcoe_get_wwn = vlan_dev_fcoe_get_wwn, .ndo_fcoe_ddp_target = vlan_dev_fcoe_ddp_target, #endif + .ndo_fix_features = vlan_dev_fix_features, }; void vlan_setup(struct net_device *dev)
Note: get_flags was actually broken, because it should return the flags capped with vlan_features. This is now done implicitly by limiting netdev->hw_features. RX checksumming offload control is (and was) broken, as there was no way before to say whether it's done for tagged packets. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- net/8021q/vlan.c | 3 +-- net/8021q/vlan_dev.c | 50 ++++++++++++++------------------------------------ 2 files changed, 15 insertions(+), 38 deletions(-)