Re: [PATCH v3 1/5] net: Introduce new feature setting ops

[Ben Hutchings <bhutchings@solarflare.com>]

On Sat, 2011-01-29 at 19:39 +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
> 	+ ETHTOOL_GSTRINGS(ETH_SS_FEATURES): get feature bits names (meaning)
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  include/linux/ethtool.h   |   86 +++++++++++++++++++++++++
>  include/linux/netdevice.h |   44 ++++++++++++-
>  net/core/dev.c            |   47 ++++++++++++--
>  net/core/ethtool.c        |  154 +++++++++++++++++++++++++++++++++++++++++----
>  4 files changed, 312 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 1908929..b832083 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -251,6 +251,7 @@ enum ethtool_stringset {
>  	ETH_SS_STATS,
>  	ETH_SS_PRIV_FLAGS,
>  	ETH_SS_NTUPLE_FILTERS,
> +	ETH_SS_FEATURES,
>  };
>  
>  /* for passing string sets for data tagging */
> @@ -523,6 +524,88 @@ struct ethtool_flash {
>  	char	data[ETHTOOL_FLASH_MAX_FILENAME];
>  };
>  
> +/* for returning and changing feature sets */
> +
> +/**
> + * struct ethtool_get_features_block - block with state of 32 features
> + * @avaliable: mask of changeable features

Typo, should be @available.

> + * @requested: mask of features requested to be enabled if possible
> + * @active: mask of currently enabled features
> + * @never_changed: mask of never-changeable features

I don't think the description of never_changed is clear enough.  It
should be described as something like:

    Mask of feature flags that are not changeable for any device.

> + */
> +struct ethtool_get_features_block {
> +	__u32	available;	/* features togglable */
> +	__u32	requested;	/* features requested to be enabled */
> +	__u32	active;		/* features currently enabled */
> +	__u32	never_changed;	/* never-changeable features */

Don't comment the fields inline as well as in the kernel-doc.

> +};
> +
> +/**
> + * struct ethtool_gfeatures - command to get state of device's features
> + * @cmd: command number = %ETHTOOL_GFEATURES
> + * @size: in: array size of the features[] array
> + *       out: count of features[] elements filled

The value on output should be the size required to read all features, so
that the caller can discover it easily.

The two lines describing 'size' will be wrapped together when converted
to another format (manual page, HTML...).  You need to use at least a
full stop (period) to separate them.

[...]
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index c7d7074..6490860 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -783,6 +783,16 @@ struct netdev_tc_txq {
>   *	Set hardware filter for RFS.  rxq_index is the target queue index;
>   *	flow_id is a flow ID to be passed to rps_may_expire_flow() later.
>   *	Return the filter ID on success, or a negative error code.
> + *
> + * u32 (*ndo_fix_features)(struct net_device *dev, u32 features);
> + *	Modifies features supported by device depending on device-specific
> + *	constraints. Should not modify hardware state.

I don't think this wording is clear enough.  How about:

    Adjusts the requested feature flags according to device-specific
    constraints, and returns the resulting flags.  Must not modify
    the device state.

[...]
> @@ -954,6 +974,12 @@ struct net_device {
>  #define NETIF_F_TSO6		(SKB_GSO_TCPV6 << NETIF_F_GSO_SHIFT)
>  #define NETIF_F_FSO		(SKB_GSO_FCOE << NETIF_F_GSO_SHIFT)
>  
> +	/* Features valid for ethtool to change */
> +	/* = all defined minus driver/device-class-related */
> +#define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
> +				  NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)

Shouldn't NETIF_F_NO_CSUM and NETIF_F_HIGHDMA be included in this?  (And
excluded from NETIF_F_ALL_TX_OFFLOADS.)

> +#define NETIF_F_ETHTOOL_BITS	(0x1f3fffff & ~NETIF_F_NEVER_CHANGE)
> +
>  	/* List of features with software fallbacks. */
>  #define NETIF_F_GSO_SOFTWARE	(NETIF_F_TSO | NETIF_F_TSO_ECN | \
>  				 NETIF_F_TSO6 | NETIF_F_UFO)
> @@ -964,6 +990,12 @@ 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)
> +
> +#define NETIF_F_ALL_TX_OFFLOADS	(NETIF_F_ALL_CSUM | NETIF_F_SG | \
> +				 NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \
> +				 NETIF_F_SCTP_CSUM | NETIF_F_FCOE_CRC)
> +
>  	/*
>  	 * 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 ddd5df2..0ccbee6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
[...]
> @@ -5267,6 +5273,35 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
>  }
>  EXPORT_SYMBOL(netdev_fix_features);
>  
> +void netdev_update_features(struct net_device *dev)
> +{
> +	u32 features;
> +	int err = 0;
> +
> +	features = netdev_get_wanted_features(dev);
> +
> +	if (dev->netdev_ops->ndo_fix_features)
> +		features = dev->netdev_ops->ndo_fix_features(dev, features);
> +
> +	/* driver might be less strict about feature dependencies */
> +	features = netdev_fix_features(dev, features);
> +
> +	if (dev->features == features)
> +		return;
> +
> +	netdev_info(dev, "Features changed: 0x%08x -> 0x%08x\n",
> +		dev->features, features);
> +
> +	if (dev->netdev_ops->ndo_set_features)
> +		err = dev->netdev_ops->ndo_set_features(dev, features);
> +
> +	if (!err)
> +		dev->features = features;
> +	else if (err < 0)
> +		netdev_err(dev, "set_features() failed (%d)\n", err);

The error message should include the feature flags passed, since the
previous informational message may be filtered out.

> +}
> +EXPORT_SYMBOL(netdev_update_features);
> +
>  /**
>   *	netif_stacked_transfer_operstate -	transfer operstate
>   *	@rootdev: the root or lower level device to transfer state from
[...]
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 5984ee0..1420edd 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)
>  {
> @@ -171,6 +172,136 @@ EXPORT_SYMBOL(ethtool_ntuple_flush);
>  
>  /* Handlers for each ethtool command */
>  
> +#define ETHTOOL_DEV_FEATURE_WORDS	1
> +
> +static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_gfeatures cmd = {
> +		.cmd = ETHTOOL_GFEATURES,
> +		.size = 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,
> +			.never_changed = NETIF_F_NEVER_CHANGE,
> +		},
> +	};
> +	u32 __user *sizeaddr;
> +	u32 in_size;
> +
> +	sizeaddr = useraddr + offsetof(struct ethtool_gfeatures, size);
> +	if (get_user(in_size, sizeaddr))
> +		return -EFAULT;
> +
> +	if (in_size < ETHTOOL_DEV_FEATURE_WORDS)
> +		return -EINVAL;

I don't think this should be considered invalid.  Instead:

	u32 copy_size;
	...
	copy_size = min_t(u32, in_size, ETHTOOL_DEV_FEATURE_WORDS);

> +	if (copy_to_user(useraddr, &cmd, sizeof(cmd)))
> +		return -EFAULT;
> +	useraddr += sizeof(cmd);
> +	if (copy_to_user(useraddr, features, sizeof(features)))

and:

	if (copy_to_user(useraddr, features, copy_size * sizeof(features[0]))

[...]
> +static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GSTRING_LEN] = {
> +	/* NETIF_F_SG */              "scatter-gather",

SG really means TX DMA gather only, as the driver is responsible for
allocating its own RX buffers.

> +	/* NETIF_F_IP_CSUM */         "tx-checksum-hw-ipv4",
> +	/* NETIF_F_NO_CSUM */         "tx-checksum-local",
> +	/* NETIF_F_HW_CSUM */         "tx-checksum-hw-ip-generic",
> +	/* NETIF_F_IPV6_CSUM */       "tx_checksum-hw-ipv6",
> +	/* NETIF_F_HIGHDMA */         "highdma",
> +	/* NETIF_F_FRAGLIST */        "scatter-gather-fraglist",
> +	/* NETIF_F_HW_VLAN_TX */      "tx-vlan-hw",
> +
> +	/* NETIF_F_HW_VLAN_RX */      "rx-vlan-hw",
> +	/* NETIF_F_HW_VLAN_FILTER */  "rx-vlan-filter",
> +	/* NETIF_F_VLAN_CHALLENGED */ "*vlan-challenged",

Don't mark the unchangeable features specially here; that can be done by
userland.  Actually, I wonder whether they really need descriptions at
all.

> +	/* NETIF_F_GSO */             "generic-segmentation-offload",
> +	/* NETIF_F_LLTX */            "*lockless-tx",
> +	/* NETIF_F_NETNS_LOCAL */     "*netns-local",
> +	/* NETIF_F_GRO */             "generic-receive-offload",
> +	/* NETIF_F_LRO */             "large-receive-offload",
> +
> +	/* NETIF_F_TSO */             "tcp-segmentation-offload",
> +	/* NETIF_F_UFO */             "udp-fragmentation-offload",
> +	/* NETIF_F_GSO_ROBUST */      "gso-robust",
> +	/* NETIF_F_TSO_ECN */         "tcp-ecn-segmentation-offload",
> +	/* NETIF_F_TSO6 */            "ipv6-tcp-segmentation-offload",
> +	/* NETIF_F_FSO */             "fcoe-segmentation-offload",
> +	"",
> +	"",
> +
> +	/* NETIF_F_FCOE_CRC */        "tx-checksum-fcoe-crc",
> +	/* NETIF_F_SCTP_CSUM */       "tx-checksum-sctp",
> +	/* NETIF_F_FCOE_MTU */        "fcoe-mtu",
> +	/* NETIF_F_NTUPLE */          "ntuple-filter",
[...]

I think this should be named 'rx-ntuple-filter'.  TX filtering may be
controlled for related devices (VFs) and is completely separate from
this.

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.