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

[Ben Hutchings <bhutchings@solarflare.com>]

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.