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

[Ben Hutchings <bhutchings@solarflare.com>]

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.