Date:	Sun, 19 Dec 2010 01:49:59 +0100
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

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
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists