diff mbox

[3/4] ethtool: can't set combined and tx/rx channel counts at the same time

Message ID 1454961965-15116-4-git-send-email-jacob.e.keller@intel.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Keller, Jacob E Feb. 8, 2016, 8:06 p.m. UTC 
Based on reading current implementations of {GS}CHANNELS, most drivers
either support only combined counts or only separate counts. At least
one driver supported setting combined or separate channels. No driver
supported combined and separate channels setting at the same time, and
this patch adds a sanity check to prevent such requests. I am not 100%
sure if this is correct as I was not able to find documentation and
I think it currently depended on driver implementation.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 net/core/ethtool.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

kernel test robot Feb. 8, 2016, 8:25 p.m. UTC | #1 
Hi Jacob,

[auto build test ERROR on net/master]
[also build test ERROR on v4.5-rc3 next-20160208]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jacob-Keller/ethtool-correctly-ensure-GS-CHANNELS-doesn-t-conflict-with-GS-RXFH/20160209-041131
config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   net/core/ethtool.c: In function 'ethtool_set_channels':
   net/core/ethtool.c:1289:6: error: void value not ignored as it ought to be
     ret = dev->ethtool_ops->get_channels(dev, &max);
         ^
   net/core/ethtool.c:1303:6: error: expected ')' before 'return'
         return -EINVAL;
         ^
>> net/core/ethtool.c:2159:1: error: expected declaration or statement at end of input
    }
    ^
   net/core/ethtool.c:1280:6: warning: unused variable 'max_rx' [-Wunused-variable]
     u32 max_rx = 0;
         ^
   net/core/ethtool.c: At top level:
   net/core/ethtool.c:116:12: warning: 'ethtool_get_features' defined but not used [-Wunused-function]
    static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:154:12: warning: 'ethtool_set_features' defined but not used [-Wunused-function]
    static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:237:13: warning: '__ethtool_get_strings' defined but not used [-Wunused-function]
    static void __ethtool_get_strings(struct net_device *dev,
                ^
   net/core/ethtool.c:296:12: warning: 'ethtool_get_one_feature' defined but not used [-Wunused-function]
    static int ethtool_get_one_feature(struct net_device *dev,
               ^
   net/core/ethtool.c:310:12: warning: 'ethtool_set_one_feature' defined but not used [-Wunused-function]
    static int ethtool_set_one_feature(struct net_device *dev,
               ^
   net/core/ethtool.c:340:12: warning: '__ethtool_get_flags' defined but not used [-Wunused-function]
    static u32 __ethtool_get_flags(struct net_device *dev)
               ^
   net/core/ethtool.c:358:12: warning: '__ethtool_set_flags' defined but not used [-Wunused-function]
    static int __ethtool_set_flags(struct net_device *dev, u32 data)
               ^
   net/core/ethtool.c:402:12: warning: 'ethtool_get_settings' defined but not used [-Wunused-function]
    static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:416:12: warning: 'ethtool_set_settings' defined but not used [-Wunused-function]
    static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:429:31: warning: 'ethtool_get_drvinfo' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
                                  ^
   net/core/ethtool.c:475:31: warning: 'ethtool_get_sset_info' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_sset_info(struct net_device *dev,
                                  ^
   net/core/ethtool.c:531:31: warning: 'ethtool_set_rxnfc' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
                                  ^
   net/core/ethtool.c:563:31: warning: 'ethtool_get_rxnfc' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
                                  ^
   net/core/ethtool.c:678:31: warning: 'ethtool_get_rxfh_indir' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev,
                                  ^
   net/core/ethtool.c:726:31: warning: 'ethtool_set_rxfh_indir' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_rxfh_indir(struct net_device *dev,
                                  ^
   net/core/ethtool.c:788:31: warning: 'ethtool_get_rxfh' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
                                  ^
   net/core/ethtool.c:860:31: warning: 'ethtool_set_rxfh' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
                                  ^
   net/core/ethtool.c:955:12: warning: 'ethtool_get_regs' defined but not used [-Wunused-function]
    static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:991:12: warning: 'ethtool_reset' defined but not used [-Wunused-function]
    static int ethtool_reset(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1011:12: warning: 'ethtool_get_wol' defined but not used [-Wunused-function]
    static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1025:12: warning: 'ethtool_set_wol' defined but not used [-Wunused-function]
    static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1038:12: warning: 'ethtool_get_eee' defined but not used [-Wunused-function]
    static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1059:12: warning: 'ethtool_set_eee' defined but not used [-Wunused-function]
    static int ethtool_set_eee(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1072:12: warning: 'ethtool_nway_reset' defined but not used [-Wunused-function]
    static int ethtool_nway_reset(struct net_device *dev)
               ^
   net/core/ethtool.c:1080:12: warning: 'ethtool_get_link' defined but not used [-Wunused-function]
    static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1145:12: warning: 'ethtool_get_eeprom' defined but not used [-Wunused-function]
    static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1157:12: warning: 'ethtool_set_eeprom' defined but not used [-Wunused-function]
    static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1205:31: warning: 'ethtool_get_coalesce' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
                                  ^
   net/core/ethtool.c:1220:31: warning: 'ethtool_set_coalesce' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
                                  ^
   net/core/ethtool.c:1234:12: warning: 'ethtool_get_ringparam' defined but not used [-Wunused-function]
    static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1248:12: warning: 'ethtool_set_ringparam' defined but not used [-Wunused-function]
    static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1261:31: warning: 'ethtool_get_channels' defined but not used [-Wunused-function]

vim +2159 net/core/ethtool.c

^1da177e Linus Torvalds    2005-04-16  2153  		dev->ethtool_ops->complete(dev);
d8a33ac4 Stephen Hemminger 2005-05-29  2154  
d8a33ac4 Stephen Hemminger 2005-05-29  2155  	if (old_features != dev->features)
d8a33ac4 Stephen Hemminger 2005-05-29  2156  		netdev_features_change(dev);
d8a33ac4 Stephen Hemminger 2005-05-29  2157  
^1da177e Linus Torvalds    2005-04-16  2158  	return rc;
^1da177e Linus Torvalds    2005-04-16 @2159  }

:::::: The code at line 2159 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot Feb. 8, 2016, 8:30 p.m. UTC | #2 
Hi Jacob,

[auto build test WARNING on net/master]
[also build test WARNING on v4.5-rc3 next-20160208]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jacob-Keller/ethtool-correctly-ensure-GS-CHANNELS-doesn-t-conflict-with-GS-RXFH/20160209-041131
config: x86_64-randconfig-x012-201606 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   net/core/ethtool.c: In function 'ethtool_set_channels':
   net/core/ethtool.c:1289:6: error: void value not ignored as it ought to be
     ret = dev->ethtool_ops->get_channels(dev, &max);
         ^
   net/core/ethtool.c:1303:6: error: expected ')' before 'return'
         return -EINVAL;
         ^
   net/core/ethtool.c:2159:1: error: expected declaration or statement at end of input
    }
    ^
>> net/core/ethtool.c:1280:6: warning: unused variable 'max_rx' [-Wunused-variable]
     u32 max_rx = 0;
         ^
   net/core/ethtool.c: At top level:
   net/core/ethtool.c:116:12: warning: 'ethtool_get_features' defined but not used [-Wunused-function]
    static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:154:12: warning: 'ethtool_set_features' defined but not used [-Wunused-function]
    static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:237:13: warning: '__ethtool_get_strings' defined but not used [-Wunused-function]
    static void __ethtool_get_strings(struct net_device *dev,
                ^
   net/core/ethtool.c:296:12: warning: 'ethtool_get_one_feature' defined but not used [-Wunused-function]
    static int ethtool_get_one_feature(struct net_device *dev,
               ^
   net/core/ethtool.c:310:12: warning: 'ethtool_set_one_feature' defined but not used [-Wunused-function]
    static int ethtool_set_one_feature(struct net_device *dev,
               ^
   net/core/ethtool.c:340:12: warning: '__ethtool_get_flags' defined but not used [-Wunused-function]
    static u32 __ethtool_get_flags(struct net_device *dev)
               ^
   net/core/ethtool.c:358:12: warning: '__ethtool_set_flags' defined but not used [-Wunused-function]
    static int __ethtool_set_flags(struct net_device *dev, u32 data)
               ^
   net/core/ethtool.c:402:12: warning: 'ethtool_get_settings' defined but not used [-Wunused-function]
    static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:416:12: warning: 'ethtool_set_settings' defined but not used [-Wunused-function]
    static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:429:31: warning: 'ethtool_get_drvinfo' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
                                  ^
   net/core/ethtool.c:475:31: warning: 'ethtool_get_sset_info' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_sset_info(struct net_device *dev,
                                  ^
   net/core/ethtool.c:531:31: warning: 'ethtool_set_rxnfc' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
                                  ^
   net/core/ethtool.c:563:31: warning: 'ethtool_get_rxnfc' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev,
                                  ^
   net/core/ethtool.c:678:31: warning: 'ethtool_get_rxfh_indir' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev,
                                  ^
   net/core/ethtool.c:726:31: warning: 'ethtool_set_rxfh_indir' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_rxfh_indir(struct net_device *dev,
                                  ^
   net/core/ethtool.c:788:31: warning: 'ethtool_get_rxfh' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
                                  ^
   net/core/ethtool.c:860:31: warning: 'ethtool_set_rxfh' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
                                  ^
   net/core/ethtool.c:955:12: warning: 'ethtool_get_regs' defined but not used [-Wunused-function]
    static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:991:12: warning: 'ethtool_reset' defined but not used [-Wunused-function]
    static int ethtool_reset(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1011:12: warning: 'ethtool_get_wol' defined but not used [-Wunused-function]
    static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1025:12: warning: 'ethtool_set_wol' defined but not used [-Wunused-function]
    static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1038:12: warning: 'ethtool_get_eee' defined but not used [-Wunused-function]
    static int ethtool_get_eee(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1059:12: warning: 'ethtool_set_eee' defined but not used [-Wunused-function]
    static int ethtool_set_eee(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1072:12: warning: 'ethtool_nway_reset' defined but not used [-Wunused-function]
    static int ethtool_nway_reset(struct net_device *dev)
               ^
   net/core/ethtool.c:1080:12: warning: 'ethtool_get_link' defined but not used [-Wunused-function]
    static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
               ^
   net/core/ethtool.c:1145:12: warning: 'ethtool_get_eeprom' defined but not used [-Wunused-function]
    static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1157:12: warning: 'ethtool_set_eeprom' defined but not used [-Wunused-function]
    static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1205:31: warning: 'ethtool_get_coalesce' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
                                  ^
   net/core/ethtool.c:1220:31: warning: 'ethtool_set_coalesce' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
                                  ^
   net/core/ethtool.c:1234:12: warning: 'ethtool_get_ringparam' defined but not used [-Wunused-function]
    static int ethtool_get_ringparam(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1248:12: warning: 'ethtool_set_ringparam' defined but not used [-Wunused-function]
    static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
               ^
   net/core/ethtool.c:1261:31: warning: 'ethtool_get_channels' defined but not used [-Wunused-function]
    static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
                                  ^
   net/core/ethtool.c:1276:31: warning: 'ethtool_set_channels' defined but not used [-Wunused-function]

vim +/max_rx +1280 net/core/ethtool.c

8b5933c38 amit salecha 2011-04-07  1274  }
8b5933c38 amit salecha 2011-04-07  1275  
8b5933c38 amit salecha 2011-04-07  1276  static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
8b5933c38 amit salecha 2011-04-07  1277  						   void __user *useraddr)
8b5933c38 amit salecha 2011-04-07  1278  {
214881553 Jacob Keller 2016-02-08  1279  	struct ethtool_channels channels, max;
bb15c47a7 Jacob Keller 2016-02-08 @1280  	u32 max_rx = 0;
bb15c47a7 Jacob Keller 2016-02-08  1281  	int ret;
8b5933c38 amit salecha 2011-04-07  1282  
214881553 Jacob Keller 2016-02-08  1283  	if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
8b5933c38 amit salecha 2011-04-07  1284  		return -EOPNOTSUPP;
8b5933c38 amit salecha 2011-04-07  1285  
8b5933c38 amit salecha 2011-04-07  1286  	if (copy_from_user(&channels, useraddr, sizeof(channels)))
8b5933c38 amit salecha 2011-04-07  1287  		return -EFAULT;
8b5933c38 amit salecha 2011-04-07  1288  
214881553 Jacob Keller 2016-02-08 @1289  	ret = dev->ethtool_ops->get_channels(dev, &max);
214881553 Jacob Keller 2016-02-08  1290  	if (ret)
214881553 Jacob Keller 2016-02-08  1291  		return ret;
214881553 Jacob Keller 2016-02-08  1292  

:::::: The code at line 1280 was first introduced by commit
:::::: bb15c47a78c2d9aab399f5dae676b03fefda79ca ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

:::::: TO: Jacob Keller <jacob.e.keller@intel.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Jakub Kiciński Feb. 8, 2016, 8:52 p.m. UTC | #3 
On Mon,  8 Feb 2016 12:06:04 -0800, Jacob Keller wrote:
> +	/* can't set combined and separate channels at the same time */
> +	if ((channels.combined_count &&
> +	     (channels.rx_count || channels.tx_count))
> +	    return -EINVAL;
> +
>  	/* ensure the new Rx count fits within the configured Rx flow
>  	 * indirection table settings */
>  	if (netif_is_rxfh_configured(dev) &&

My understanding is that unsymmetrical rx/tx queue configuration with
combined IRQ vectors should be expressed as:

combined = min(tx, rx);
num_tx = tx - combined;
num_rx = rx - combined;

Please see:

https://www.mail-archive.com/netdev@vger.kernel.org/msg93977.html

Patches which I cooked up for this are rotting in my queue blocked by
other things, unfortunately.
Keller, Jacob E Feb. 8, 2016, 10 p.m. UTC | #4 
On Mon, 2016-02-08 at 20:52 +0000, Jakub Kicinski wrote:
> On Mon,  8 Feb 2016 12:06:04 -0800, Jacob Keller wrote:

> > +	/* can't set combined and separate channels at the same

> > time */

> > +	if ((channels.combined_count &&

> > +	     (channels.rx_count || channels.tx_count))

> > +	    return -EINVAL;

> > +

> >  	/* ensure the new Rx count fits within the configured Rx

> > flow

> >  	 * indirection table settings */

> >  	if (netif_is_rxfh_configured(dev) &&

> 

> My understanding is that unsymmetrical rx/tx queue configuration with

> combined IRQ vectors should be expressed as:

> 

> combined = min(tx, rx);

> num_tx = tx - combined;

> num_rx = rx - combined;

> 

> Please see:

> 

> https://www.mail-archive.com/netdev@vger.kernel.org/msg93977.html

> 

> Patches which I cooked up for this are rotting in my queue blocked by

> other things, unfortunately.


That's definitely not how any driver currently uses it today, that I
can see... At least drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
is currently assuming you can't set combined and asymmetric queues at
the same time.

I can ok dropping this change, and looking at implementing what you
suggest, as that makes more sense to me.

Regards,
Jake
Keller, Jacob E Feb. 8, 2016, 10:15 p.m. UTC | #5 
On Mon, 2016-02-08 at 20:52 +0000, Jakub Kicinski wrote:
> On Mon,  8 Feb 2016 12:06:04 -0800, Jacob Keller wrote:

> > +	/* can't set combined and separate channels at the same

> > time */

> > +	if ((channels.combined_count &&

> > +	     (channels.rx_count || channels.tx_count))

> > +	    return -EINVAL;

> > +

> >  	/* ensure the new Rx count fits within the configured Rx

> > flow

> >  	 * indirection table settings */

> >  	if (netif_is_rxfh_configured(dev) &&

> 

> My understanding is that unsymmetrical rx/tx queue configuration with

> combined IRQ vectors should be expressed as:

> 

> combined = min(tx, rx);

> num_tx = tx - combined;

> num_rx = rx - combined;

> 

> Please see:

> 

> https://www.mail-archive.com/netdev@vger.kernel.org/msg93977.html

> 

> Patches which I cooked up for this are rotting in my queue blocked by

> other things, unfortunately.



It looks like there was no follow up from your previous comment?

I think this looks right, in which case my patch should be dropped. I
do not think that .max_rx and .max_tx include the total for both Tx and
Rx maximums, though, so I am not sure best how to express the
possibility.

For now, I'll just drop this patch from the series.

Regards,
Jake
diff mbox

Patch

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 71aff99157c3..7d4cef7b7176 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1297,6 +1297,11 @@  static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 	    (channels.other_count > max.max_other))
 		return -EINVAL;
 
+	/* can't set combined and separate channels at the same time */
+	if ((channels.combined_count &&
+	     (channels.rx_count || channels.tx_count))
+	    return -EINVAL;
+
 	/* ensure the new Rx count fits within the configured Rx flow
 	 * indirection table settings */
 	if (netif_is_rxfh_configured(dev) &&