Message ID | 1305335142.2851.70.camel@bwh-desktop |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote: > ethtool_set_feature_compat() squashes the feature mask into a boolean, > which is not correct for ethtool_ops::set_flags. > > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually > makes things more complicated for the ethtool utility and any other > application using the ethtool API. They will still need to fall back to > the old offload control commands in order to support older kernel > versions. The fallback code in the kernel adds a third possibility for > them to handle. So make ETHTOOL_SFEATURES fail when the driver > implements the old offload control operations, and let userland do the > fallback. > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> This will disable SFEATURES for drivers which implement changing newer features that have no old ethtool ops (e.g. NETIF_F_LOOPBACK), but are not converted, yet. This might matter when bisecting. It's easy to fix this. The code is going away for 2.6.40, though. Do you want to get rid of ETHTOOL_F_COMPAT bit before 2.6.39? BTW, what are the complications for userspace? This change misses ethtool_get_features_compat() setting available bits. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote: > ethtool_set_feature_compat() squashes the feature mask into a boolean, > which is not correct for ethtool_ops::set_flags. > > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually > makes things more complicated for the ethtool utility and any other > application using the ethtool API. They will still need to fall back to > the old offload control commands in order to support older kernel > versions. The fallback code in the kernel adds a third possibility for > them to handle. So make ETHTOOL_SFEATURES fail when the driver > implements the old offload control operations, and let userland do the > fallback. BTW, the idea behind the compat code is that if ETHTOOL_[GS]FEATURES is available, then there should be no need to fallback to old ops. For a userspace tool that targets only kernels >= 2.6.39 there's no need to care about old ops at all. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Sending this from my home address as solarflare.com mail is under maintenance and SPF prevents me using that address entirely.] On Sat, 2011-05-14 at 11:54 +0200, Michał Mirosław wrote: > On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote: > > ethtool_set_feature_compat() squashes the feature mask into a boolean, > > which is not correct for ethtool_ops::set_flags. > > > > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually > > makes things more complicated for the ethtool utility and any other > > application using the ethtool API. They will still need to fall back to > > the old offload control commands in order to support older kernel > > versions. The fallback code in the kernel adds a third possibility for > > them to handle. So make ETHTOOL_SFEATURES fail when the driver > > implements the old offload control operations, and let userland do the > > fallback. > > > > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> > > This will disable SFEATURES for drivers which implement changing newer > features that have no old ethtool ops (e.g. NETIF_F_LOOPBACK), but are > not converted, yet. This might matter when bisecting. > > It's easy to fix this. Yes, I realise that. > The code is going away for 2.6.40, though. > Do you want to get rid of ETHTOOL_F_COMPAT bit before 2.6.39? Right, I don't want this to ever be in a stable release. > BTW, what are the complications for userspace? With 2.6.38 and earlier, ETHTOOL_SFEATURES will fail; ethtool -K will fall back to the individual operations can can report which of them failed and why. With 2.6.39 and a converted driver, ethtool -K can find out exactly which features are changeable and report whether any of the individual changes are not supported at all. If there's something wrong with the combination of features then ETHTOOL_SFEATURES will return ETHTOOL_F_WISH and it can report that the combination is not supported. With 2.6.39 and an unconverted driver, ETHTOOL_SFEATURES can return ETHTOOL_F_COMPAT which just tells us some change failed, but not why. We can try to guess what went wrong by reading back the features, but it's unclear. This is particularly problematic with set_flags (again) where the ethtool core can't tell which flags are settable in an unconverted driver. I would much prefer not to have this third case existing in a single stable release. > This change misses ethtool_get_features_compat() setting available bits. Yes, but that is less of a problem in practice. Ben.
On Sat, 2011-05-14 at 12:35 +0200, Michał Mirosław wrote: > On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote: > > ethtool_set_feature_compat() squashes the feature mask into a boolean, > > which is not correct for ethtool_ops::set_flags. > > > > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually > > makes things more complicated for the ethtool utility and any other > > application using the ethtool API. They will still need to fall back to > > the old offload control commands in order to support older kernel > > versions. The fallback code in the kernel adds a third possibility for > > them to handle. So make ETHTOOL_SFEATURES fail when the driver > > implements the old offload control operations, and let userland do the > > fallback. > > BTW, the idea behind the compat code is that if ETHTOOL_[GS]FEATURES is > available, then there should be no need to fallback to old ops. For > a userspace tool that targets only kernels >= 2.6.39 there's no need > to care about old ops at all. Well that's not true, because those tools still have to deal with ETHTOOL_F_COMPAT. And we're supposed to have all drivers converted for 2.6.40, so the hypothetical new tool only has to wait one more release. Ben.
On Mon, May 16, 2011 at 03:45:23AM +0100, Ben Hutchings wrote: > On Sat, 2011-05-14 at 12:35 +0200, Michał Mirosław wrote: > > On Sat, May 14, 2011 at 02:05:42AM +0100, Ben Hutchings wrote: > > > ethtool_set_feature_compat() squashes the feature mask into a boolean, > > > which is not correct for ethtool_ops::set_flags. > > > > > > We could fix this, but the fallback code for ETHTOOL_SFEATURES actually > > > makes things more complicated for the ethtool utility and any other > > > application using the ethtool API. They will still need to fall back to > > > the old offload control commands in order to support older kernel > > > versions. The fallback code in the kernel adds a third possibility for > > > them to handle. So make ETHTOOL_SFEATURES fail when the driver > > > implements the old offload control operations, and let userland do the > > > fallback. > > BTW, the idea behind the compat code is that if ETHTOOL_[GS]FEATURES is > > available, then there should be no need to fallback to old ops. For > > a userspace tool that targets only kernels >= 2.6.39 there's no need > > to care about old ops at all. > Well that's not true, because those tools still have to deal with > ETHTOOL_F_COMPAT. And we're supposed to have all drivers converted for > 2.6.40, so the hypothetical new tool only has to wait one more release. I think that handling ETHTOOL_F_COMPAT is like ETHTOOL_F_WISH - the tool needs to reread features and check what was changed. So we can get rid of the former by replacing it with the latter. Changing some feature state may change others that were not requested to be changed (not present in .valid). This probably should be signalled as well. Extend ETHTOOL_F_WISH meaning (I prefer this one) or introduce new bit? Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index dc80d82..5fb916c 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -625,9 +625,6 @@ struct ethtool_sfeatures { * Probably there are other device-specific constraints on some features * in the set. When %ETHTOOL_F_UNSUPPORTED is set, .valid is considered * here as though ignored bits were cleared. - * %ETHTOOL_F_COMPAT - some or all changes requested were made by calling - * compatibility functions. Requested offload state cannot be properly - * managed by kernel. * * Meaning of bits in the masks are obtained by %ETHTOOL_GSSET_INFO (number of * bits in the arrays - always multiple of 32) and %ETHTOOL_GSTRINGS commands @@ -637,12 +634,10 @@ struct ethtool_sfeatures { enum ethtool_sfeatures_retval_bits { ETHTOOL_F_UNSUPPORTED__BIT, ETHTOOL_F_WISH__BIT, - ETHTOOL_F_COMPAT__BIT, }; #define ETHTOOL_F_UNSUPPORTED (1 << ETHTOOL_F_UNSUPPORTED__BIT) #define ETHTOOL_F_WISH (1 << ETHTOOL_F_WISH__BIT) -#define ETHTOOL_F_COMPAT (1 << ETHTOOL_F_COMPAT__BIT) #ifdef __KERNEL__ diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 74ead9e..5f58f1f 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -207,52 +207,6 @@ static void ethtool_get_features_compat(struct net_device *dev, features[0].available |= flags_dup_features; } -static int ethtool_set_feature_compat(struct net_device *dev, - int (*legacy_set)(struct net_device *, u32), - struct ethtool_set_features_block *features, u32 mask) -{ - u32 do_set; - - if (!legacy_set) - return 0; - - if (!(features[0].valid & mask)) - return 0; - - features[0].valid &= ~mask; - - do_set = !!(features[0].requested & mask); - - if (legacy_set(dev, do_set) < 0) - netdev_info(dev, - "Legacy feature change (%s) failed for 0x%08x\n", - do_set ? "set" : "clear", mask); - - return 1; -} - -static int ethtool_set_features_compat(struct net_device *dev, - struct ethtool_set_features_block *features) -{ - int compat; - - if (!dev->ethtool_ops) - return 0; - - compat = ethtool_set_feature_compat(dev, dev->ethtool_ops->set_sg, - features, NETIF_F_SG); - compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_tx_csum, - features, NETIF_F_ALL_CSUM); - compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_tso, - features, NETIF_F_ALL_TSO); - compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_rx_csum, - features, NETIF_F_RXCSUM); - compat |= ethtool_set_feature_compat(dev, dev->ethtool_ops->set_flags, - features, flags_dup_features); - - return compat; -} - static int ethtool_get_features(struct net_device *dev, void __user *useraddr) { struct ethtool_gfeatures cmd = { @@ -307,8 +261,13 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr) if (features[0].valid & ~NETIF_F_ETHTOOL_BITS) return -EINVAL; - if (ethtool_set_features_compat(dev, features)) - ret |= ETHTOOL_F_COMPAT; + /* If the driver implements any of the old operations, don't + * update features directly. + */ + if (dev->ethtool_ops->set_sg || dev->ethtool_ops->set_tx_csum || + dev->ethtool_ops->set_tso || dev->ethtool_ops->set_rx_csum || + dev->ethtool_ops->set_flags) + return -EOPNOTSUPP; if (features[0].valid & ~dev->hw_features) { features[0].valid &= dev->hw_features;
ethtool_set_feature_compat() squashes the feature mask into a boolean, which is not correct for ethtool_ops::set_flags. We could fix this, but the fallback code for ETHTOOL_SFEATURES actually makes things more complicated for the ethtool utility and any other application using the ethtool API. They will still need to fall back to the old offload control commands in order to support older kernel versions. The fallback code in the kernel adds a third possibility for them to handle. So make ETHTOOL_SFEATURES fail when the driver implements the old offload control operations, and let userland do the fallback. Signed-off-by: Ben Hutchings <bhutchings@solarflare.com> --- include/linux/ethtool.h | 5 ---- net/core/ethtool.c | 55 ++++++----------------------------------------- 2 files changed, 7 insertions(+), 53 deletions(-)