[RFC PATCH] vlan: convert VLAN devices to use hw_features

[RFC PATCH] vlan: convert VLAN devices to use hw_features

[Michał Mirosław]

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(-)

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)
-- 
1.7.2.5

Re: [RFC PATCH] vlan: convert VLAN devices to use hw_features

[Jesse Gross]

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()).

Re: [RFC PATCH] vlan: convert VLAN devices to use hw_features

[Michał Mirosław]

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

[PATCH v2] vlan: convert VLAN devices to use ndo_fix_features()

[Michał Mirosław]

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     |    8 ++------
 net/8021q/vlan_dev.c |   50 ++++++++++++++------------------------------------
 2 files changed, 16 insertions(+), 42 deletions(-)

changes from v1:
 - folded netdev_features_change() in vlan_transfer_features()
   This now depends on patch titled:
	net: Call netdev_features_change() from netdev_update_features()

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 7850412..e47600b 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -327,10 +327,6 @@ static void vlan_sync_address(struct net_device *dev,
 static void vlan_transfer_features(struct net_device *dev,
 				   struct net_device *vlandev)
 {
-	u32 old_features = vlandev->features;
-
-	vlandev->features &= ~dev->vlan_features;
-	vlandev->features |= dev->features & dev->vlan_features;
 	vlandev->gso_max_size = dev->gso_max_size;
 
 	if (dev->features & NETIF_F_HW_VLAN_TX)
@@ -341,8 +337,8 @@ static void vlan_transfer_features(struct net_device *dev,
 #if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
 	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
 #endif
-	if (old_features != vlandev->features)
-		netdev_features_change(vlandev);
+
+	netdev_update_features(vlandev);
 }
 
 static void __vlan_device_event(struct net_device *dev, unsigned long event)
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)
-- 
1.7.2.5

Re: [PATCH v2] vlan: convert VLAN devices to use ndo_fix_features()

[David Miller]

From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Sat,  2 Apr 2011 16:41:41 +0200 (CEST)

> 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>

Applied.