[RFC PATCH 00/12] net: Unified offload configuration

[RFC PATCH 00/12] net: Unified offload configuration

[Michał Mirosław]

This is a second attempt at defining a unified offload-setting interface
for network drivers. The changes are influenced by comments on the original
proposal and deeper look into what drivers do.

The idea:

Introduce two new fields to struct net_device to hold toggable and
user-requested feature sets. Offload features can be in:

  hw_features
	features user can toggle (e.g., via ethtool)

  wanted_features
	features requested by user - his/her wish if you prefer
	(subset of .hw_features + software-only features)

  features
	features currently active; if some offloads depend
	on other conditions, the set in .features & .hw_features
	might be not equal to .wanted_features & .hw_features

To check and transfer offload settings from .wanted_features to
.features, there are two new hooks in struct net_device_ops:

  features = ndo_fix_features(netdev, features);
	modifies passed feature set according to device-specific
	conditions

  err = ndo_set_features(netdev, features);
	should reconfigure the hardware for a new feature set

Core code:

  netdev_update_features(netdev);
	should be called after feature conditions changed, i.e.
	after MTU change, or slave device reconfiguration

  features = netdev_get_wanted_features(netdev);
	convenience function: replaces bits from .hw_features 
	(plus software-only features) in .features with .wanted_features

Other things added:
  - a compatibility layer for old ethtool ops for drivers converted
    to new offload setting API
  - transitional calls to old ethtool_ops for not-yet-converted drivers
  - a new flag for RX checksumming to replace driver-private fields/flags
  - request software offloads (GSO and GRO) by default
  - example conversions of a few drivers

Things to do after:
  - convert all drivers
  - remove redundant offload state in drivers
  - remove old ethtool_ops hooks (set_tso, set_flags, set_hw_csum, etc.)

This is only build-tested on x86 allyesconfig, so probably not for general
use, yet.

Best Regards,
Michał Mirosław

Michał Mirosław (12):
  net: Move check of checksum features to netdev_fix_features()
  net: Introduce new feature setting ops
  net: ethtool: use ndo_fix_features for offload setting
  net: introduce NETIF_F_RXCSUM
  net: ethtool: use ndo_fix_features for ethtool_ops->set_flags
  bridge: convert br_features_recompute() to ndo_fix_features
  vlan: convert VLAN devices to use ndo_fix_features()
  jme: convert offload constraints to ndo_fix_features
  virtio_net: convert to ndo_fix_features
  Intel net drivers: convert to ndo_fix_features
  veth: convert to hw_features
  skge: convert to hw_features

 drivers/net/e1000/e1000_ethtool.c  |   69 -------
 drivers/net/e1000/e1000_main.c     |   31 +++-
 drivers/net/e1000e/ethtool.c       |   62 ------
 drivers/net/e1000e/netdev.c        |   31 +++-
 drivers/net/igb/igb_ethtool.c      |   67 -------
 drivers/net/igb/igb_main.c         |   34 +++-
 drivers/net/igbvf/ethtool.c        |   57 ------
 drivers/net/igbvf/netdev.c         |   26 ++-
 drivers/net/ixgb/ixgb.h            |    2 +
 drivers/net/ixgb/ixgb_ethtool.c    |   59 +------
 drivers/net/ixgb/ixgb_main.c       |   31 +++-
 drivers/net/ixgbe/ixgbe_ethtool.c  |   65 -------
 drivers/net/ixgbe/ixgbe_main.c     |   32 +++-
 drivers/net/ixgbevf/ethtool.c      |   46 -----
 drivers/net/ixgbevf/ixgbevf_main.c |   27 +++-
 drivers/net/jme.c                  |   79 ++------
 drivers/net/jme.h                  |    2 -
 drivers/net/skge.c                 |   53 +-----
 drivers/net/skge.h                 |    1 -
 drivers/net/veth.c                 |   45 +----
 drivers/net/virtio_net.c           |   46 ++---
 include/linux/ethtool.h            |   29 +++-
 include/linux/netdevice.h          |   34 ++++
 net/8021q/vlan.c                   |    3 +-
 net/8021q/vlan_dev.c               |   51 ++----
 net/bridge/br_device.c             |   57 +-----
 net/bridge/br_if.c                 |   17 +-
 net/bridge/br_notify.c             |    2 +-
 net/bridge/br_private.h            |    4 +-
 net/core/dev.c                     |   80 ++++++--
 net/core/ethtool.c                 |  364 +++++++++++++++++++-----------------
 31 files changed, 579 insertions(+), 927 deletions(-)

-- 
1.7.2.3

[RFC PATCH 06/12] bridge: convert br_features_recompute() to ndo_fix_features

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/bridge/br_device.c  |   57 +++++-----------------------------------------
 net/bridge/br_if.c      |   17 ++++++-------
 net/bridge/br_notify.c  |    2 +-
 net/bridge/br_private.h |    4 +-
 4 files changed, 18 insertions(+), 62 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 5564435..78193ad 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -78,7 +78,7 @@ static int br_dev_open(struct net_device *dev)
 {
 	struct net_bridge *br = netdev_priv(dev);
 
-	br_features_recompute(br);
+	netdev_update_features(dev);
 	netif_start_queue(dev);
 	br_stp_enable_bridge(br);
 	br_multicast_open(br);
@@ -173,48 +173,12 @@ static void br_getinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 	strcpy(info->bus_info, "N/A");
 }
 
-static int br_set_sg(struct net_device *dev, u32 data)
+static unsigned long br_fix_features(struct net_device *dev,
+	unsigned long features)
 {
 	struct net_bridge *br = netdev_priv(dev);
 
-	if (data)
-		br->feature_mask |= NETIF_F_SG;
-	else
-		br->feature_mask &= ~NETIF_F_SG;
-
-	br_features_recompute(br);
-	return 0;
-}
-
-static int br_set_tso(struct net_device *dev, u32 data)
-{
-	struct net_bridge *br = netdev_priv(dev);
-
-	if (data)
-		br->feature_mask |= NETIF_F_TSO;
-	else
-		br->feature_mask &= ~NETIF_F_TSO;
-
-	br_features_recompute(br);
-	return 0;
-}
-
-static int br_set_tx_csum(struct net_device *dev, u32 data)
-{
-	struct net_bridge *br = netdev_priv(dev);
-
-	if (data)
-		br->feature_mask |= NETIF_F_NO_CSUM;
-	else
-		br->feature_mask &= ~NETIF_F_ALL_CSUM;
-
-	br_features_recompute(br);
-	return 0;
-}
-
-static int br_set_flags(struct net_device *netdev, u32 data)
-{
-	return ethtool_op_set_flags(netdev, data, ETH_FLAG_TXVLAN);
+	return br_features_recompute(br, features);
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -300,16 +264,6 @@ void br_netpoll_disable(struct net_bridge_port *p)
 static const struct ethtool_ops br_ethtool_ops = {
 	.get_drvinfo    = br_getinfo,
 	.get_link	= ethtool_op_get_link,
-	.get_tx_csum	= ethtool_op_get_tx_csum,
-	.set_tx_csum 	= br_set_tx_csum,
-	.get_sg		= ethtool_op_get_sg,
-	.set_sg		= br_set_sg,
-	.get_tso	= ethtool_op_get_tso,
-	.set_tso	= br_set_tso,
-	.get_ufo	= ethtool_op_get_ufo,
-	.set_ufo	= ethtool_op_set_ufo,
-	.get_flags	= ethtool_op_get_flags,
-	.set_flags	= br_set_flags,
 };
 
 static const struct net_device_ops br_netdev_ops = {
@@ -326,6 +280,7 @@ static const struct net_device_ops br_netdev_ops = {
 	.ndo_netpoll_cleanup	 = br_netpoll_cleanup,
 	.ndo_poll_controller	 = br_poll_controller,
 #endif
+	.ndo_fix_features        = br_fix_features,
 };
 
 static void br_dev_free(struct net_device *dev)
@@ -350,4 +305,6 @@ void br_dev_setup(struct net_device *dev)
 	dev->features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
 			NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_LLTX |
 			NETIF_F_NETNS_LOCAL | NETIF_F_GSO | NETIF_F_HW_VLAN_TX;
+	dev->hw_features = NETIF_F_NO_CSUM | NETIF_F_SG | NETIF_F_GSO_MASK |
+			   NETIF_F_HW_VLAN_TX;
 }
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d9d1e2b..94dc9ca 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -203,7 +203,6 @@ static struct net_device *new_bridge_dev(struct net *net, const char *name)
 
 	memcpy(br->group_addr, br_group_address, ETH_ALEN);
 
-	br->feature_mask = dev->features;
 	br->stp_enabled = BR_NO_STP;
 	br->designated_root = br->bridge_id;
 	br->root_path_cost = 0;
@@ -362,15 +361,16 @@ int br_min_mtu(const struct net_bridge *br)
 /*
  * Recomputes features using slave's features
  */
-void br_features_recompute(struct net_bridge *br)
+unsigned long br_features_recompute(struct net_bridge *br,
+	unsigned long features)
 {
 	struct net_bridge_port *p;
-	unsigned long features, mask;
+	unsigned long mask;
 
-	features = mask = br->feature_mask;
 	if (list_empty(&br->port_list))
-		goto done;
+		return features;
 
+	mask = features;
 	features &= ~NETIF_F_ONE_FOR_ALL;
 
 	list_for_each_entry(p, &br->port_list, list) {
@@ -378,8 +378,7 @@ void br_features_recompute(struct net_bridge *br)
 						     p->dev->features, mask);
 	}
 
-done:
-	br->dev->features = netdev_fix_features(features, NULL);
+	return features;
 }
 
 /* called with RTNL */
@@ -441,7 +440,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	spin_lock_bh(&br->lock);
 	br_stp_recalculate_bridge_id(br);
-	br_features_recompute(br);
+	netdev_update_features(br->dev);
 
 	if ((dev->flags & IFF_UP) && netif_carrier_ok(dev) &&
 	    (br->dev->flags & IFF_UP))
@@ -483,7 +482,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 
 	spin_lock_bh(&br->lock);
 	br_stp_recalculate_bridge_id(br);
-	br_features_recompute(br);
+	netdev_update_features(br->dev);
 	spin_unlock_bh(&br->lock);
 
 	return 0;
diff --git a/net/bridge/br_notify.c b/net/bridge/br_notify.c
index 7d337c9..274ef4a 100644
--- a/net/bridge/br_notify.c
+++ b/net/bridge/br_notify.c
@@ -62,7 +62,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
 	case NETDEV_FEAT_CHANGE:
 		spin_lock_bh(&br->lock);
 		if (netif_running(br->dev))
-			br_features_recompute(br);
+			netdev_update_features(br->dev);
 		spin_unlock_bh(&br->lock);
 		break;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 84aac77..d4a8b4b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -182,7 +182,6 @@ struct net_bridge
 	struct br_cpu_netstats __percpu *stats;
 	spinlock_t			hash_lock;
 	struct hlist_head		hash[BR_HASH_SIZE];
-	unsigned long			feature_mask;
 #ifdef CONFIG_BRIDGE_NETFILTER
 	struct rtable 			fake_rtable;
 	bool				nf_call_iptables;
@@ -376,7 +375,8 @@ extern int br_add_if(struct net_bridge *br,
 extern int br_del_if(struct net_bridge *br,
 	      struct net_device *dev);
 extern int br_min_mtu(const struct net_bridge *br);
-extern void br_features_recompute(struct net_bridge *br);
+extern unsigned long br_features_recompute(struct net_bridge *br,
+	unsigned long new_features);
 
 /* br_input.c */
 extern int br_handle_frame_finish(struct sk_buff *skb);
-- 
1.7.2.3

[RFC PATCH 07/12] 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     |    3 +-
 net/8021q/vlan_dev.c |   51 ++++++++++++++-----------------------------------
 2 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 6e64f7c..583d47b 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -329,8 +329,7 @@ static void vlan_transfer_features(struct net_device *dev,
 {
 	unsigned long 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 be73753..468c899 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -691,8 +691,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;
+	dev->features |= real_dev->vlan_features | NETIF_F_LLTX;
 	dev->gso_max_size = real_dev->gso_max_size;
 
 	/* ipv6 shared card related stuff */
@@ -745,6 +745,18 @@ static void vlan_dev_uninit(struct net_device *dev)
 	}
 }
 
+static unsigned long vlan_dev_fix_features(struct net_device *dev,
+	unsigned long 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)
 {
@@ -760,18 +772,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)
 {
 
@@ -809,32 +809,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 = {
@@ -859,6 +837,7 @@ static const struct net_device_ops vlan_netdev_ops = {
 	.ndo_fcoe_disable	= vlan_dev_fcoe_disable,
 	.ndo_fcoe_get_wwn	= vlan_dev_fcoe_get_wwn,
 #endif
+	.ndo_fix_features	= vlan_dev_fix_features,
 };
 
 void vlan_setup(struct net_device *dev)
-- 
1.7.2.3

Re: [RFC PATCH 07/12] vlan: convert VLAN devices to use ndo_fix_features()

[Ben Hutchings]

On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> 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 |   51 ++++++++++++++-----------------------------------
>  2 files changed, 16 insertions(+), 38 deletions(-)
> 
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 6e64f7c..583d47b 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -329,8 +329,7 @@ static void vlan_transfer_features(struct net_device *dev,
>  {
>  	unsigned long 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 be73753..468c899 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -691,8 +691,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;
[...]

net_device::hw_features is supposed to represent features that can be
toggled, but the inclusion of a flag in net_device::vlan_features does
not mean the feature can be toggled.

If this is to be a straight conversion, that line should be:

	dev->hw_features = real_dev->vlan_features & NETIF_F_TSO;

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 07/12] vlan: convert VLAN devices to use ndo_fix_features()

[Michał Mirosław]

On Thu, Dec 16, 2010 at 11:36:58PM +0000, Ben Hutchings wrote:
> On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > 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 |   51 ++++++++++++++-----------------------------------
> >  2 files changed, 16 insertions(+), 38 deletions(-)
> > 
> > diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> > index 6e64f7c..583d47b 100644
> > --- a/net/8021q/vlan.c
> > +++ b/net/8021q/vlan.c
> > @@ -329,8 +329,7 @@ static void vlan_transfer_features(struct net_device *dev,
> >  {
> >  	unsigned long 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 be73753..468c899 100644
> > --- a/net/8021q/vlan_dev.c
> > +++ b/net/8021q/vlan_dev.c
> > @@ -691,8 +691,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;
> [...]
> net_device::hw_features is supposed to represent features that can be
> toggled, but the inclusion of a flag in net_device::vlan_features does
> not mean the feature can be toggled.

All TX offloads are by definition togglable since it just a matter of not
issuing skb's that need them. RX offloads on the other hand are controlled
by underlying device, so probably should be removed from this set and
forced on or left alone in ndo_fix_features().

> If this is to be a straight conversion, that line should be:
> 	dev->hw_features = real_dev->vlan_features & NETIF_F_TSO;

I'll mask this with ALL_CSUM+SG+ALL_TSO if you don't mind extending the
possibilities anyway.

Best Regards,
Michał Mirosław

[RFC PATCH 01/12] net: Move check of checksum features to netdev_fix_features()

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/dev.c |   32 +++++++++++++++++---------------
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f937726..6823275 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5020,6 +5020,23 @@ static void rollback_registered(struct net_device *dev)
 
 unsigned long netdev_fix_features(unsigned long features, const char *name)
 {
+	/* Fix illegal checksum combinations */
+	if ((features & NETIF_F_HW_CSUM) &&
+	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+		if (name)
+			printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
+				name);
+		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+	}
+
+	if ((features & NETIF_F_NO_CSUM) &&
+	    (features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
+		if (name)
+			printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
+				name);
+		features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
+	}
+
 	/* Fix illegal SG+CSUM combinations. */
 	if ((features & NETIF_F_SG) &&
 	    !(features & NETIF_F_ALL_CSUM)) {
@@ -5195,21 +5212,6 @@ int register_netdevice(struct net_device *dev)
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
-	/* Fix illegal checksum combinations */
-	if ((dev->features & NETIF_F_HW_CSUM) &&
-	    (dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		printk(KERN_NOTICE "%s: mixed HW and IP checksum settings.\n",
-		       dev->name);
-		dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
-	}
-
-	if ((dev->features & NETIF_F_NO_CSUM) &&
-	    (dev->features & (NETIF_F_HW_CSUM|NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
-		printk(KERN_NOTICE "%s: mixed no checksumming and other settings.\n",
-		       dev->name);
-		dev->features &= ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM|NETIF_F_HW_CSUM);
-	}
-
 	dev->features = netdev_fix_features(dev->features, dev->name);
 
 	/* Enable software GSO if SG is supported. */
-- 
1.7.2.3

[RFC PATCH 02/12] net: Introduce new feature setting ops

[Michał Mirosław]

This introduces a new framework to handle device features setting.
It consists of:
  - new fields in struct net_device:
	+ hw_features - features that hw/driver supports toggling
	+ wanted_features - features that user wants enabled, when possible
  - new netdev_ops:
	+ feat = ndo_fix_features(dev, feat) - API checking constraints for
		enabling features or their combinations
	+ ndo_set_features(dev) - API updating hardware state to match
		changed dev->features
  - new ethtool commands:
	+ ETHTOOL_GFEATURES/ETHTOOL_SFEATURES: get/set dev->wanted_features
		and trigger device reconfiguration if resulting dev->features
		changed
	[TODO: this might be extended to support device-specific flags, and
	keep NETIF_F flags from becoming part of ABI by using GET_STRINGS
	for describing the bits]
	[Note: ETHTOOL_GFEATURES and ETHTOOL_SFEATURES' data is supposed to
	be 'compatible', so that you can R/M/W without additional copying]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/ethtool.h   |   28 ++++++++++++++++++++++
 include/linux/netdevice.h |   29 +++++++++++++++++++++++
 net/core/dev.c            |   40 ++++++++++++++++++++++++++++----
 net/core/ethtool.c        |   56 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 148 insertions(+), 5 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 1908929..0267d45 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -523,6 +523,31 @@ struct ethtool_flash {
 	char	data[ETHTOOL_FLASH_MAX_FILENAME];
 };
 
+/* for returning feature sets */
+#define ETHTOOL_DEV_FEATURE_WORDS	1
+
+struct ethtool_get_features_block {
+	__u32	available;	/* features togglable */
+	__u32	requested;	/* features requested to be enabled */
+	__u32	active;		/* features currently enabled */
+	__u32	__pad[1];
+};
+
+struct ethtool_set_features_block {
+	__u32	valid;		/* bits valid in .requested */
+	__u32	requested;	/* features requested */
+	__u32	__pad[2];
+};
+
+struct ethtool_features {
+	__u32	cmd;
+	__u32	count;		/* blocks */
+	union {
+		struct ethtool_get_features_block get;
+		struct ethtool_set_features_block set;
+	} features[0];
+};
+
 #ifdef __KERNEL__
 
 #include <linux/rculist.h>
@@ -744,6 +769,9 @@ struct ethtool_ops {
 #define ETHTOOL_GRXFHINDIR	0x00000038 /* Get RX flow hash indir'n table */
 #define ETHTOOL_SRXFHINDIR	0x00000039 /* Set RX flow hash indir'n table */
 
+#define ETHTOOL_GFEATURES	0x0000003a /* Get device offload settings */
+#define ETHTOOL_SFEATURES	0x0000003b /* Change device offload settings */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d31bc3c..4b20944 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -756,6 +756,17 @@ struct xps_dev_maps {
  * int (*ndo_set_vf_port)(struct net_device *dev, int vf,
  *			  struct nlattr *port[]);
  * int (*ndo_get_vf_port)(struct net_device *dev, int vf, struct sk_buff *skb);
+ *
+ * unsigned long (*ndo_fix_features)(struct net_device *dev,
+ *				     unsigned long features);
+ *	Modifies features supported by device depending on device-specific
+ *	constraints. Should not modify hardware state.
+ *
+ * int (*ndo_set_features)(struct net_device *dev, unsigned long features);
+ *	Called to update hardware configuration to new features. Selected
+ *	features might be less than what was returned by ndo_fix_features()).
+ *	Must return >0 if it changed dev->features by itself.
+ *
  */
 #define HAVE_NET_DEVICE_OPS
 struct net_device_ops {
@@ -828,6 +839,10 @@ struct net_device_ops {
 	int			(*ndo_fcoe_get_wwn)(struct net_device *dev,
 						    u64 *wwn, int type);
 #endif
+	unsigned long		(*ndo_fix_features)(struct net_device *dev,
+						    unsigned long features);
+	int			(*ndo_set_features)(struct net_device *dev,
+						    unsigned long features);
 };
 
 /*
@@ -934,6 +949,14 @@ struct net_device {
 				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
 				 NETIF_F_FRAGLIST)
 
+	/* toggable features with no driver requirements */
+#define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
+
+	/* ethtool-toggable features */
+	unsigned long		hw_features;
+	/* ethtool-requested features */
+	unsigned long		wanted_features;
+
 	/* Interface index. Unique device identifier	*/
 	int			ifindex;
 	int			iflink;
@@ -2286,9 +2309,15 @@ extern char *netdev_drivername(const struct net_device *dev, char *buffer, int l
 
 extern void linkwatch_run_queue(void);
 
+static inline unsigned long netdev_get_wanted_features(struct net_device *dev)
+{
+	unsigned long toggable = dev->hw_features | NETIF_F_SOFT_FEATURES;
+	return (dev->features & ~toggable) | dev->wanted_features;
+}
 unsigned long netdev_increment_features(unsigned long all, unsigned long one,
 					unsigned long mask);
 unsigned long netdev_fix_features(unsigned long features, const char *name);
+void netdev_update_features(struct net_device *dev);
 
 void netif_stacked_transfer_operstate(const struct net_device *rootdev,
 					struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 6823275..1e616bb 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5078,6 +5078,35 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
 }
 EXPORT_SYMBOL(netdev_fix_features);
 
+void netdev_update_features(struct net_device *dev)
+{
+	unsigned long enable;
+	int err = 0;
+
+	enable = netdev_get_wanted_features(dev);
+
+	if (dev->netdev_ops->ndo_fix_features)
+		enable = dev->netdev_ops->ndo_fix_features(dev, enable);
+
+	/* driver might be less strict about feature dependencies */
+	enable = netdev_fix_features(enable, dev->name);
+
+	if (dev->features == enable)
+		return;
+
+	netdev_info(dev, "Features changed: %08lx -> %08lx\n",
+		dev->features, enable);
+
+	if (dev->netdev_ops->ndo_set_features)
+		err = dev->netdev_ops->ndo_set_features(dev, enable);
+
+	if (!err)
+		dev->features = enable;
+	else if (err < 0)
+		netdev_err(dev, "set_features() failed (%d)\n", err);
+}
+EXPORT_SYMBOL(netdev_update_features);
+
 /**
  *	netif_stacked_transfer_operstate -	transfer operstate
  *	@rootdev: the root or lower level device to transfer state from
@@ -5212,11 +5241,12 @@ int register_netdevice(struct net_device *dev)
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
-	dev->features = netdev_fix_features(dev->features, dev->name);
-
-	/* Enable software GSO if SG is supported. */
-	if (dev->features & NETIF_F_SG)
-		dev->features |= NETIF_F_GSO;
+	/* Transfer toggable features to wanted_features and enable
+	 * software GSO and GRO as these need no driver support.
+	 */
+	dev->wanted_features = (dev->features & dev->hw_features)
+		| NETIF_F_SOFT_FEATURES;
+	netdev_update_features(dev);
 
 	/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
 	 * vlan_dev_init() will do the dev->features check, so these features
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1774178..f08e7f1 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -171,6 +171,55 @@ EXPORT_SYMBOL(ethtool_ntuple_flush);
 
 /* Handlers for each ethtool command */
 
+static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_features cmd = {
+		.cmd = ETHTOOL_GFEATURES,
+		.count = ETHTOOL_DEV_FEATURE_WORDS,
+	};
+	struct ethtool_get_features_block features[ETHTOOL_DEV_FEATURE_WORDS] = {
+		{
+			.available = dev->hw_features,
+			.requested = dev->wanted_features,
+			.active = dev->features,
+		},
+	};
+
+	if (copy_to_user(useraddr, &cmd, sizeof(cmd)))
+		return -EFAULT;
+	useraddr += sizeof(cmd);
+	if (copy_to_user(useraddr, features, sizeof(features)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_features cmd;
+	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
+
+	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
+		return -EFAULT;
+	useraddr += sizeof(cmd);
+
+	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
+		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
+
+	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
+		return -EFAULT;
+	memset(features + cmd.count, 0,
+		sizeof(features) - sizeof(*features) * cmd.count);
+
+	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
+
+	dev->wanted_features &= ~features[0].valid;
+	dev->wanted_features |= features[0].valid & features[0].requested;
+
+	netdev_update_features(dev);
+
+	return 0;
+}
+
 static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_cmd cmd = { .cmd = ETHTOOL_GSET };
@@ -1500,6 +1549,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GRXCLSRLCNT:
 	case ETHTOOL_GRXCLSRULE:
 	case ETHTOOL_GRXCLSRLALL:
+	case ETHTOOL_GFEATURES:
 		break;
 	default:
 		if (!capable(CAP_NET_ADMIN))
@@ -1693,6 +1743,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SRXFHINDIR:
 		rc = ethtool_set_rxfh_indir(dev, useraddr);
 		break;
+	case ETHTOOL_GFEATURES:
+		rc = ethtool_get_features(dev, useraddr);
+		break;
+	case ETHTOOL_SFEATURES:
+		rc = ethtool_set_features(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.2.3

Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

[Ben Hutchings]

On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> This introduces a new framework to handle device features setting.
> It consists of:
>   - new fields in struct net_device:
> 	+ hw_features - features that hw/driver supports toggling
> 	+ wanted_features - features that user wants enabled, when possible
>   - new netdev_ops:
> 	+ feat = ndo_fix_features(dev, feat) - API checking constraints for
> 		enabling features or their combinations
> 	+ ndo_set_features(dev) - API updating hardware state to match
> 		changed dev->features
>   - new ethtool commands:
> 	+ ETHTOOL_GFEATURES/ETHTOOL_SFEATURES: get/set dev->wanted_features
> 		and trigger device reconfiguration if resulting dev->features
> 		changed
> 	[TODO: this might be extended to support device-specific flags, and
> 	keep NETIF_F flags from becoming part of ABI by using GET_STRINGS
> 	for describing the bits]

We already have ETHTOOL_{G,S}PFLAGS for that, though.

> 	[Note: ETHTOOL_GFEATURES and ETHTOOL_SFEATURES' data is supposed to
> 	be 'compatible', so that you can R/M/W without additional copying]

But if you expect userland to do that, what is the point of the 'valid'
mask?  Shouldn't userland do something like:

	struct ethtool_features feat = { ETHTOOL_SFEATURES };
	...
	if (off_tso_wanted >= 0)
		feat.features[0].valid |= NETIF_F_TSO;
	if (off_tso_wanted > 0)
		feat.features[0].requested |= NETIF_F_TSO;
	...

[...]
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -523,6 +523,31 @@ struct ethtool_flash {
>         char    data[ETHTOOL_FLASH_MAX_FILENAME];
>  };
>  
> +/* for returning feature sets */
> +#define ETHTOOL_DEV_FEATURE_WORDS      1
> +
> +struct ethtool_get_features_block {
> +       __u32   available;      /* features togglable */
> +       __u32   requested;      /* features requested to be enabled */
> +       __u32   active;         /* features currently enabled */
> +       __u32   __pad[1];
> +};
> +
> +struct ethtool_set_features_block {
> +       __u32   valid;          /* bits valid in .requested */
> +       __u32   requested;      /* features requested */
> +       __u32   __pad[2];
> +};
> +
> +struct ethtool_features {
> +       __u32   cmd;
> +       __u32   count;          /* blocks */
> +       union {
> +               struct ethtool_get_features_block get;
> +               struct ethtool_set_features_block set;
> +       } features[0];
> +};

I want kernel-doc comments with a proper description of semantics.

> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
[...]
> @@ -934,6 +949,14 @@ struct net_device {
>  				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
>  				 NETIF_F_FRAGLIST)
>  
> +	/* toggable features with no driver requirements */
> +#define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
> +
> +	/* ethtool-toggable features */

The verb is 'toggle' so this adjective should be either 'togglable' or
'toggleable'.  Or you could choose a different adjective.

> +	unsigned long		hw_features;
> +	/* ethtool-requested features */
> +	unsigned long		wanted_features;
> +

While you're at it, you could change all these 'features' fields and
parameters to u32, since we presumably won't be defining features that
can only be enabled on 64-bit architectures.

[...]
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 1774178..f08e7f1 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -171,6 +171,55 @@ EXPORT_SYMBOL(ethtool_ntuple_flush);
>  
>  /* Handlers for each ethtool command */
>  
> +static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_features cmd = {
> +		.cmd = ETHTOOL_GFEATURES,
> +		.count = ETHTOOL_DEV_FEATURE_WORDS,
> +	};
> +	struct ethtool_get_features_block features[ETHTOOL_DEV_FEATURE_WORDS] = {
> +		{
> +			.available = dev->hw_features,
> +			.requested = dev->wanted_features,
> +			.active = dev->features,
> +		},
> +	};
> +
> +	if (copy_to_user(useraddr, &cmd, sizeof(cmd)))
> +		return -EFAULT;
> +	useraddr += sizeof(cmd);
> +	if (copy_to_user(useraddr, features, sizeof(features)))
> +		return -EFAULT;

If ETHTOOL_DEV_FEATURE_WORDS increases, how do you know the user buffer
will be big enough?

> +	return 0;
> +}
> +
> +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_features cmd;
> +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> +
> +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> +		return -EFAULT;
> +	useraddr += sizeof(cmd);
> +
> +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;

So additional feature words will be silently ignored...

> +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> +		return -EFAULT;
> +	memset(features + cmd.count, 0,
> +		sizeof(features) - sizeof(*features) * cmd.count);
> +
> +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
[...]

...as will any other unsupported features.  This is not a good idea.
(However, remembering which features are wanted does seem like a good
idea.)

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

[Michał Mirosław]

On Thu, Dec 16, 2010 at 11:13:06PM +0000, Ben Hutchings wrote:
> On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > This introduces a new framework to handle device features setting.
> > It consists of:
> >   - new fields in struct net_device:
> > 	+ hw_features - features that hw/driver supports toggling
> > 	+ wanted_features - features that user wants enabled, when possible
> >   - new netdev_ops:
> > 	+ feat = ndo_fix_features(dev, feat) - API checking constraints for
> > 		enabling features or their combinations
> > 	+ ndo_set_features(dev) - API updating hardware state to match
> > 		changed dev->features
> >   - new ethtool commands:
> > 	+ ETHTOOL_GFEATURES/ETHTOOL_SFEATURES: get/set dev->wanted_features
> > 		and trigger device reconfiguration if resulting dev->features
> > 		changed
> > 	[TODO: this might be extended to support device-specific flags, and
> > 	keep NETIF_F flags from becoming part of ABI by using GET_STRINGS
> > 	for describing the bits]
> We already have ETHTOOL_{G,S}PFLAGS for that, though.

Since we're almost out of bits in features, I thought that I might as well
just take care of that in one go. I'll implement the GET_STRINGS part to
show what I mean about bits not becoming an ABI.

> > 	[Note: ETHTOOL_GFEATURES and ETHTOOL_SFEATURES' data is supposed to
> > 	be 'compatible', so that you can R/M/W without additional copying]
> But if you expect userland to do that, what is the point of the 'valid'
> mask?  Shouldn't userland do something like:
> 
> 	struct ethtool_features feat = { ETHTOOL_SFEATURES };
> 	...
> 	if (off_tso_wanted >= 0)
> 		feat.features[0].valid |= NETIF_F_TSO;
> 	if (off_tso_wanted > 0)
> 		feat.features[0].requested |= NETIF_F_TSO;
> 	...

That looks even better than what I thought originally. So this means I can
just get rid of the combined ethtool cmd struct.

> [...]
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -523,6 +523,31 @@ struct ethtool_flash {
> >         char    data[ETHTOOL_FLASH_MAX_FILENAME];
> >  };
> >  
> > +/* for returning feature sets */
> > +#define ETHTOOL_DEV_FEATURE_WORDS      1
> > +
> > +struct ethtool_get_features_block {
> > +       __u32   available;      /* features togglable */
> > +       __u32   requested;      /* features requested to be enabled */
> > +       __u32   active;         /* features currently enabled */
> > +       __u32   __pad[1];
> > +};
> > +
> > +struct ethtool_set_features_block {
> > +       __u32   valid;          /* bits valid in .requested */
> > +       __u32   requested;      /* features requested */
> > +       __u32   __pad[2];
> > +};
> > +
> > +struct ethtool_features {
> > +       __u32   cmd;
> > +       __u32   count;          /* blocks */
> > +       union {
> > +               struct ethtool_get_features_block get;
> > +               struct ethtool_set_features_block set;
> > +       } features[0];
> > +};
> I want kernel-doc comments with a proper description of semantics.

Will do.

> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> [...]
> > @@ -934,6 +949,14 @@ struct net_device {
> >  				 NETIF_F_SG | NETIF_F_HIGHDMA |		\
> >  				 NETIF_F_FRAGLIST)
> >  
> > +	/* toggable features with no driver requirements */
> > +#define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
> > +
> > +	/* ethtool-toggable features */
> 
> The verb is 'toggle' so this adjective should be either 'togglable' or
> 'toggleable'.  Or you could choose a different adjective.

'changed'. Or fixed. ;-)

> > +	unsigned long		hw_features;
> > +	/* ethtool-requested features */
> > +	unsigned long		wanted_features;
> > +
> While you're at it, you could change all these 'features' fields and
> parameters to u32, since we presumably won't be defining features that
> can only be enabled on 64-bit architectures.

Done. I also went through functions that pass features along and fixed
their types. Patch will be in the next version of this series.

> [...]
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 1774178..f08e7f1 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -171,6 +171,55 @@ EXPORT_SYMBOL(ethtool_ntuple_flush);
> >  
> >  /* Handlers for each ethtool command */
> >  
> > +static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> > +{
> > +	struct ethtool_features cmd = {
> > +		.cmd = ETHTOOL_GFEATURES,
> > +		.count = ETHTOOL_DEV_FEATURE_WORDS,
> > +	};
> > +	struct ethtool_get_features_block features[ETHTOOL_DEV_FEATURE_WORDS] = {
> > +		{
> > +			.available = dev->hw_features,
> > +			.requested = dev->wanted_features,
> > +			.active = dev->features,
> > +		},
> > +	};
> > +
> > +	if (copy_to_user(useraddr, &cmd, sizeof(cmd)))
> > +		return -EFAULT;
> > +	useraddr += sizeof(cmd);
> > +	if (copy_to_user(useraddr, features, sizeof(features)))
> > +		return -EFAULT;
> If ETHTOOL_DEV_FEATURE_WORDS increases, how do you know the user buffer
> will be big enough?

I'll change that to use count from GET_STRINGS.

> > +	return 0;
> > +}
> > +
> > +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> > +{
> > +	struct ethtool_features cmd;
> > +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> > +
> > +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > +		return -EFAULT;
> > +	useraddr += sizeof(cmd);
> > +
> > +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> > +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
> So additional feature words will be silently ignored...
> > +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> > +		return -EFAULT;
> > +	memset(features + cmd.count, 0,
> > +		sizeof(features) - sizeof(*features) * cmd.count);
> > +
> > +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> [...]
> 
> ...as will any other unsupported features.  This is not a good idea.
> (However, remembering which features are wanted does seem like a good
> idea.)

That's intentional. Unsupported features can't be enabled anyway.
hw_features is supposed to contain all features that the device can support
and is able to enable/disable. This set should be constant and anything that
is in the wanted_features set but is not supported because of other conditions
will be stripped by ndo_fix_features() call.

Other way would be to return EINVAL when bits not changeable are present in
the valid mask. I don't want to do that, since then your example of changing
a feature without GFEATURES first will not work.

Best Regards,
Michał Mirosław
 

Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

[Ben Hutchings]

On Sun, 2010-12-19 at 01:49 +0100, Michał Mirosław wrote:
> On Thu, Dec 16, 2010 at 11:13:06PM +0000, Ben Hutchings wrote:
> > On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
[...]
> > > +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> > > +{
> > > +	struct ethtool_features cmd;
> > > +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> > > +
> > > +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > > +		return -EFAULT;
> > > +	useraddr += sizeof(cmd);
> > > +
> > > +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> > > +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
> > So additional feature words will be silently ignored...
> > > +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> > > +		return -EFAULT;
> > > +	memset(features + cmd.count, 0,
> > > +		sizeof(features) - sizeof(*features) * cmd.count);
> > > +
> > > +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > [...]
> > 
> > ...as will any other unsupported features.  This is not a good idea.
> > (However, remembering which features are wanted does seem like a good
> > idea.)
> 
> That's intentional. Unsupported features can't be enabled anyway.
> hw_features is supposed to contain all features that the device can support
> and is able to enable/disable. This set should be constant and anything that
> is in the wanted_features set but is not supported because of other conditions
> will be stripped by ndo_fix_features() call.
> 
> Other way would be to return EINVAL when bits not changeable are present in
> the valid mask. I don't want to do that, since then your example of changing
> a feature without GFEATURES first will not work.

That's right, it shouldn't work.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

[Michał Mirosław]

On Sun, Dec 19, 2010 at 09:22:39PM +0000, Ben Hutchings wrote:
> On Sun, 2010-12-19 at 01:49 +0100, Michał Mirosław wrote:
> > On Thu, Dec 16, 2010 at 11:13:06PM +0000, Ben Hutchings wrote:
> > > On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> [...]
> > > > +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> > > > +{
> > > > +	struct ethtool_features cmd;
> > > > +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> > > > +
> > > > +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > > > +		return -EFAULT;
> > > > +	useraddr += sizeof(cmd);
> > > > +
> > > > +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> > > > +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
> > > So additional feature words will be silently ignored...
> > > > +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> > > > +		return -EFAULT;
> > > > +	memset(features + cmd.count, 0,
> > > > +		sizeof(features) - sizeof(*features) * cmd.count);
> > > > +
> > > > +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > > [...]
> > > 
> > > ...as will any other unsupported features.  This is not a good idea.
> > > (However, remembering which features are wanted does seem like a good
> > > idea.)
> > 
> > That's intentional. Unsupported features can't be enabled anyway.
> > hw_features is supposed to contain all features that the device can support
> > and is able to enable/disable. This set should be constant and anything that
> > is in the wanted_features set but is not supported because of other conditions
> > will be stripped by ndo_fix_features() call.
> > 
> > Other way would be to return EINVAL when bits not changeable are present in
> > the valid mask. I don't want to do that, since then your example of changing
> > a feature without GFEATURES first will not work.
> That's right, it shouldn't work.

A user says "enable any TSO available". This means ethtool could issue
a request with .valid = NETIF_F_ALL_TSO, .requested = NETIF_F_ALL_TSO.
If the device supports only TSOv4 this should enable it and leave others
alone as whatever the user wants they can't be enabled.

This is 1-1 conversion of the semantics current ethtool ops have - set_tso
corresponds exactly to the request described above. This behaviour also
allows to execute a command like "enable as many as you can of ..." that
is usual goal of user enabling hardware offloads - to get best possible
performance.

Nevertheless, what problem is generated by ignoring unsupported bits here?
I can see the point of returning -EINVAL on bits that are not defined, though.
Is that a good direction?

Best Regards,
Michał Mirosław

Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

[Ben Hutchings]

On Mon, 2010-12-20 at 00:43 +0100, Michał Mirosław wrote:
> On Sun, Dec 19, 2010 at 09:22:39PM +0000, Ben Hutchings wrote:
> > On Sun, 2010-12-19 at 01:49 +0100, Michał Mirosław wrote:
> > > On Thu, Dec 16, 2010 at 11:13:06PM +0000, Ben Hutchings wrote:
> > > > On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > [...]
> > > > > +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> > > > > +{
> > > > > +	struct ethtool_features cmd;
> > > > > +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> > > > > +
> > > > > +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > > > > +		return -EFAULT;
> > > > > +	useraddr += sizeof(cmd);
> > > > > +
> > > > > +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> > > > > +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
> > > > So additional feature words will be silently ignored...
> > > > > +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> > > > > +		return -EFAULT;
> > > > > +	memset(features + cmd.count, 0,
> > > > > +		sizeof(features) - sizeof(*features) * cmd.count);
> > > > > +
> > > > > +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > > > [...]
> > > > 
> > > > ...as will any other unsupported features.  This is not a good idea.
> > > > (However, remembering which features are wanted does seem like a good
> > > > idea.)
> > > 
> > > That's intentional. Unsupported features can't be enabled anyway.
> > > hw_features is supposed to contain all features that the device can support
> > > and is able to enable/disable. This set should be constant and anything that
> > > is in the wanted_features set but is not supported because of other conditions
> > > will be stripped by ndo_fix_features() call.
> > > 
> > > Other way would be to return EINVAL when bits not changeable are present in
> > > the valid mask. I don't want to do that, since then your example of changing
> > > a feature without GFEATURES first will not work.
> > That's right, it shouldn't work.
> 
> A user says "enable any TSO available". This means ethtool could issue
> a request with .valid = NETIF_F_ALL_TSO, .requested = NETIF_F_ALL_TSO.
> If the device supports only TSOv4 this should enable it and leave others
> alone as whatever the user wants they can't be enabled.

I see your point, but...

> This is 1-1 conversion of the semantics current ethtool ops have - set_tso
> corresponds exactly to the request described above. This behaviour also
> allows to execute a command like "enable as many as you can of ..." that
> is usual goal of user enabling hardware offloads - to get best possible
> performance.

The current behaviour is that if TSO is not supported at all then any
attempt to control it fails with error EOPNOTSUPP.  Your proposed change
would make it return success.

So I think we have to expect ethtool (or other userland tool) to query
the supported feature flags if it is commanded to change some offload
feature that is represented by multiple feature flags in
net_device::features.  Alternately, we maintain a separate set of
feature flags for ethtool that doesn't make these distinctions.  But I
think it would be useful for ethtool to be able to query and report
exactly what features the device supports.

> Nevertheless, what problem is generated by ignoring unsupported bits here?

User confusion when an ethtool command 'succeeds' but has no effect.

> I can see the point of returning -EINVAL on bits that are not defined, though.
> Is that a good direction?

Any attempt to change undefined flags should definitely result in an
error.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[RFC PATCH 05/12] net: ethtool: use ndo_fix_features for ethtool_ops->set_flags

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/core/ethtool.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 15e00b8..2f532d5 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -215,6 +215,25 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
 	return 0;
 }
 
+static int __ethtool_set_flags(struct net_device *dev, u32 data)
+{
+	if (data & ~flags_dup_features)
+		return -EINVAL;
+
+	if (!(dev->hw_features & flags_dup_features)) {
+		if (!dev->ethtool_ops->set_flags)
+			return -EOPNOTSUPP;
+		return dev->ethtool_ops->set_flags(dev, data);
+	}
+
+	dev->wanted_features =
+		(dev->wanted_features & ~flags_dup_features) | data;
+
+	netdev_update_features(dev);
+
+	return 0;
+}
+
 static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
 {
 	switch (eth_cmd) {
@@ -1635,8 +1654,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 					ethtool_op_get_flags));
 		break;
 	case ETHTOOL_SFLAGS:
-		rc = ethtool_set_value(dev, useraddr,
-				       dev->ethtool_ops->set_flags);
+		rc = ethtool_set_value(dev, useraddr, __ethtool_set_flags);
 		break;
 	case ETHTOOL_GPFLAGS:
 		rc = ethtool_get_value(dev, useraddr, ethcmd,
-- 
1.7.2.3

[RFC PATCH 03/12] net: ethtool: use ndo_fix_features for offload setting

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/netdevice.h |    2 +
 net/core/dev.c            |    8 ++
 net/core/ethtool.c        |  252 +++++++++++++++++++-------------------------
 3 files changed, 119 insertions(+), 143 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4b20944..7634cab 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -941,6 +941,8 @@ struct net_device {
 #define NETIF_F_V6_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
 #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
 
+#define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
+
 	/*
 	 * If one device supports one of these features, then enable them
 	 * for all in netdev_increment_features.
diff --git a/net/core/dev.c b/net/core/dev.c
index 1e616bb..95d0a49 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5054,6 +5054,14 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
 		features &= ~NETIF_F_TSO;
 	}
 
+	/* Software GSO depends on SG. */
+	if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) {
+		if (name)
+			printk(KERN_NOTICE "%s: Dropping NETIF_F_GSO since no "
+			       "SG feature.\n", name);
+		features &= ~NETIF_F_GSO;
+	}
+
 	if (features & NETIF_F_UFO) {
 		/* maybe split UFO into V4 and V6? */
 		if (!((features & NETIF_F_GEN_CSUM) ||
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f08e7f1..b068738 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -55,6 +55,7 @@ int ethtool_op_set_tx_csum(struct net_device *dev, u32 data)
 
 	return 0;
 }
+EXPORT_SYMBOL(ethtool_op_set_tx_csum);
 
 int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data)
 {
@@ -220,6 +221,85 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
 	return 0;
 }
 
+static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
+{
+	switch (eth_cmd) {
+	case ETHTOOL_GTXCSUM:
+	case ETHTOOL_STXCSUM:
+		return NETIF_F_ALL_CSUM|NETIF_F_SCTP_CSUM;
+	case ETHTOOL_GSG:
+	case ETHTOOL_SSG:
+		return NETIF_F_SG;
+	case ETHTOOL_GTSO:
+	case ETHTOOL_STSO:
+		return NETIF_F_ALL_TSO;
+	case ETHTOOL_GUFO:
+	case ETHTOOL_SUFO:
+		return NETIF_F_UFO;
+	case ETHTOOL_GGSO:
+	case ETHTOOL_SGSO:
+		return NETIF_F_GSO;
+	case ETHTOOL_GGRO:
+	case ETHTOOL_SGRO:
+		return NETIF_F_GRO;
+	default:
+		BUG();
+	}
+}
+
+static int ethtool_get_one_feature(struct net_device *dev, char __user *useraddr,
+	u32 ethcmd)
+{
+	struct ethtool_value edata = {
+		.cmd = ethcmd,
+		.data = !!(dev->features & ethtool_get_feature_mask(ethcmd)),
+	};
+
+	if (copy_to_user(useraddr, &edata, sizeof(edata)))
+		return -EFAULT;
+	return 0;
+}
+
+static int __ethtool_set_tx_csum(struct net_device *dev, u32 data);
+static int __ethtool_set_sg(struct net_device *dev, u32 data);
+static int __ethtool_set_tso(struct net_device *dev, u32 data);
+static int __ethtool_set_ufo(struct net_device *dev, u32 data);
+
+static int ethtool_set_one_feature(struct net_device *dev,
+	void __user *useraddr, u32 ethcmd)
+{
+	struct ethtool_value edata;
+	unsigned long mask;
+
+	if (copy_from_user(&edata, useraddr, sizeof(edata)))
+		return -EFAULT;
+
+	mask = ethtool_get_feature_mask(ethcmd);
+	mask &= dev->hw_features | NETIF_F_SOFT_FEATURES;
+	if (mask) {
+		if (edata.data)
+			dev->wanted_features |= mask;
+		else
+			dev->wanted_features &= ~mask;
+
+		netdev_update_features(dev);
+		return 0;
+	}
+
+	switch (ethcmd) {
+	case ETHTOOL_STXCSUM:
+		return __ethtool_set_tx_csum(dev, edata.data);
+	case ETHTOOL_SSG:
+		return __ethtool_set_sg(dev, edata.data);
+	case ETHTOOL_STSO:
+		return __ethtool_set_tso(dev, edata.data);
+	case ETHTOOL_SUFO:
+		return __ethtool_set_ufo(dev, edata.data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_cmd cmd = { .cmd = ETHTOOL_GSET };
@@ -1140,6 +1220,9 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
 {
 	int err;
 
+	if (data && !(dev->features & NETIF_F_ALL_CSUM))
+		return -EINVAL;
+
 	if (!data && dev->ethtool_ops->set_tso) {
 		err = dev->ethtool_ops->set_tso(dev, 0);
 		if (err)
@@ -1154,26 +1237,21 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
 	return dev->ethtool_ops->set_sg(dev, data);
 }
 
-static int ethtool_set_tx_csum(struct net_device *dev, char __user *useraddr)
+static int __ethtool_set_tx_csum(struct net_device *dev, u32 data)
 {
-	struct ethtool_value edata;
 	int err;
 
 	if (!dev->ethtool_ops->set_tx_csum)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	if (!edata.data && dev->ethtool_ops->set_sg) {
+	if (!data && dev->ethtool_ops->set_sg) {
 		err = __ethtool_set_sg(dev, 0);
 		if (err)
 			return err;
 	}
 
-	return dev->ethtool_ops->set_tx_csum(dev, edata.data);
+	return dev->ethtool_ops->set_tx_csum(dev, data);
 }
-EXPORT_SYMBOL(ethtool_op_set_tx_csum);
 
 static int ethtool_set_rx_csum(struct net_device *dev, char __user *useraddr)
 {
@@ -1191,108 +1269,28 @@ static int ethtool_set_rx_csum(struct net_device *dev, char __user *useraddr)
 	return dev->ethtool_ops->set_rx_csum(dev, edata.data);
 }
 
-static int ethtool_set_sg(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata;
-
-	if (!dev->ethtool_ops->set_sg)
-		return -EOPNOTSUPP;
-
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	if (edata.data &&
-	    !(dev->features & NETIF_F_ALL_CSUM))
-		return -EINVAL;
-
-	return __ethtool_set_sg(dev, edata.data);
-}
-
-static int ethtool_set_tso(struct net_device *dev, char __user *useraddr)
+static int __ethtool_set_tso(struct net_device *dev, u32 data)
 {
-	struct ethtool_value edata;
-
 	if (!dev->ethtool_ops->set_tso)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	if (edata.data && !(dev->features & NETIF_F_SG))
+	if (data && !(dev->features & NETIF_F_SG))
 		return -EINVAL;
 
-	return dev->ethtool_ops->set_tso(dev, edata.data);
+	return dev->ethtool_ops->set_tso(dev, data);
 }
 
-static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
+static int __ethtool_set_ufo(struct net_device *dev, u32 data)
 {
-	struct ethtool_value edata;
-
 	if (!dev->ethtool_ops->set_ufo)
 		return -EOPNOTSUPP;
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-	if (edata.data && !(dev->features & NETIF_F_SG))
+	if (data && !(dev->features & NETIF_F_SG))
 		return -EINVAL;
-	if (edata.data && !((dev->features & NETIF_F_GEN_CSUM) ||
+	if (data && !((dev->features & NETIF_F_GEN_CSUM) ||
 		(dev->features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
 			== (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM)))
 		return -EINVAL;
-	return dev->ethtool_ops->set_ufo(dev, edata.data);
-}
-
-static int ethtool_get_gso(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GGSO };
-
-	edata.data = dev->features & NETIF_F_GSO;
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
-static int ethtool_set_gso(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata;
-
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-	if (edata.data)
-		dev->features |= NETIF_F_GSO;
-	else
-		dev->features &= ~NETIF_F_GSO;
-	return 0;
-}
-
-static int ethtool_get_gro(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GGRO };
-
-	edata.data = dev->features & NETIF_F_GRO;
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
-static int ethtool_set_gro(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata;
-
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	if (edata.data) {
-		u32 rxcsum = dev->ethtool_ops->get_rx_csum ?
-				dev->ethtool_ops->get_rx_csum(dev) :
-				ethtool_op_get_rx_csum(dev);
-
-		if (!rxcsum)
-			return -EINVAL;
-		dev->features |= NETIF_F_GRO;
-	} else
-		dev->features &= ~NETIF_F_GRO;
-
-	return 0;
+	return dev->ethtool_ops->set_ufo(dev, data);
 }
 
 static int ethtool_self_test(struct net_device *dev, char __user *useraddr)
@@ -1629,33 +1627,6 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SRXCSUM:
 		rc = ethtool_set_rx_csum(dev, useraddr);
 		break;
-	case ETHTOOL_GTXCSUM:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       (dev->ethtool_ops->get_tx_csum ?
-					dev->ethtool_ops->get_tx_csum :
-					ethtool_op_get_tx_csum));
-		break;
-	case ETHTOOL_STXCSUM:
-		rc = ethtool_set_tx_csum(dev, useraddr);
-		break;
-	case ETHTOOL_GSG:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       (dev->ethtool_ops->get_sg ?
-					dev->ethtool_ops->get_sg :
-					ethtool_op_get_sg));
-		break;
-	case ETHTOOL_SSG:
-		rc = ethtool_set_sg(dev, useraddr);
-		break;
-	case ETHTOOL_GTSO:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       (dev->ethtool_ops->get_tso ?
-					dev->ethtool_ops->get_tso :
-					ethtool_op_get_tso));
-		break;
-	case ETHTOOL_STSO:
-		rc = ethtool_set_tso(dev, useraddr);
-		break;
 	case ETHTOOL_TEST:
 		rc = ethtool_self_test(dev, useraddr);
 		break;
@@ -1671,21 +1642,6 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GPERMADDR:
 		rc = ethtool_get_perm_addr(dev, useraddr);
 		break;
-	case ETHTOOL_GUFO:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       (dev->ethtool_ops->get_ufo ?
-					dev->ethtool_ops->get_ufo :
-					ethtool_op_get_ufo));
-		break;
-	case ETHTOOL_SUFO:
-		rc = ethtool_set_ufo(dev, useraddr);
-		break;
-	case ETHTOOL_GGSO:
-		rc = ethtool_get_gso(dev, useraddr);
-		break;
-	case ETHTOOL_SGSO:
-		rc = ethtool_set_gso(dev, useraddr);
-		break;
 	case ETHTOOL_GFLAGS:
 		rc = ethtool_get_value(dev, useraddr, ethcmd,
 				       (dev->ethtool_ops->get_flags ?
@@ -1716,12 +1672,6 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SRXCLSRLINS:
 		rc = ethtool_set_rxnfc(dev, ethcmd, useraddr);
 		break;
-	case ETHTOOL_GGRO:
-		rc = ethtool_get_gro(dev, useraddr);
-		break;
-	case ETHTOOL_SGRO:
-		rc = ethtool_set_gro(dev, useraddr);
-		break;
 	case ETHTOOL_FLASHDEV:
 		rc = ethtool_flash_device(dev, useraddr);
 		break;
@@ -1749,6 +1699,22 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SFEATURES:
 		rc = ethtool_set_features(dev, useraddr);
 		break;
+	case ETHTOOL_GTXCSUM:
+	case ETHTOOL_GSG:
+	case ETHTOOL_GTSO:
+	case ETHTOOL_GUFO:
+	case ETHTOOL_GGSO:
+	case ETHTOOL_GGRO:
+		rc = ethtool_get_one_feature(dev, useraddr, ethcmd);
+		break;
+	case ETHTOOL_STXCSUM:
+	case ETHTOOL_SSG:
+	case ETHTOOL_STSO:
+	case ETHTOOL_SUFO:
+	case ETHTOOL_SGSO:
+	case ETHTOOL_SGRO:
+		rc = ethtool_set_one_feature(dev, useraddr, ethcmd);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.2.3

Re: [RFC PATCH 03/12] net: ethtool: use ndo_fix_features for offload setting

[Ben Hutchings]

On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  include/linux/netdevice.h |    2 +
>  net/core/dev.c            |    8 ++
>  net/core/ethtool.c        |  252 +++++++++++++++++++-------------------------
>  3 files changed, 119 insertions(+), 143 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 4b20944..7634cab 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -941,6 +941,8 @@ struct net_device {
>  #define NETIF_F_V6_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
>  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
>  
> +#define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> +
>  	/*
>  	 * If one device supports one of these features, then enable them
>  	 * for all in netdev_increment_features.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1e616bb..95d0a49 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5054,6 +5054,14 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
>  		features &= ~NETIF_F_TSO;
>  	}
>  
> +	/* Software GSO depends on SG. */
> +	if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) {
> +		if (name)
> +			printk(KERN_NOTICE "%s: Dropping NETIF_F_GSO since no "
> +			       "SG feature.\n", name);
> +		features &= ~NETIF_F_GSO;
> +	}
> +
>  	if (features & NETIF_F_UFO) {
>  		/* maybe split UFO into V4 and V6? */
>  		if (!((features & NETIF_F_GEN_CSUM) ||

The severity of these messages will need to be reduced if ethtool relies
on this function to propagate feature changes.  (And I wonder why some
of them are ERR and some NOTICE.)

> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f08e7f1..b068738 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -55,6 +55,7 @@ int ethtool_op_set_tx_csum(struct net_device *dev, u32 data)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(ethtool_op_set_tx_csum);
>  
>  int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data)
>  {
> @@ -220,6 +221,85 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
>  	return 0;
>  }
>  
> +static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
> +{
> +	switch (eth_cmd) {
> +	case ETHTOOL_GTXCSUM:
> +	case ETHTOOL_STXCSUM:
> +		return NETIF_F_ALL_CSUM|NETIF_F_SCTP_CSUM;

I wonder whether this should cover NETIF_F_FCOE_CRC as well (ixgbe
currently doesn't seem to allow toggling it).

There should be spaces around the '|' operator.

> +static int __ethtool_set_tx_csum(struct net_device *dev, u32 data);
> +static int __ethtool_set_sg(struct net_device *dev, u32 data);
> +static int __ethtool_set_tso(struct net_device *dev, u32 data);
> +static int __ethtool_set_ufo(struct net_device *dev, u32 data);
> +
> +static int ethtool_set_one_feature(struct net_device *dev,
> +	void __user *useraddr, u32 ethcmd)
> +{
> +	struct ethtool_value edata;
> +	unsigned long mask;
> +
> +	if (copy_from_user(&edata, useraddr, sizeof(edata)))
> +		return -EFAULT;
> +
> +	mask = ethtool_get_feature_mask(ethcmd);
> +	mask &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> +	if (mask) {
> +		if (edata.data)
> +			dev->wanted_features |= mask;
> +		else
> +			dev->wanted_features &= ~mask;
> +
> +		netdev_update_features(dev);
> +		return 0;
> +	}
> +
> +	switch (ethcmd) {
> +	case ETHTOOL_STXCSUM:
> +		return __ethtool_set_tx_csum(dev, edata.data);
> +	case ETHTOOL_SSG:
> +		return __ethtool_set_sg(dev, edata.data);
> +	case ETHTOOL_STSO:
> +		return __ethtool_set_tso(dev, edata.data);
> +	case ETHTOOL_SUFO:
> +		return __ethtool_set_ufo(dev, edata.data);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
[...]

This deserves some comments.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 03/12] net: ethtool: use ndo_fix_features for offload setting

[Michał Mirosław]

On Thu, Dec 16, 2010 at 11:23:49PM +0000, Ben Hutchings wrote:
> On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >  include/linux/netdevice.h |    2 +
> >  net/core/dev.c            |    8 ++
> >  net/core/ethtool.c        |  252 +++++++++++++++++++-------------------------
> >  3 files changed, 119 insertions(+), 143 deletions(-)
> > 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 4b20944..7634cab 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -941,6 +941,8 @@ struct net_device {
> >  #define NETIF_F_V6_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
> >  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
> >  
> > +#define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> > +
> >  	/*
> >  	 * If one device supports one of these features, then enable them
> >  	 * for all in netdev_increment_features.
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 1e616bb..95d0a49 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -5054,6 +5054,14 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
> >  		features &= ~NETIF_F_TSO;
> >  	}
> >  
> > +	/* Software GSO depends on SG. */
> > +	if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) {
> > +		if (name)
> > +			printk(KERN_NOTICE "%s: Dropping NETIF_F_GSO since no "
> > +			       "SG feature.\n", name);
> > +		features &= ~NETIF_F_GSO;
> > +	}
> > +
> >  	if (features & NETIF_F_UFO) {
> >  		/* maybe split UFO into V4 and V6? */
> >  		if (!((features & NETIF_F_GEN_CSUM) ||
> The severity of these messages will need to be reduced if ethtool relies
> on this function to propagate feature changes.  (And I wonder why some
> of them are ERR and some NOTICE.)

Patch is ready (all converted to netdev_info).

> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index f08e7f1..b068738 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -55,6 +55,7 @@ int ethtool_op_set_tx_csum(struct net_device *dev, u32 data)
> >  
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL(ethtool_op_set_tx_csum);
> >  
> >  int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data)
> >  {
> > @@ -220,6 +221,85 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> >  	return 0;
> >  }
> >  
> > +static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
> > +{
> > +	switch (eth_cmd) {
> > +	case ETHTOOL_GTXCSUM:
> > +	case ETHTOOL_STXCSUM:
> > +		return NETIF_F_ALL_CSUM|NETIF_F_SCTP_CSUM;
> I wonder whether this should cover NETIF_F_FCOE_CRC as well (ixgbe
> currently doesn't seem to allow toggling it).

This mask is only for 'legacy' set_tx_csum ethtool commands. Other
checksumming options - or whatever bits are included in hw_features - can be
toggled by ETHTOOL_SFEATURES.

> There should be spaces around the '|' operator.

Fixed.

> > +static int __ethtool_set_tx_csum(struct net_device *dev, u32 data);
> > +static int __ethtool_set_sg(struct net_device *dev, u32 data);
> > +static int __ethtool_set_tso(struct net_device *dev, u32 data);
> > +static int __ethtool_set_ufo(struct net_device *dev, u32 data);
> > +
> > +static int ethtool_set_one_feature(struct net_device *dev,
> > +	void __user *useraddr, u32 ethcmd)
> > +{
> > +	struct ethtool_value edata;
> > +	unsigned long mask;
> > +
> > +	if (copy_from_user(&edata, useraddr, sizeof(edata)))
> > +		return -EFAULT;
> > +
> > +	mask = ethtool_get_feature_mask(ethcmd);
> > +	mask &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > +	if (mask) {
> > +		if (edata.data)
> > +			dev->wanted_features |= mask;
> > +		else
> > +			dev->wanted_features &= ~mask;
> > +
> > +		netdev_update_features(dev);
> > +		return 0;
> > +	}
> > +
> > +	switch (ethcmd) {
> > +	case ETHTOOL_STXCSUM:
> > +		return __ethtool_set_tx_csum(dev, edata.data);
> > +	case ETHTOOL_SSG:
> > +		return __ethtool_set_sg(dev, edata.data);
> > +	case ETHTOOL_STSO:
> > +		return __ethtool_set_tso(dev, edata.data);
> > +	case ETHTOOL_SUFO:
> > +		return __ethtool_set_ufo(dev, edata.data);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> [...]
> 
> This deserves some comments.

Sure.

This is compatibility layer for current ethtool tools, and drivers
not converted to ndo_fix_features() and friends.

Best Regards,
Michał Mirosław

[RFC PATCH 04/12] net: introduce NETIF_F_RXCSUM

[Michał Mirosław]

Introduce NETIF_F_RXCSUM to replace device-private flags for RX checksum
offload. Integrate it with ndo_fix_features.

ethtool_op_get_rx_csum() is removed altogether as nothing in-tree uses it.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 include/linux/ethtool.h   |    1 -
 include/linux/netdevice.h |    3 +++
 net/core/ethtool.c        |   34 +++++++++++-----------------------
 3 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 0267d45..2475b41 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -568,7 +568,6 @@ struct net_device;
 
 /* Some generic methods drivers may use in their ethtool_ops */
 u32 ethtool_op_get_link(struct net_device *dev);
-u32 ethtool_op_get_rx_csum(struct net_device *dev);
 u32 ethtool_op_get_tx_csum(struct net_device *dev);
 int ethtool_op_set_tx_csum(struct net_device *dev, u32 data);
 int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7634cab..ce35514 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -920,6 +920,7 @@ struct net_device {
 #define NETIF_F_FCOE_MTU	(1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
 #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
 #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
+#define NETIF_F_RXCSUM		(1 << 29) /* Receive checksumming offload */
 
 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
@@ -2379,6 +2380,8 @@ static inline int dev_ethtool_get_settings(struct net_device *dev,
 
 static inline u32 dev_ethtool_get_rx_csum(struct net_device *dev)
 {
+	if (dev->hw_features & NETIF_F_RXCSUM)
+		return !!(dev->features & NETIF_F_RXCSUM);
 	if (!dev->ethtool_ops || !dev->ethtool_ops->get_rx_csum)
 		return 0;
 	return dev->ethtool_ops->get_rx_csum(dev);
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b068738..15e00b8 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -34,12 +34,6 @@ u32 ethtool_op_get_link(struct net_device *dev)
 }
 EXPORT_SYMBOL(ethtool_op_get_link);
 
-u32 ethtool_op_get_rx_csum(struct net_device *dev)
-{
-	return (dev->features & NETIF_F_ALL_CSUM) != 0;
-}
-EXPORT_SYMBOL(ethtool_op_get_rx_csum);
-
 u32 ethtool_op_get_tx_csum(struct net_device *dev)
 {
 	return (dev->features & NETIF_F_ALL_CSUM) != 0;
@@ -227,6 +221,9 @@ static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
 	case ETHTOOL_GTXCSUM:
 	case ETHTOOL_STXCSUM:
 		return NETIF_F_ALL_CSUM|NETIF_F_SCTP_CSUM;
+	case ETHTOOL_GRXCSUM:
+	case ETHTOOL_SRXCSUM:
+		return NETIF_F_RXCSUM;
 	case ETHTOOL_GSG:
 	case ETHTOOL_SSG:
 		return NETIF_F_SG;
@@ -261,6 +258,7 @@ static int ethtool_get_one_feature(struct net_device *dev, char __user *useraddr
 }
 
 static int __ethtool_set_tx_csum(struct net_device *dev, u32 data);
+static int __ethtool_set_rx_csum(struct net_device *dev, u32 data);
 static int __ethtool_set_sg(struct net_device *dev, u32 data);
 static int __ethtool_set_tso(struct net_device *dev, u32 data);
 static int __ethtool_set_ufo(struct net_device *dev, u32 data);
@@ -289,6 +287,8 @@ static int ethtool_set_one_feature(struct net_device *dev,
 	switch (ethcmd) {
 	case ETHTOOL_STXCSUM:
 		return __ethtool_set_tx_csum(dev, edata.data);
+	case ETHTOOL_SRXCSUM:
+		return __ethtool_set_rx_csum(dev, edata.data);
 	case ETHTOOL_SSG:
 		return __ethtool_set_sg(dev, edata.data);
 	case ETHTOOL_STSO:
@@ -1253,20 +1253,15 @@ static int __ethtool_set_tx_csum(struct net_device *dev, u32 data)
 	return dev->ethtool_ops->set_tx_csum(dev, data);
 }
 
-static int ethtool_set_rx_csum(struct net_device *dev, char __user *useraddr)
+static int __ethtool_set_rx_csum(struct net_device *dev, u32 data)
 {
-	struct ethtool_value edata;
-
 	if (!dev->ethtool_ops->set_rx_csum)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	if (!edata.data && dev->ethtool_ops->set_sg)
+	if (!data)
 		dev->features &= ~NETIF_F_GRO;
 
-	return dev->ethtool_ops->set_rx_csum(dev, edata.data);
+	return dev->ethtool_ops->set_rx_csum(dev, data);
 }
 
 static int __ethtool_set_tso(struct net_device *dev, u32 data)
@@ -1618,15 +1613,6 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SPAUSEPARAM:
 		rc = ethtool_set_pauseparam(dev, useraddr);
 		break;
-	case ETHTOOL_GRXCSUM:
-		rc = ethtool_get_value(dev, useraddr, ethcmd,
-				       (dev->ethtool_ops->get_rx_csum ?
-					dev->ethtool_ops->get_rx_csum :
-					ethtool_op_get_rx_csum));
-		break;
-	case ETHTOOL_SRXCSUM:
-		rc = ethtool_set_rx_csum(dev, useraddr);
-		break;
 	case ETHTOOL_TEST:
 		rc = ethtool_self_test(dev, useraddr);
 		break;
@@ -1700,6 +1686,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		rc = ethtool_set_features(dev, useraddr);
 		break;
 	case ETHTOOL_GTXCSUM:
+	case ETHTOOL_GRXCSUM:
 	case ETHTOOL_GSG:
 	case ETHTOOL_GTSO:
 	case ETHTOOL_GUFO:
@@ -1708,6 +1695,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 		rc = ethtool_get_one_feature(dev, useraddr, ethcmd);
 		break;
 	case ETHTOOL_STXCSUM:
+	case ETHTOOL_SRXCSUM:
 	case ETHTOOL_SSG:
 	case ETHTOOL_STSO:
 	case ETHTOOL_SUFO:
-- 
1.7.2.3

Re: [RFC PATCH 04/12] net: introduce NETIF_F_RXCSUM

[Ben Hutchings]

On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> Introduce NETIF_F_RXCSUM to replace device-private flags for RX checksum
> offload. Integrate it with ndo_fix_features.
> 
> ethtool_op_get_rx_csum() is removed altogether as nothing in-tree uses it.

Not explicitly, but what about drivers that turn TX and RX checksumming
on and off together?  They are implicitly covered by the case which
you're removing:

[...]
> -	case ETHTOOL_GRXCSUM:
> -		rc = ethtool_get_value(dev, useraddr, ethcmd,
> -				       (dev->ethtool_ops->get_rx_csum ?
> -					dev->ethtool_ops->get_rx_csum :
> -					ethtool_op_get_rx_csum));
[...]

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH 04/12] net: introduce NETIF_F_RXCSUM

[Michał Mirosław]

On Thu, Dec 16, 2010 at 11:27:29PM +0000, Ben Hutchings wrote:
> On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > Introduce NETIF_F_RXCSUM to replace device-private flags for RX checksum
> > offload. Integrate it with ndo_fix_features.
> > 
> > ethtool_op_get_rx_csum() is removed altogether as nothing in-tree uses it.
> Not explicitly, but what about drivers that turn TX and RX checksumming
> on and off together?  They are implicitly covered by the case which
> you're removing:
> [...]
> > -	case ETHTOOL_GRXCSUM:
> > -		rc = ethtool_get_value(dev, useraddr, ethcmd,
> > -				       (dev->ethtool_ops->get_rx_csum ?
> > -					dev->ethtool_ops->get_rx_csum :
> > -					ethtool_op_get_rx_csum));
> [...]

I had an impression that drivers not defining get_rx_csum() had no
RX checksumming or didn't allow it to be toggled. I'll have a closer look
on this.

Best Regards,
Michał Mirosław

Re: [RFC PATCH 04/12] net: introduce NETIF_F_RXCSUM

[Michał Mirosław]

On Sun, Dec 19, 2010 at 01:57:15AM +0100, Michał Mirosław wrote:
> On Thu, Dec 16, 2010 at 11:27:29PM +0000, Ben Hutchings wrote:
> > On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > > Introduce NETIF_F_RXCSUM to replace device-private flags for RX checksum
> > > offload. Integrate it with ndo_fix_features.
> > > 
> > > ethtool_op_get_rx_csum() is removed altogether as nothing in-tree uses it.
> > Not explicitly, but what about drivers that turn TX and RX checksumming
> > on and off together?  They are implicitly covered by the case which
> > you're removing:
> > [...]
> > > -	case ETHTOOL_GRXCSUM:
> > > -		rc = ethtool_get_value(dev, useraddr, ethcmd,
> > > -				       (dev->ethtool_ops->get_rx_csum ?
> > > -					dev->ethtool_ops->get_rx_csum :
> > > -					ethtool_op_get_rx_csum));
> > [...]
> I had an impression that drivers not defining get_rx_csum() had no
> RX checksumming or didn't allow it to be toggled. I'll have a closer look
> on this.

All drivers that have set_rx_csum() also define matching get_rx_csum().

Anyway, the code this is removing is broken in that it assumes TX csum
capability implying RX csum capability.

As a side note, GRO receive functions (TCPv4 and v6 - no other protocols
use it) are checking skb->ip_summed != CHECKSUM_NONE before allowing the
packet to be passed on, so it's not even necessary to force disabling GRO
when RX csum is off.

Best Regards,
Michał Mirosław

[RFC PATCH 09/12] virtio_net: convert to ndo_fix_features

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/virtio_net.c |   46 +++++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 90a23e4..a6b3914 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -705,17 +705,6 @@ static int virtnet_close(struct net_device *dev)
 	return 0;
 }
 
-static int virtnet_set_tx_csum(struct net_device *dev, u32 data)
-{
-	struct virtnet_info *vi = netdev_priv(dev);
-	struct virtio_device *vdev = vi->vdev;
-
-	if (data && !virtio_has_feature(vdev, VIRTIO_NET_F_CSUM))
-		return -ENOSYS;
-
-	return ethtool_op_set_tx_hw_csum(dev, data);
-}
-
 static void virtnet_set_rx_mode(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -817,10 +806,6 @@ static void virtnet_vlan_rx_kill_vid(struct net_device *dev, u16 vid)
 }
 
 static const struct ethtool_ops virtnet_ethtool_ops = {
-	.set_tx_csum = virtnet_set_tx_csum,
-	.set_sg = ethtool_op_set_sg,
-	.set_tso = ethtool_op_set_tso,
-	.set_ufo = ethtool_op_set_ufo,
 	.get_link = ethtool_op_get_link,
 };
 
@@ -907,22 +892,29 @@ static int virtnet_probe(struct virtio_device *vdev)
 	SET_NETDEV_DEV(dev, &vdev->dev);
 
 	/* Do we support "hardware" checksums? */
-	if (csum && virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CSUM)) {
 		/* This opens up the world of extra features. */
-		dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
-		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
-			dev->features |= NETIF_F_TSO | NETIF_F_UFO
+		dev->hw_features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+		if (csum)
+			dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
+
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
+			dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
 				| NETIF_F_TSO_ECN | NETIF_F_TSO6;
 		}
 		/* Individual feature bits: what can host handle? */
-		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_HOST_TSO4))
-			dev->features |= NETIF_F_TSO;
-		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_HOST_TSO6))
-			dev->features |= NETIF_F_TSO6;
-		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
-			dev->features |= NETIF_F_TSO_ECN;
-		if (gso && virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
-			dev->features |= NETIF_F_UFO;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_TSO4))
+			dev->hw_features |= NETIF_F_TSO;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_TSO6))
+			dev->hw_features |= NETIF_F_TSO6;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
+			dev->hw_features |= NETIF_F_TSO_ECN;
+		if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
+			dev->hw_features |= NETIF_F_UFO;
+
+		if (gso)
+			dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO);
+		/* (!csum && gso) case will be fixed by register_netdev() */
 	}
 
 	/* Configuration may specify what MAC to use.  Otherwise random. */
-- 
1.7.2.3

[RFC PATCH 08/12] jme: convert offload constraints to ndo_fix_features

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/jme.c |   79 +++++++++++++---------------------------------------
 drivers/net/jme.h |    2 -
 2 files changed, 20 insertions(+), 61 deletions(-)

diff --git a/drivers/net/jme.c b/drivers/net/jme.c
index 2411e72..1271109 100644
--- a/drivers/net/jme.c
+++ b/drivers/net/jme.c
@@ -2075,17 +2075,9 @@ jme_change_mtu(struct net_device *netdev, int new_mtu)
 		jme_restart_rx_engine(jme);
 	}
 
-	if (new_mtu > 1900) {
-		netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-				NETIF_F_TSO | NETIF_F_TSO6);
-	} else {
-		if (test_bit(JME_FLAG_TXCSUM, &jme->flags))
-			netdev->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-		if (test_bit(JME_FLAG_TSO, &jme->flags))
-			netdev->features |= NETIF_F_TSO | NETIF_F_TSO6;
-	}
-
 	netdev->mtu = new_mtu;
+	netdev_update_features(netdev);
+
 	jme_reset_link(jme);
 
 	return 0;
@@ -2482,20 +2474,21 @@ jme_set_msglevel(struct net_device *netdev, u32 value)
 	jme->msg_enable = value;
 }
 
-static u32
-jme_get_rx_csum(struct net_device *netdev)
+static unsigned long
+jme_fix_features(struct net_device *netdev, unsigned long features)
 {
-	struct jme_adapter *jme = netdev_priv(netdev);
-	return jme->reg_rxmcs & RXMCS_CHECKSUM;
+	if (netdev->mtu > 1900)
+		features &= ~(NETIF_F_ALL_TSO | NETIF_F_ALL_CSUM);
+	return features;
 }
 
 static int
-jme_set_rx_csum(struct net_device *netdev, u32 on)
+jme_set_features(struct net_device *netdev, unsigned long features)
 {
 	struct jme_adapter *jme = netdev_priv(netdev);
 
 	spin_lock_bh(&jme->rxmcs_lock);
-	if (on)
+	if (features & NETIF_F_RXCSUM)
 		jme->reg_rxmcs |= RXMCS_CHECKSUM;
 	else
 		jme->reg_rxmcs &= ~RXMCS_CHECKSUM;
@@ -2506,42 +2499,6 @@ jme_set_rx_csum(struct net_device *netdev, u32 on)
 }
 
 static int
-jme_set_tx_csum(struct net_device *netdev, u32 on)
-{
-	struct jme_adapter *jme = netdev_priv(netdev);
-
-	if (on) {
-		set_bit(JME_FLAG_TXCSUM, &jme->flags);
-		if (netdev->mtu <= 1900)
-			netdev->features |=
-				NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-	} else {
-		clear_bit(JME_FLAG_TXCSUM, &jme->flags);
-		netdev->features &=
-				~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-	}
-
-	return 0;
-}
-
-static int
-jme_set_tso(struct net_device *netdev, u32 on)
-{
-	struct jme_adapter *jme = netdev_priv(netdev);
-
-	if (on) {
-		set_bit(JME_FLAG_TSO, &jme->flags);
-		if (netdev->mtu <= 1900)
-			netdev->features |= NETIF_F_TSO | NETIF_F_TSO6;
-	} else {
-		clear_bit(JME_FLAG_TSO, &jme->flags);
-		netdev->features &= ~(NETIF_F_TSO | NETIF_F_TSO6);
-	}
-
-	return 0;
-}
-
-static int
 jme_nway_reset(struct net_device *netdev)
 {
 	struct jme_adapter *jme = netdev_priv(netdev);
@@ -2682,11 +2639,6 @@ static const struct ethtool_ops jme_ethtool_ops = {
 	.get_link		= jme_get_link,
 	.get_msglevel           = jme_get_msglevel,
 	.set_msglevel           = jme_set_msglevel,
-	.get_rx_csum		= jme_get_rx_csum,
-	.set_rx_csum		= jme_set_rx_csum,
-	.set_tx_csum		= jme_set_tx_csum,
-	.set_tso		= jme_set_tso,
-	.set_sg			= ethtool_op_set_sg,
 	.nway_reset             = jme_nway_reset,
 	.get_eeprom_len		= jme_get_eeprom_len,
 	.get_eeprom		= jme_get_eeprom,
@@ -2744,6 +2696,8 @@ static const struct net_device_ops jme_netdev_ops = {
 	.ndo_change_mtu		= jme_change_mtu,
 	.ndo_tx_timeout		= jme_tx_timeout,
 	.ndo_vlan_rx_register	= jme_vlan_rx_register,
+	.ndo_fix_features       = jme_fix_features,
+	.ndo_set_features       = jme_set_features,
 };
 
 static int __devinit
@@ -2798,6 +2752,12 @@ jme_init_one(struct pci_dev *pdev,
 	netdev->netdev_ops = &jme_netdev_ops;
 	netdev->ethtool_ops		= &jme_ethtool_ops;
 	netdev->watchdog_timeo		= TX_TIMEOUT;
+	netdev->hw_features		=	NETIF_F_IP_CSUM |
+						NETIF_F_IPV6_CSUM |
+						NETIF_F_SG |
+						NETIF_F_TSO |
+						NETIF_F_TSO6 |
+						NETIF_F_RXCSUM;
 	netdev->features		=	NETIF_F_IP_CSUM |
 						NETIF_F_IPV6_CSUM |
 						NETIF_F_SG |
@@ -2880,8 +2840,9 @@ jme_init_one(struct pci_dev *pdev,
 	jme->reg_rxmcs = RXMCS_DEFAULT;
 	jme->reg_txpfc = 0;
 	jme->reg_pmcs = PMCS_MFEN;
-	set_bit(JME_FLAG_TXCSUM, &jme->flags);
-	set_bit(JME_FLAG_TSO, &jme->flags);
+
+	if (jme->reg_rxmcs & RXMCS_CHECKSUM)
+		netdev->features |= NETIF_F_RXCSUM;
 
 	/*
 	 * Get Max Read Req Size from PCI Config Space
diff --git a/drivers/net/jme.h b/drivers/net/jme.h
index eac0926..4923376 100644
--- a/drivers/net/jme.h
+++ b/drivers/net/jme.h
@@ -434,8 +434,6 @@ struct jme_adapter {
 enum jme_flags_bits {
 	JME_FLAG_MSI		= 1,
 	JME_FLAG_SSET		= 2,
-	JME_FLAG_TXCSUM		= 3,
-	JME_FLAG_TSO		= 4,
 	JME_FLAG_POLL		= 5,
 	JME_FLAG_SHUTDOWN	= 6,
 };
-- 
1.7.2.3

[RFC PATCH 11/12] veth: convert to hw_features

[Michał Mirosław]

This depends on 'net/veth: Fix packet checksumming' patch.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/veth.c |   45 +++++----------------------------------------
 1 files changed, 5 insertions(+), 40 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index cca6a71..6e9f23c 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -36,7 +36,6 @@ struct veth_net_stats {
 struct veth_priv {
 	struct net_device *peer;
 	struct veth_net_stats __percpu *stats;
-	unsigned ip_summed;
 };
 
 /*
@@ -99,47 +98,10 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 	data[0] = priv->peer->ifindex;
 }
 
-static u32 veth_get_rx_csum(struct net_device *dev)
-{
-	struct veth_priv *priv;
-
-	priv = netdev_priv(dev);
-	return priv->ip_summed == CHECKSUM_UNNECESSARY;
-}
-
-static int veth_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct veth_priv *priv;
-
-	priv = netdev_priv(dev);
-	priv->ip_summed = data ? CHECKSUM_UNNECESSARY : CHECKSUM_NONE;
-	return 0;
-}
-
-static u32 veth_get_tx_csum(struct net_device *dev)
-{
-	return (dev->features & NETIF_F_NO_CSUM) != 0;
-}
-
-static int veth_set_tx_csum(struct net_device *dev, u32 data)
-{
-	if (data)
-		dev->features |= NETIF_F_NO_CSUM;
-	else
-		dev->features &= ~NETIF_F_NO_CSUM;
-	return 0;
-}
-
 static const struct ethtool_ops veth_ethtool_ops = {
 	.get_settings		= veth_get_settings,
 	.get_drvinfo		= veth_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
-	.get_rx_csum		= veth_get_rx_csum,
-	.set_rx_csum		= veth_set_rx_csum,
-	.get_tx_csum		= veth_get_tx_csum,
-	.set_tx_csum		= veth_set_tx_csum,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
 	.get_strings		= veth_get_strings,
 	.get_sset_count		= veth_get_sset_count,
 	.get_ethtool_stats	= veth_get_ethtool_stats,
@@ -168,8 +130,9 @@ static netdev_tx_t veth_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* we can't change ip_summed == CHECKSUM_PARTIAL, as that
 	   will cause bad checksum on forwarded packets */
-	if (skb->ip_summed == CHECKSUM_NONE)
-		skb->ip_summed = rcv_priv->ip_summed;
+	if (skb->ip_summed == CHECKSUM_NONE &&
+	    rcv->features & NETIF_F_RXCSUM)
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	length = skb->len + ETH_HLEN;
 	if (dev_forward_skb(rcv, skb) != NET_RX_SUCCESS)
@@ -304,6 +267,8 @@ static void veth_setup(struct net_device *dev)
 	dev->ethtool_ops = &veth_ethtool_ops;
 	dev->features |= NETIF_F_LLTX;
 	dev->destructor = veth_dev_free;
+
+	dev->hw_features = NETIF_F_NO_CSUM | NETIF_F_SG | NETIF_F_RXCSUM;
 }
 
 /*
-- 
1.7.2.3

[RFC PATCH 12/12] skge: convert to hw_features

[Michał Mirosław]

The hardware might do full HW_CSUM, not just IP_CSUM, but it's not tested
and so not changed here.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/skge.c |   53 ++++-----------------------------------------------
 drivers/net/skge.h |    1 -
 2 files changed, 5 insertions(+), 49 deletions(-)

diff --git a/drivers/net/skge.c b/drivers/net/skge.c
index 50815fb..41e02e9 100644
--- a/drivers/net/skge.c
+++ b/drivers/net/skge.c
@@ -537,46 +537,6 @@ static int skge_nway_reset(struct net_device *dev)
 	return 0;
 }
 
-static int skge_set_sg(struct net_device *dev, u32 data)
-{
-	struct skge_port *skge = netdev_priv(dev);
-	struct skge_hw *hw = skge->hw;
-
-	if (hw->chip_id == CHIP_ID_GENESIS && data)
-		return -EOPNOTSUPP;
-	return ethtool_op_set_sg(dev, data);
-}
-
-static int skge_set_tx_csum(struct net_device *dev, u32 data)
-{
-	struct skge_port *skge = netdev_priv(dev);
-	struct skge_hw *hw = skge->hw;
-
-	if (hw->chip_id == CHIP_ID_GENESIS && data)
-		return -EOPNOTSUPP;
-
-	return ethtool_op_set_tx_csum(dev, data);
-}
-
-static u32 skge_get_rx_csum(struct net_device *dev)
-{
-	struct skge_port *skge = netdev_priv(dev);
-
-	return skge->rx_csum;
-}
-
-/* Only Yukon supports checksum offload. */
-static int skge_set_rx_csum(struct net_device *dev, u32 data)
-{
-	struct skge_port *skge = netdev_priv(dev);
-
-	if (skge->hw->chip_id == CHIP_ID_GENESIS && data)
-		return -EOPNOTSUPP;
-
-	skge->rx_csum = data;
-	return 0;
-}
-
 static void skge_get_pauseparam(struct net_device *dev,
 				struct ethtool_pauseparam *ecmd)
 {
@@ -925,10 +885,6 @@ static const struct ethtool_ops skge_ethtool_ops = {
 	.set_pauseparam = skge_set_pauseparam,
 	.get_coalesce	= skge_get_coalesce,
 	.set_coalesce	= skge_set_coalesce,
-	.set_sg		= skge_set_sg,
-	.set_tx_csum	= skge_set_tx_csum,
-	.get_rx_csum	= skge_get_rx_csum,
-	.set_rx_csum	= skge_set_rx_csum,
 	.get_strings	= skge_get_strings,
 	.phys_id	= skge_phys_id,
 	.get_sset_count = skge_get_sset_count,
@@ -3085,7 +3041,8 @@ static struct sk_buff *skge_rx_get(struct net_device *dev,
 	}
 
 	skb_put(skb, len);
-	if (skge->rx_csum) {
+
+	if (dev->features & NETIF_F_RXCSUM) {
 		skb->csum = csum;
 		skb->ip_summed = CHECKSUM_COMPLETE;
 	}
@@ -3847,10 +3804,10 @@ static struct net_device *skge_devinit(struct skge_hw *hw, int port,
 	setup_timer(&skge->link_timer, xm_link_timer, (unsigned long) skge);
 
 	if (hw->chip_id != CHIP_ID_GENESIS) {
-		dev->features |= NETIF_F_IP_CSUM | NETIF_F_SG;
-		skge->rx_csum = 1;
+		dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
+		                   NETIF_F_RXCSUM;
+		dev->features |= dev->hw_features;
 	}
-	dev->features |= NETIF_F_GRO;
 
 	/* read the mac address */
 	memcpy_fromio(dev->dev_addr, hw->regs + B2_MAC_1 + port*8, ETH_ALEN);
diff --git a/drivers/net/skge.h b/drivers/net/skge.h
index 507addc..f055c47 100644
--- a/drivers/net/skge.h
+++ b/drivers/net/skge.h
@@ -2460,7 +2460,6 @@ struct skge_port {
 	struct timer_list    link_timer;
 	enum pause_control   flow_control;
 	enum pause_status    flow_status;
-	u8		     rx_csum;
 	u8		     blink_on;
 	u8		     wol;
 	u8		     autoneg;	/* AUTONEG_ENABLE, AUTONEG_DISABLE */
-- 
1.7.2.3

[RFC PATCH 10/12] Intel net drivers: convert to ndo_fix_features

[Michał Mirosław]

Private rx_csum flags are now duplicate of netdev->features & NETIF_F_RXCSUM.
Removing this needs deeper surgery.

Since ixgbevf doesn't change hardware state on RX csum enable/disable
its reset is avoided.

Things noticed:
 - e1000, e1000e and ixgb have RX csum disabled by default
 - HW VLAN acceleration probably can be toggled, but it's left as is
 - the resets on RX csum offload change can probably be avoided
 - there is A LOT of copy-and-pasted code here

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/e1000/e1000_ethtool.c  |   69 ------------------------------------
 drivers/net/e1000/e1000_main.c     |   31 ++++++++++++++--
 drivers/net/e1000e/ethtool.c       |   62 --------------------------------
 drivers/net/e1000e/netdev.c        |   31 ++++++++++++++--
 drivers/net/igb/igb_ethtool.c      |   67 ----------------------------------
 drivers/net/igb/igb_main.c         |   34 ++++++++++++++----
 drivers/net/igbvf/ethtool.c        |   57 -----------------------------
 drivers/net/igbvf/netdev.c         |   26 +++++++++++---
 drivers/net/ixgb/ixgb.h            |    2 +
 drivers/net/ixgb/ixgb_ethtool.c    |   59 +------------------------------
 drivers/net/ixgb/ixgb_main.c       |   31 ++++++++++++++--
 drivers/net/ixgbe/ixgbe_ethtool.c  |   65 ---------------------------------
 drivers/net/ixgbe/ixgbe_main.c     |   32 ++++++++++++-----
 drivers/net/ixgbevf/ethtool.c      |   46 ------------------------
 drivers/net/ixgbevf/ixgbevf_main.c |   27 +++++++++++---
 15 files changed, 178 insertions(+), 461 deletions(-)

diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index f4d0922..8002b5b 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -288,69 +288,6 @@ static int e1000_set_pauseparam(struct net_device *netdev,
 	return retval;
 }
 
-static u32 e1000_get_rx_csum(struct net_device *netdev)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	return adapter->rx_csum;
-}
-
-static int e1000_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	adapter->rx_csum = data;
-
-	if (netif_running(netdev))
-		e1000_reinit_locked(adapter);
-	else
-		e1000_reset(adapter);
-	return 0;
-}
-
-static u32 e1000_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_HW_CSUM) != 0;
-}
-
-static int e1000_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
-
-	if (hw->mac_type < e1000_82543) {
-		if (!data)
-			return -EINVAL;
-		return 0;
-	}
-
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-
-	return 0;
-}
-
-static int e1000_set_tso(struct net_device *netdev, u32 data)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
-
-	if ((hw->mac_type < e1000_82544) ||
-	    (hw->mac_type == e1000_82547))
-		return data ? -EINVAL : 0;
-
-	if (data)
-		netdev->features |= NETIF_F_TSO;
-	else
-		netdev->features &= ~NETIF_F_TSO;
-
-	netdev->features &= ~NETIF_F_TSO6;
-
-	e_info(probe, "TSO is %s\n", data ? "Enabled" : "Disabled");
-	adapter->tso_force = true;
-	return 0;
-}
-
 static u32 e1000_get_msglevel(struct net_device *netdev)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
@@ -1921,12 +1858,6 @@ static const struct ethtool_ops e1000_ethtool_ops = {
 	.set_ringparam          = e1000_set_ringparam,
 	.get_pauseparam         = e1000_get_pauseparam,
 	.set_pauseparam         = e1000_set_pauseparam,
-	.get_rx_csum            = e1000_get_rx_csum,
-	.set_rx_csum            = e1000_set_rx_csum,
-	.get_tx_csum            = e1000_get_tx_csum,
-	.set_tx_csum            = e1000_set_tx_csum,
-	.set_sg                 = ethtool_op_set_sg,
-	.set_tso                = e1000_set_tso,
 	.self_test              = e1000_diag_test,
 	.get_strings            = e1000_get_strings,
 	.phys_id                = e1000_phys_id,
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 340e12d..a85765e 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -788,6 +788,25 @@ static int e1000_is_need_ioport(struct pci_dev *pdev)
 	}
 }
 
+static int e1000_set_features(struct net_device *netdev,
+	unsigned long features)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	unsigned long changed = features ^ netdev->features;
+
+	if (!(changed & NETIF_F_RXCSUM))
+		return 0;
+
+	adapter->rx_csum = !!(features & NETIF_F_RXCSUM);
+
+	if (netif_running(netdev))
+		e1000_reinit_locked(adapter);
+	else
+		e1000_reset(adapter);
+
+	return 0;
+}
+
 static const struct net_device_ops e1000_netdev_ops = {
 	.ndo_open		= e1000_open,
 	.ndo_stop		= e1000_close,
@@ -806,6 +825,7 @@ static const struct net_device_ops e1000_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= e1000_netpoll,
 #endif
+	.ndo_set_features       = e1000_set_features,
 };
 
 /**
@@ -998,16 +1018,19 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 	err = -EIO;
 
 	if (hw->mac_type >= e1000_82543) {
-		netdev->features = NETIF_F_SG |
-				   NETIF_F_HW_CSUM |
-				   NETIF_F_HW_VLAN_TX |
+		netdev->hw_features = NETIF_F_SG |
+				   NETIF_F_HW_CSUM;
+		netdev->features = NETIF_F_HW_VLAN_TX |
 				   NETIF_F_HW_VLAN_RX |
 				   NETIF_F_HW_VLAN_FILTER;
 	}
 
 	if ((hw->mac_type >= e1000_82544) &&
 	   (hw->mac_type != e1000_82547))
-		netdev->features |= NETIF_F_TSO;
+		netdev->hw_features |= NETIF_F_TSO;
+
+	netdev->features |= netdev->hw_features;
+	netdev->hw_features |= NETIF_F_RXCSUM;
 
 	if (pci_using_dac) {
 		netdev->features |= NETIF_F_HIGHDMA;
diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
index 39349d6..adb6f14 100644
--- a/drivers/net/e1000e/ethtool.c
+++ b/drivers/net/e1000e/ethtool.c
@@ -351,59 +351,6 @@ out:
 	return retval;
 }
 
-static u32 e1000_get_rx_csum(struct net_device *netdev)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	return adapter->flags & FLAG_RX_CSUM_ENABLED;
-}
-
-static int e1000_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-
-	if (data)
-		adapter->flags |= FLAG_RX_CSUM_ENABLED;
-	else
-		adapter->flags &= ~FLAG_RX_CSUM_ENABLED;
-
-	if (netif_running(netdev))
-		e1000e_reinit_locked(adapter);
-	else
-		e1000e_reset(adapter);
-	return 0;
-}
-
-static u32 e1000_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_HW_CSUM) != 0;
-}
-
-static int e1000_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-
-	return 0;
-}
-
-static int e1000_set_tso(struct net_device *netdev, u32 data)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-
-	if (data) {
-		netdev->features |= NETIF_F_TSO;
-		netdev->features |= NETIF_F_TSO6;
-	} else {
-		netdev->features &= ~NETIF_F_TSO;
-		netdev->features &= ~NETIF_F_TSO6;
-	}
-
-	adapter->flags |= FLAG_TSO_FORCE;
-	return 0;
-}
-
 static u32 e1000_get_msglevel(struct net_device *netdev)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
@@ -2027,14 +1974,6 @@ static const struct ethtool_ops e1000_ethtool_ops = {
 	.set_ringparam		= e1000_set_ringparam,
 	.get_pauseparam		= e1000_get_pauseparam,
 	.set_pauseparam		= e1000_set_pauseparam,
-	.get_rx_csum		= e1000_get_rx_csum,
-	.set_rx_csum		= e1000_set_rx_csum,
-	.get_tx_csum		= e1000_get_tx_csum,
-	.set_tx_csum		= e1000_set_tx_csum,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= e1000_set_tso,
 	.self_test		= e1000_diag_test,
 	.get_strings		= e1000_get_strings,
 	.phys_id		= e1000_phys_id,
@@ -2042,7 +1981,6 @@ static const struct ethtool_ops e1000_ethtool_ops = {
 	.get_sset_count		= e1000e_get_sset_count,
 	.get_coalesce		= e1000_get_coalesce,
 	.set_coalesce		= e1000_set_coalesce,
-	.get_flags		= ethtool_op_get_flags,
 };
 
 void e1000e_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index a45dafd..8d71c35 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -5663,6 +5663,28 @@ static void e1000_eeprom_checks(struct e1000_adapter *adapter)
 	}
 }
 
+static int e1000_set_features(struct net_device *netdev,
+	unsigned long features)
+{
+	struct e1000_adapter *adapter = netdev_priv(netdev);
+	unsigned long changed = features ^ netdev->features;
+
+	if (!(changed & NETIF_F_RXCSUM))
+		return 0;
+
+	if (features & NETIF_F_RXCSUM)
+		adapter->flags |= FLAG_RX_CSUM_ENABLED;
+	else
+		adapter->flags &= ~FLAG_RX_CSUM_ENABLED;
+
+	if (netif_running(netdev))
+		e1000e_reinit_locked(adapter);
+	else
+		e1000e_reset(adapter);
+
+	return 0;
+}
+
 static const struct net_device_ops e1000e_netdev_ops = {
 	.ndo_open		= e1000_open,
 	.ndo_stop		= e1000_close,
@@ -5681,6 +5703,7 @@ static const struct net_device_ops e1000e_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= e1000_netpoll,
 #endif
+	.ndo_set_features	= e1000_set_features,
 };
 
 /**
@@ -5835,17 +5858,17 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
 	if (e1000_check_reset_block(&adapter->hw))
 		e_info("PHY reset is blocked due to SOL/IDER session.\n");
 
-	netdev->features = NETIF_F_SG |
+	netdev->hw_features = NETIF_F_SG |
 			   NETIF_F_HW_CSUM |
+			   NETIF_F_TSO | NETIF_F_TSO6;
+	netdev->features = netdev->hw_features |
 			   NETIF_F_HW_VLAN_TX |
 			   NETIF_F_HW_VLAN_RX;
+	netdev->hw_features |= NETIF_F_RXCSUM;
 
 	if (adapter->flags & FLAG_HAS_HW_VLAN_FILTER)
 		netdev->features |= NETIF_F_HW_VLAN_FILTER;
 
-	netdev->features |= NETIF_F_TSO;
-	netdev->features |= NETIF_F_TSO6;
-
 	netdev->vlan_features |= NETIF_F_TSO;
 	netdev->vlan_features |= NETIF_F_TSO6;
 	netdev->vlan_features |= NETIF_F_HW_CSUM;
diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index a70e16b..7736b59 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -313,65 +313,6 @@ static int igb_set_pauseparam(struct net_device *netdev,
 	return retval;
 }
 
-static u32 igb_get_rx_csum(struct net_device *netdev)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-	return !!(adapter->rx_ring[0]->flags & IGB_RING_FLAG_RX_CSUM);
-}
-
-static int igb_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-	int i;
-
-	for (i = 0; i < adapter->num_rx_queues; i++) {
-		if (data)
-			adapter->rx_ring[i]->flags |= IGB_RING_FLAG_RX_CSUM;
-		else
-			adapter->rx_ring[i]->flags &= ~IGB_RING_FLAG_RX_CSUM;
-	}
-
-	return 0;
-}
-
-static u32 igb_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_IP_CSUM) != 0;
-}
-
-static int igb_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-
-	if (data) {
-		netdev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-		if (adapter->hw.mac.type >= e1000_82576)
-			netdev->features |= NETIF_F_SCTP_CSUM;
-	} else {
-		netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-		                      NETIF_F_SCTP_CSUM);
-	}
-
-	return 0;
-}
-
-static int igb_set_tso(struct net_device *netdev, u32 data)
-{
-	struct igb_adapter *adapter = netdev_priv(netdev);
-
-	if (data) {
-		netdev->features |= NETIF_F_TSO;
-		netdev->features |= NETIF_F_TSO6;
-	} else {
-		netdev->features &= ~NETIF_F_TSO;
-		netdev->features &= ~NETIF_F_TSO6;
-	}
-
-	dev_info(&adapter->pdev->dev, "TSO is %s\n",
-		 data ? "Enabled" : "Disabled");
-	return 0;
-}
-
 static u32 igb_get_msglevel(struct net_device *netdev)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
@@ -2189,14 +2130,6 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.set_ringparam          = igb_set_ringparam,
 	.get_pauseparam         = igb_get_pauseparam,
 	.set_pauseparam         = igb_set_pauseparam,
-	.get_rx_csum            = igb_get_rx_csum,
-	.set_rx_csum            = igb_set_rx_csum,
-	.get_tx_csum            = igb_get_tx_csum,
-	.set_tx_csum            = igb_set_tx_csum,
-	.get_sg                 = ethtool_op_get_sg,
-	.set_sg                 = ethtool_op_set_sg,
-	.get_tso                = ethtool_op_get_tso,
-	.set_tso                = igb_set_tso,
 	.self_test              = igb_diag_test,
 	.get_strings            = igb_get_strings,
 	.phys_id                = igb_phys_id,
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 62348fc..663144a 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1687,6 +1687,22 @@ void igb_reset(struct igb_adapter *adapter)
 	igb_get_phy_info(hw);
 }
 
+static int igb_set_features(struct net_device *netdev,
+	unsigned long features)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	int i;
+
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		if (features & NETIF_F_RXCSUM)
+			adapter->rx_ring[i]->flags |= IGB_RING_FLAG_RX_CSUM;
+		else
+			adapter->rx_ring[i]->flags &= ~IGB_RING_FLAG_RX_CSUM;
+	}
+
+	return 0;
+}
+
 static const struct net_device_ops igb_netdev_ops = {
 	.ndo_open		= igb_open,
 	.ndo_stop		= igb_close,
@@ -1709,6 +1725,7 @@ static const struct net_device_ops igb_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= igb_netpoll,
 #endif
+	.ndo_set_features       = igb_set_features,
 };
 
 /**
@@ -1848,17 +1865,18 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 		dev_info(&pdev->dev,
 			"PHY reset is blocked due to SOL/IDER session.\n");
 
-	netdev->features = NETIF_F_SG |
+	netdev->hw_features = NETIF_F_SG |
 			   NETIF_F_IP_CSUM |
+			   NETIF_F_IPV6_CSUM |
+			   NETIF_F_TSO |
+			   NETIF_F_TSO6 |
+			   NETIF_F_RXCSUM;
+
+	netdev->features = netdev->hw_features |
 			   NETIF_F_HW_VLAN_TX |
 			   NETIF_F_HW_VLAN_RX |
 			   NETIF_F_HW_VLAN_FILTER;
 
-	netdev->features |= NETIF_F_IPV6_CSUM;
-	netdev->features |= NETIF_F_TSO;
-	netdev->features |= NETIF_F_TSO6;
-	netdev->features |= NETIF_F_GRO;
-
 	netdev->vlan_features |= NETIF_F_TSO;
 	netdev->vlan_features |= NETIF_F_TSO6;
 	netdev->vlan_features |= NETIF_F_IP_CSUM;
@@ -1870,8 +1888,10 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 		netdev->vlan_features |= NETIF_F_HIGHDMA;
 	}
 
-	if (hw->mac.type >= e1000_82576)
+	if (hw->mac.type >= e1000_82576) {
+		netdev->hw_features |= NETIF_F_SCTP_CSUM;
 		netdev->features |= NETIF_F_SCTP_CSUM;
+	}
 
 	adapter->en_mng_pt = igb_enable_mng_pass_thru(hw);
 
diff --git a/drivers/net/igbvf/ethtool.c b/drivers/net/igbvf/ethtool.c
index ed6e3d9..91ada3c 100644
--- a/drivers/net/igbvf/ethtool.c
+++ b/drivers/net/igbvf/ethtool.c
@@ -128,55 +128,6 @@ static int igbvf_set_pauseparam(struct net_device *netdev,
 	return -EOPNOTSUPP;
 }
 
-static u32 igbvf_get_rx_csum(struct net_device *netdev)
-{
-	struct igbvf_adapter *adapter = netdev_priv(netdev);
-	return !(adapter->flags & IGBVF_FLAG_RX_CSUM_DISABLED);
-}
-
-static int igbvf_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct igbvf_adapter *adapter = netdev_priv(netdev);
-
-	if (data)
-		adapter->flags &= ~IGBVF_FLAG_RX_CSUM_DISABLED;
-	else
-		adapter->flags |= IGBVF_FLAG_RX_CSUM_DISABLED;
-
-	return 0;
-}
-
-static u32 igbvf_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_IP_CSUM) != 0;
-}
-
-static int igbvf_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	if (data)
-		netdev->features |= (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-	else
-		netdev->features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-	return 0;
-}
-
-static int igbvf_set_tso(struct net_device *netdev, u32 data)
-{
-	struct igbvf_adapter *adapter = netdev_priv(netdev);
-
-	if (data) {
-		netdev->features |= NETIF_F_TSO;
-		netdev->features |= NETIF_F_TSO6;
-	} else {
-		netdev->features &= ~NETIF_F_TSO;
-		netdev->features &= ~NETIF_F_TSO6;
-	}
-
-	dev_info(&adapter->pdev->dev, "TSO is %s\n",
-	         data ? "Enabled" : "Disabled");
-	return 0;
-}
-
 static u32 igbvf_get_msglevel(struct net_device *netdev)
 {
 	struct igbvf_adapter *adapter = netdev_priv(netdev);
@@ -518,14 +469,6 @@ static const struct ethtool_ops igbvf_ethtool_ops = {
 	.set_ringparam		= igbvf_set_ringparam,
 	.get_pauseparam		= igbvf_get_pauseparam,
 	.set_pauseparam		= igbvf_set_pauseparam,
-	.get_rx_csum            = igbvf_get_rx_csum,
-	.set_rx_csum            = igbvf_set_rx_csum,
-	.get_tx_csum		= igbvf_get_tx_csum,
-	.set_tx_csum		= igbvf_set_tx_csum,
-	.get_sg			= ethtool_op_get_sg,
-	.set_sg			= ethtool_op_set_sg,
-	.get_tso		= ethtool_op_get_tso,
-	.set_tso		= igbvf_set_tso,
 	.self_test		= igbvf_diag_test,
 	.get_sset_count		= igbvf_get_sset_count,
 	.get_strings		= igbvf_get_strings,
diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
index 4fb023b..71acae7 100644
--- a/drivers/net/igbvf/netdev.c
+++ b/drivers/net/igbvf/netdev.c
@@ -2597,6 +2597,19 @@ static void igbvf_print_device_info(struct igbvf_adapter *adapter)
 	dev_info(&pdev->dev, "MAC: %d\n", hw->mac.type);
 }
 
+static int igbvf_set_features(struct net_device *netdev,
+	unsigned long features)
+{
+	struct igbvf_adapter *adapter = netdev_priv(netdev);
+
+	if (features & NETIF_F_RXCSUM)
+		adapter->flags &= ~IGBVF_FLAG_RX_CSUM_DISABLED;
+	else
+		adapter->flags |= IGBVF_FLAG_RX_CSUM_DISABLED;
+
+	return 0;
+}
+
 static const struct net_device_ops igbvf_netdev_ops = {
 	.ndo_open                       = igbvf_open,
 	.ndo_stop                       = igbvf_close,
@@ -2613,6 +2626,7 @@ static const struct net_device_ops igbvf_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller            = igbvf_netpoll,
 #endif
+	.ndo_set_features               = igbvf_set_features,
 };
 
 /**
@@ -2721,16 +2735,18 @@ static int __devinit igbvf_probe(struct pci_dev *pdev,
 
 	adapter->bd_number = cards_found++;
 
-	netdev->features = NETIF_F_SG |
+	netdev->hw_features = NETIF_F_SG |
 	                   NETIF_F_IP_CSUM |
+	                   NETIF_F_IPV6_CSUM |
+			   NETIF_F_TSO |
+			   NETIF_F_TSO6 |
+			   NETIF_F_RXCSUM;
+
+	netdev->features = netdev->hw_features |
 	                   NETIF_F_HW_VLAN_TX |
 	                   NETIF_F_HW_VLAN_RX |
 	                   NETIF_F_HW_VLAN_FILTER;
 
-	netdev->features |= NETIF_F_IPV6_CSUM;
-	netdev->features |= NETIF_F_TSO;
-	netdev->features |= NETIF_F_TSO6;
-
 	if (pci_using_dac)
 		netdev->features |= NETIF_F_HIGHDMA;
 
diff --git a/drivers/net/ixgb/ixgb.h b/drivers/net/ixgb/ixgb.h
index 521c0c7..ac0f424 100644
--- a/drivers/net/ixgb/ixgb.h
+++ b/drivers/net/ixgb/ixgb.h
@@ -207,6 +207,8 @@ extern void ixgb_set_ethtool_ops(struct net_device *netdev);
 extern char ixgb_driver_name[];
 extern const char ixgb_driver_version[];
 
+extern void ixgb_set_speed_duplex(struct net_device *netdev);
+
 extern int ixgb_up(struct ixgb_adapter *adapter);
 extern void ixgb_down(struct ixgb_adapter *adapter, bool kill_watchdog);
 extern void ixgb_reset(struct ixgb_adapter *adapter);
diff --git a/drivers/net/ixgb/ixgb_ethtool.c b/drivers/net/ixgb/ixgb_ethtool.c
index 43994c1..35e6f11 100644
--- a/drivers/net/ixgb/ixgb_ethtool.c
+++ b/drivers/net/ixgb/ixgb_ethtool.c
@@ -115,7 +115,7 @@ ixgb_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 	return 0;
 }
 
-static void ixgb_set_speed_duplex(struct net_device *netdev)
+void ixgb_set_speed_duplex(struct net_device *netdev)
 {
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
 	/* be optimistic about our link, since we were up before */
@@ -194,57 +194,6 @@ ixgb_set_pauseparam(struct net_device *netdev,
 }
 
 static u32
-ixgb_get_rx_csum(struct net_device *netdev)
-{
-	struct ixgb_adapter *adapter = netdev_priv(netdev);
-
-	return adapter->rx_csum;
-}
-
-static int
-ixgb_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct ixgb_adapter *adapter = netdev_priv(netdev);
-
-	adapter->rx_csum = data;
-
-	if (netif_running(netdev)) {
-		ixgb_down(adapter, true);
-		ixgb_up(adapter);
-		ixgb_set_speed_duplex(netdev);
-	} else
-		ixgb_reset(adapter);
-	return 0;
-}
-
-static u32
-ixgb_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_HW_CSUM) != 0;
-}
-
-static int
-ixgb_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	if (data)
-		netdev->features |= NETIF_F_HW_CSUM;
-	else
-		netdev->features &= ~NETIF_F_HW_CSUM;
-
-	return 0;
-}
-
-static int
-ixgb_set_tso(struct net_device *netdev, u32 data)
-{
-	if (data)
-		netdev->features |= NETIF_F_TSO;
-	else
-		netdev->features &= ~NETIF_F_TSO;
-	return 0;
-}
-
-static u32
 ixgb_get_msglevel(struct net_device *netdev)
 {
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
@@ -720,14 +669,8 @@ static const struct ethtool_ops ixgb_ethtool_ops = {
 	.set_ringparam = ixgb_set_ringparam,
 	.get_pauseparam	= ixgb_get_pauseparam,
 	.set_pauseparam	= ixgb_set_pauseparam,
-	.get_rx_csum = ixgb_get_rx_csum,
-	.set_rx_csum = ixgb_set_rx_csum,
-	.get_tx_csum = ixgb_get_tx_csum,
-	.set_tx_csum = ixgb_set_tx_csum,
-	.set_sg	= ethtool_op_set_sg,
 	.get_msglevel = ixgb_get_msglevel,
 	.set_msglevel = ixgb_set_msglevel,
-	.set_tso = ixgb_set_tso,
 	.get_strings = ixgb_get_strings,
 	.phys_id = ixgb_phys_id,
 	.get_sset_count = ixgb_get_sset_count,
diff --git a/drivers/net/ixgb/ixgb_main.c b/drivers/net/ixgb/ixgb_main.c
index 5639ccc..0a1b036 100644
--- a/drivers/net/ixgb/ixgb_main.c
+++ b/drivers/net/ixgb/ixgb_main.c
@@ -326,6 +326,28 @@ ixgb_reset(struct ixgb_adapter *adapter)
 	}
 }
 
+static int
+ixgb_set_features(struct net_device *netdev, unsigned long features)
+{
+	struct ixgb_adapter *adapter = netdev_priv(netdev);
+	unsigned long changed = features ^ netdev->features;
+
+        if (!(changed & NETIF_F_RXCSUM))
+                return 0;
+
+	adapter->rx_csum = !!(features & NETIF_F_RXCSUM);
+
+	if (netif_running(netdev)) {
+		ixgb_down(adapter, true);
+		ixgb_up(adapter);
+		ixgb_set_speed_duplex(netdev);
+	} else
+		ixgb_reset(adapter);
+
+	return 0;
+}
+
+
 static const struct net_device_ops ixgb_netdev_ops = {
 	.ndo_open 		= ixgb_open,
 	.ndo_stop		= ixgb_close,
@@ -342,6 +364,7 @@ static const struct net_device_ops ixgb_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= ixgb_netpoll,
 #endif
+	.ndo_set_features       = ixgb_set_features,
 };
 
 /**
@@ -441,12 +464,14 @@ ixgb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		goto err_sw_init;
 
-	netdev->features = NETIF_F_SG |
-			   NETIF_F_HW_CSUM |
+	netdev->hw_features = NETIF_F_SG |
+			   NETIF_F_TSO |
+			   NETIF_F_HW_CSUM;
+	netdev->features = netdev->hw_features |
 			   NETIF_F_HW_VLAN_TX |
 			   NETIF_F_HW_VLAN_RX |
 			   NETIF_F_HW_VLAN_FILTER;
-	netdev->features |= NETIF_F_TSO;
+	netdev->hw_features |= NETIF_F_RXCSUM;
 
 	if (pci_using_dac) {
 		netdev->features |= NETIF_F_HIGHDMA;
diff --git a/drivers/net/ixgbe/ixgbe_ethtool.c b/drivers/net/ixgbe/ixgbe_ethtool.c
index 23ff23e..f0795db 100644
--- a/drivers/net/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c
@@ -406,62 +406,6 @@ static int ixgbe_set_pauseparam(struct net_device *netdev,
 	return 0;
 }
 
-static u32 ixgbe_get_rx_csum(struct net_device *netdev)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	return adapter->flags & IXGBE_FLAG_RX_CSUM_ENABLED;
-}
-
-static int ixgbe_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	if (data)
-		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
-	else
-		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
-
-	return 0;
-}
-
-static u32 ixgbe_get_tx_csum(struct net_device *netdev)
-{
-	return (netdev->features & NETIF_F_IP_CSUM) != 0;
-}
-
-static int ixgbe_set_tx_csum(struct net_device *netdev, u32 data)
-{
-	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	u32 feature_list;
-
-	feature_list = (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);
-	switch (adapter->hw.mac.type) {
-	case ixgbe_mac_82599EB:
-	case ixgbe_mac_X540:
-		feature_list |= NETIF_F_SCTP_CSUM;
-		break;
-	default:
-		break;
-	}
-	if (data)
-		netdev->features |= feature_list;
-	else
-		netdev->features &= ~feature_list;
-
-	return 0;
-}
-
-static int ixgbe_set_tso(struct net_device *netdev, u32 data)
-{
-	if (data) {
-		netdev->features |= NETIF_F_TSO;
-		netdev->features |= NETIF_F_TSO6;
-	} else {
-		netdev->features &= ~NETIF_F_TSO;
-		netdev->features &= ~NETIF_F_TSO6;
-	}
-	return 0;
-}
-
 static u32 ixgbe_get_msglevel(struct net_device *netdev)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
@@ -2370,16 +2314,8 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_ringparam          = ixgbe_set_ringparam,
 	.get_pauseparam         = ixgbe_get_pauseparam,
 	.set_pauseparam         = ixgbe_set_pauseparam,
-	.get_rx_csum            = ixgbe_get_rx_csum,
-	.set_rx_csum            = ixgbe_set_rx_csum,
-	.get_tx_csum            = ixgbe_get_tx_csum,
-	.set_tx_csum            = ixgbe_set_tx_csum,
-	.get_sg                 = ethtool_op_get_sg,
-	.set_sg                 = ethtool_op_set_sg,
 	.get_msglevel           = ixgbe_get_msglevel,
 	.set_msglevel           = ixgbe_set_msglevel,
-	.get_tso                = ethtool_op_get_tso,
-	.set_tso                = ixgbe_set_tso,
 	.self_test              = ixgbe_diag_test,
 	.get_strings            = ixgbe_get_strings,
 	.phys_id                = ixgbe_phys_id,
@@ -2387,7 +2323,6 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.get_ethtool_stats      = ixgbe_get_ethtool_stats,
 	.get_coalesce           = ixgbe_get_coalesce,
 	.set_coalesce           = ixgbe_set_coalesce,
-	.get_flags              = ethtool_op_get_flags,
 	.set_flags              = ixgbe_set_flags,
 	.set_rx_ntuple          = ixgbe_set_rx_ntuple,
 };
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index ca9036d..ad0e0ca 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6850,6 +6850,17 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 	return stats;
 }
 
+static int ixgbe_set_features(struct net_device *netdev,
+	unsigned long features)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	if (features & NETIF_F_RXCSUM)
+		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
+	else
+		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
+
+	return 0;
+}
 
 static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_open		= ixgbe_open,
@@ -6880,6 +6891,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_fcoe_disable = ixgbe_fcoe_disable,
 	.ndo_fcoe_get_wwn = ixgbe_fcoe_get_wwn,
 #endif /* IXGBE_FCOE */
+	.ndo_set_features = ixgbe_set_features,
 };
 
 static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
@@ -7144,20 +7156,22 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 
 	ixgbe_probe_vf(adapter, ii);
 
-	netdev->features = NETIF_F_SG |
+	netdev->hw_features = NETIF_F_SG |
 			   NETIF_F_IP_CSUM |
+			   NETIF_F_IPV6_CSUM |
+			   NETIF_F_TSO |
+			   NETIF_F_TSO6 |
+			   NETIF_F_RXCSUM;
+
+	if (adapter->hw.mac.type == ixgbe_mac_82599EB
+	    || adapter->hw.mac.type == ixgbe_mac_X540)
+		netdev->hw_features |= NETIF_F_SCTP_CSUM;
+
+	netdev->features = netdev->hw_features |
 			   NETIF_F_HW_VLAN_TX |
 			   NETIF_F_HW_VLAN_RX |
 			   NETIF_F_HW_VLAN_FILTER;
 
-	netdev->features |= NETIF_F_IPV6_CSUM;
-	netdev->features |= NETIF_F_TSO;
-	netdev->features |= NETIF_F_TSO6;
-	netdev->features |= NETIF_F_GRO;
-
-	if (adapter->hw.mac.type == ixgbe_mac_82599EB)
-		netdev->features |= NETIF_F_SCTP_CSUM;
-
 	netdev->vlan_features |= NETIF_F_TSO;
 	netdev->vlan_features |= NETIF_F_TSO6;
 	netdev->vlan_features |= NETIF_F_IP_CSUM;
diff --git a/drivers/net/ixgbevf/ethtool.c b/drivers/net/ixgbevf/ethtool.c
index fa29b3c..9157eea 100644
--- a/drivers/net/ixgbevf/ethtool.c
+++ b/drivers/net/ixgbevf/ethtool.c
@@ -115,44 +115,6 @@ static int ixgbevf_get_settings(struct net_device *netdev,
 	return 0;
 }
 
-static u32 ixgbevf_get_rx_csum(struct net_device *netdev)
-{
-	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
-	return adapter->flags & IXGBE_FLAG_RX_CSUM_ENABLED;
-}
-
-static int ixgbevf_set_rx_csum(struct net_device *netdev, u32 data)
-{
-	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
-	if (data)
-		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
-	else
-		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
-
-	if (netif_running(netdev)) {
-		if (!adapter->dev_closed)
-			ixgbevf_reinit_locked(adapter);
-	} else {
-		ixgbevf_reset(adapter);
-	}
-
-	return 0;
-}
-
-static int ixgbevf_set_tso(struct net_device *netdev, u32 data)
-{
-	if (data) {
-		netdev->features |= NETIF_F_TSO;
-		netdev->features |= NETIF_F_TSO6;
-	} else {
-		netif_tx_stop_all_queues(netdev);
-		netdev->features &= ~NETIF_F_TSO;
-		netdev->features &= ~NETIF_F_TSO6;
-		netif_tx_start_all_queues(netdev);
-	}
-	return 0;
-}
-
 static u32 ixgbevf_get_msglevel(struct net_device *netdev)
 {
 	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
@@ -718,16 +680,8 @@ static struct ethtool_ops ixgbevf_ethtool_ops = {
 	.get_link               = ethtool_op_get_link,
 	.get_ringparam          = ixgbevf_get_ringparam,
 	.set_ringparam          = ixgbevf_set_ringparam,
-	.get_rx_csum            = ixgbevf_get_rx_csum,
-	.set_rx_csum            = ixgbevf_set_rx_csum,
-	.get_tx_csum            = ethtool_op_get_tx_csum,
-	.set_tx_csum            = ethtool_op_set_tx_ipv6_csum,
-	.get_sg                 = ethtool_op_get_sg,
-	.set_sg                 = ethtool_op_set_sg,
 	.get_msglevel           = ixgbevf_get_msglevel,
 	.set_msglevel           = ixgbevf_set_msglevel,
-	.get_tso                = ethtool_op_get_tso,
-	.set_tso                = ixgbevf_set_tso,
 	.self_test              = ixgbevf_diag_test,
 	.get_sset_count         = ixgbevf_get_sset_count,
 	.get_strings            = ixgbevf_get_strings,
diff --git a/drivers/net/ixgbevf/ixgbevf_main.c b/drivers/net/ixgbevf/ixgbevf_main.c
index 809e38c..091d05b 100644
--- a/drivers/net/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ixgbevf/ixgbevf_main.c
@@ -3252,6 +3252,20 @@ static void ixgbevf_shutdown(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 }
 
+static int ixgbevf_set_features(struct net_device *netdev,
+	unsigned long features)
+{
+	struct ixgbevf_adapter *adapter = netdev_priv(netdev);
+
+	if (features & NETIF_F_RXCSUM)
+		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;
+	else
+		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
+
+	return 0;
+}
+
+
 static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_open		= &ixgbevf_open,
 	.ndo_stop		= &ixgbevf_close,
@@ -3265,6 +3279,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_vlan_rx_register	= &ixgbevf_vlan_rx_register,
 	.ndo_vlan_rx_add_vid	= &ixgbevf_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= &ixgbevf_vlan_rx_kill_vid,
+	.ndo_set_features       = ixgbevf_set_features,
 };
 
 static void ixgbevf_assign_netdev_ops(struct net_device *dev)
@@ -3379,16 +3394,18 @@ static int __devinit ixgbevf_probe(struct pci_dev *pdev,
 	/* setup the private structure */
 	err = ixgbevf_sw_init(adapter);
 
-	netdev->features = NETIF_F_SG |
+	netdev->hw_features = NETIF_F_SG |
 			   NETIF_F_IP_CSUM |
+			   NETIF_F_IPV6_CSUM |
+			   NETIF_F_TSO |
+			   NETIF_F_TSO6 |
+			   NETIF_F_RXCSUM;
+
+	netdev->features = netdev->hw_features |
 			   NETIF_F_HW_VLAN_TX |
 			   NETIF_F_HW_VLAN_RX |
 			   NETIF_F_HW_VLAN_FILTER;
 
-	netdev->features |= NETIF_F_IPV6_CSUM;
-	netdev->features |= NETIF_F_TSO;
-	netdev->features |= NETIF_F_TSO6;
-	netdev->features |= NETIF_F_GRO;
 	netdev->vlan_features |= NETIF_F_TSO;
 	netdev->vlan_features |= NETIF_F_TSO6;
 	netdev->vlan_features |= NETIF_F_IP_CSUM;
-- 
1.7.2.3