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

[Ben Hutchings <bhutchings@solarflare.com>]

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.