[PATCH 0/3] ethtool: correct {GS}CHANNELS and {GS}RXFH conflict

[PATCH 0/3] ethtool: correct {GS}CHANNELS and {GS}RXFH conflict

[Jacob Keller]

This patch series fixes up ethtool_set_channels operation which
allowed modifying the RXFH table indirectly by reducing the number of
queues below the current max queue used by the Rx flow table. Most
drivers incorrectly allowed this to destroy the Rx flow table and
would then start by reinitializing it to default settings. However,
drivers are not able to correctly handle the conflict since there was
no way to differentiate between the default settings and the user
requested explicit settings.

To fix this, implement a new netdev private flag which we use to
indicate whether the RXFH has been user configured. If someone has
a better alternative of how to store this information, let me know.
I do not think that priv_flags is the best solution but I have not had
any better idea.

Secondly, we add a function which just calls the driver's get_rxfh
callback to determine the current indirection table. Loop through this
and we can determine the current highest queue that will be used by
RSS.

Now, modify ethtool_set_channels to add a check ensuring that if (a)
we have had rxfh configured by user, (b) we can get the maximum RSS
queue currently used, then we ensure that the newly requested Rx count
(or combined count) is at least as high as this maximum RSS queue. The
reasoning here is that we can always safely increase the number of
queues. If we decrease the queues we must ensure that the decrease
does not go lower than the highest in-use queue for the Rx flow table.

Drivers may still need to be patched if they currently overwrite the
Rx flow table during channel configuration. If the driver currently
always resets Rx flow table when increasing number of queues it must
be patched to only do this when netif_is_rxfh_configured returns
false.

The second two patches are suggestions which I think are valid and
noticed while implementing the above changes. The second patch simply
adds a check to ensure that all provided channel counts fit within
driver defined maximums.

The third patch I am unsure on so would like comment. I believe that
combined queue counts and separate tx/rx queue counts should not be
configurable at the same time (the only driver which allowed both does
requires they can't be defined at the same time). Other drivers
support either separate or combined but not both. If this isn't the
case then go ahead and ignore the third patch.

The fourth patch fixes fm10k to correctly recofigure the RSS reta
table whenever it is still unconfigured. This means that the default
state will provide RSS to every queue. Once the user has configured
RXFH, then we should maintain it. In addition, since the case where we
must reconfigure the RSS table in this case should now no longer
occur, add a dev_err message to indicate the user that we did so.

Jacob Keller (4):
  ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}
  ethtool: ensure channel counts are within bounds during SCHANNELS
  ethtool: can't set combined and tx/rx channel counts at the same time
  fm10k: don't reinitialize RSS flow table when RXFH configured

 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 10 +++-
 include/linux/netdevice.h                     |  8 +++
 net/core/ethtool.c                            | 79 ++++++++++++++++++++++++++-
 3 files changed, 93 insertions(+), 4 deletions(-)

-- 
2.7.0.236.gda096a0.dirty

[PATCH 1/4] ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

[Jacob Keller]

Ethernet drivers implementing both {GS}RXFH and {GS}CHANNELS ethtool ops
incorrectly allow SCHANNELS when it would conflict with the settings
from SRXFH. This occurs because it is not possible for drivers to
understand whether their Rx flow indirection table has been configured
or is in the default state. In addition, drivers currently behave in
various ways when increasing the number of Rx channels.

Some drivers will always destroy the Rx flow indirection table when this
occurs, whether it has been set by the user or not. Other drivers will
attempt to preserve the table even if the user has never modified it
from the default driver settings. Neither of these situation is
desirable because it leads to unexpected behavior or loss of user
configuration.

The correct behavior is to simply return -EINVAL when SCHANNELS would
conflict with the current Rx flow table settings. However, it should
only do so if the current settings were modified by the user. If we
required that the new settings never conflict with the current (default)
Rx flow settings, we would force users to first reduce their Rx flow
settings and then reduce the number of Rx channels.

This patch proposes a solution implemented in net/core/ethtool.c which
ensures that all drivers behave correctly. It checks whether the RXFH
table has been configured to non-default settings, and stores this
information in a private netdev flag. When the number of channels is
requested to change, it first ensures that the current Rx flow table is
not going to assign flows to now disabled channels.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 include/linux/netdevice.h |  8 +++++++
 net/core/ethtool.c        | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 289c2314d766..044fa2d88c7f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1291,6 +1291,7 @@ struct net_device_ops {
  * @IFF_OPENVSWITCH: device is a Open vSwitch master
  * @IFF_L3MDEV_SLAVE: device is enslaved to an L3 master device
  * @IFF_TEAM: device is a team device
+ * @IFF_RXFH_CONFIGURED: device has had Rx Flow indirection table configured
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1318,6 +1319,7 @@ enum netdev_priv_flags {
 	IFF_OPENVSWITCH			= 1<<22,
 	IFF_L3MDEV_SLAVE		= 1<<23,
 	IFF_TEAM			= 1<<24,
+	IFF_RXFH_CONFIGURED		= 1<<25,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -1345,6 +1347,7 @@ enum netdev_priv_flags {
 #define IFF_OPENVSWITCH			IFF_OPENVSWITCH
 #define IFF_L3MDEV_SLAVE		IFF_L3MDEV_SLAVE
 #define IFF_TEAM			IFF_TEAM
+#define IFF_RXFH_CONFIGURED		IFF_RXFH_CONFIGURED
 
 /**
  *	struct net_device - The DEVICE structure.
@@ -4045,6 +4048,11 @@ static inline bool netif_is_lag_port(const struct net_device *dev)
 	return netif_is_bond_slave(dev) || netif_is_team_port(dev);
 }
 
+static inline bool netif_is_rxfh_configured(const struct net_device *dev)
+{
+	return dev->priv_flags & IFF_RXFH_CONFIGURED;
+}
+
 /* This device needs to keep skb dst for qdisc enqueue or ndo_start_xmit() */
 static inline void netif_keep_dst(struct net_device *dev)
 {
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index daf04709dd3c..8a40948c1fc6 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -642,6 +642,39 @@ void netdev_rss_key_fill(void *buffer, size_t len)
 }
 EXPORT_SYMBOL(netdev_rss_key_fill);
 
+static inline int ethool_get_max_rxfh_channel(struct net_device *dev, u32 *max)
+{
+	u32 dev_size, current_max = 0;
+	u32 *indir;
+	int ret, i;
+
+	if (!dev->ethtool_ops->get_rxfh_indir_size ||
+	    !dev->ethtool_ops->get_rxfh)
+		return -EOPNOTSUPP;
+	dev_size = dev->ethtool_ops->get_rxfh_indir_size(dev);
+	if (dev_size == 0)
+		return -EOPNOTSUPP;
+
+	indir = kcalloc(dev_size, sizeof(indir[0]), GFP_USER);
+	if (!indir)
+		return -ENOMEM;
+
+	ret = dev->ethtool_ops->get_rxfh(dev, indir, NULL, NULL);
+	if (ret)
+		goto out;
+
+	for (i = dev_size; i--;) {
+		if (indir[i] > current_max)
+			current_max = indir[i];
+	}
+
+	*max = current_max;
+
+out:
+	kfree(indir);
+	return ret;
+}
+
 static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev,
 						     void __user *useraddr)
 {
@@ -738,6 +771,14 @@ static noinline_for_stack int ethtool_set_rxfh_indir(struct net_device *dev,
 	}
 
 	ret = ops->set_rxfh(dev, indir, NULL, ETH_RSS_HASH_NO_CHANGE);
+	if (ret)
+		goto out;
+
+	/* indicate whether rxfh was set to default */
+	if (user_size == 0)
+		dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
+	else
+		dev->priv_flags |= IFF_RXFH_CONFIGURED;
 
 out:
 	kfree(indir);
@@ -897,6 +938,14 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	}
 
 	ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
+	if (ret)
+		goto out;
+
+	/* indicate whether rxfh was set to default */
+	if (rxfh.indir_size == 0)
+		dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
+	else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
+		dev->priv_flags |= IFF_RXFH_CONFIGURED;
 
 out:
 	kfree(rss_config);
@@ -1228,6 +1277,8 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
 	struct ethtool_channels channels;
+	u32 max_rx = 0;
+	int ret;
 
 	if (!dev->ethtool_ops->set_channels)
 		return -EOPNOTSUPP;
@@ -1235,6 +1286,14 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 	if (copy_from_user(&channels, useraddr, sizeof(channels)))
 		return -EFAULT;
 
+	/* ensure the new Rx count fits within the configured Rx flow
+	 * indirection table settings */
+	if (netif_is_rxfh_configured(dev) &&
+	    ethtool_get_max_rxfh_channel(dev, &max_rx) &&
+	    ((channels.rx_count > max_rx) ||
+	     (channels.combined_count > max_rx))
+	    return -EINVAL;
+
 	return dev->ethtool_ops->set_channels(dev, &channels);
 }
 
-- 
2.7.0.236.gda096a0.dirty

[PATCH 2/4] ethtool: ensure channel counts are within bounds during SCHANNELS

[Jacob Keller]

Add a sanity check to ensure that all requested channel sizes are within
bounds, which should reduce errors in driver implementation.

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

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 8a40948c1fc6..71aff99157c3 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1276,16 +1276,27 @@ static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
 static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
-	struct ethtool_channels channels;
+	struct ethtool_channels channels, max;
 	u32 max_rx = 0;
 	int ret;
 
-	if (!dev->ethtool_ops->set_channels)
+	if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&channels, useraddr, sizeof(channels)))
 		return -EFAULT;
 
+	ret = dev->ethtool_ops->get_channels(dev, &max);
+	if (ret)
+		return ret;
+
+	/* ensure new counts are within the maximums */
+	if ((channels.rx_count > max.max_rx) ||
+	    (channels.tx_count > max.max_tx) ||
+	    (channels.combined_count > max.max_combined) ||
+	    (channels.other_count > max.max_other))
+		return -EINVAL;
+
 	/* ensure the new Rx count fits within the configured Rx flow
 	 * indirection table settings */
 	if (netif_is_rxfh_configured(dev) &&
-- 
2.7.0.236.gda096a0.dirty

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

[Jacob Keller]

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(+)

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) &&
-- 
2.7.0.236.gda096a0.dirty

[PATCH 4/4] fm10k: don't reinitialize RSS flow table when RXFH configured

[Jacob Keller]

Also print an error message incase we do have to reconfigure as this
should no longer happen anymore due to ethtool changes. If it somehow
does occur, user should be made aware of it.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 134ce4daa994..4e58f9155883 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1937,8 +1937,10 @@ static void fm10k_init_reta(struct fm10k_intfc *interface)
 	u16 i, rss_i = interface->ring_feature[RING_F_RSS].indices;
 	u32 reta, base;
 
-	/* If the netdev is initialized we have to maintain table if possible */
-	if (interface->netdev->reg_state != NETREG_UNINITIALIZED) {
+	/* If the Rx flow indirection table has been configured manually, we
+	 * need to maintain it when possible.
+	 */
+	if (netif_is_rxfh_configured(interface->netdev)) {
 		for (i = FM10K_RETA_SIZE; i--;) {
 			reta = interface->reta[i];
 			if ((((reta << 24) >> 24) < rss_i) &&
@@ -1946,6 +1948,10 @@ static void fm10k_init_reta(struct fm10k_intfc *interface)
 			    (((reta <<  8) >> 24) < rss_i) &&
 			    (((reta)       >> 24) < rss_i))
 				continue;
+
+			/* this should never happen */
+			dev_err(&interface->pdev->dev,
+				"RSS indirection table assigned flows out of queue bounds. Reconfiguring.\n");
 			goto repopulate_reta;
 		}
 
-- 
2.7.0.236.gda096a0.dirty

Re: [PATCH 1/4] ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

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:1292:6: error: implicit declaration of function 'ethtool_get_max_rxfh_channel' [-Werror=implicit-function-declaration]
         ethtool_get_max_rxfh_channel(dev, &max_rx) &&
         ^
>> net/core/ethtool.c:1295:6: error: expected ')' before 'return'
         return -EINVAL;
         ^
>> net/core/ethtool.c:1298:1: error: expected expression before '}' token
    }
    ^
   net/core/ethtool.c:1281:6: warning: unused variable 'ret' [-Wunused-variable]
     int ret;
         ^
   net/core/ethtool.c:1298:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^
   cc1: some warnings being treated as errors

vim +/ethtool_get_max_rxfh_channel +1292 net/core/ethtool.c

  1286		if (copy_from_user(&channels, useraddr, sizeof(channels)))
  1287			return -EFAULT;
  1288	
  1289		/* ensure the new Rx count fits within the configured Rx flow
  1290		 * indirection table settings */
  1291		if (netif_is_rxfh_configured(dev) &&
> 1292		    ethtool_get_max_rxfh_channel(dev, &max_rx) &&
  1293		    ((channels.rx_count > max_rx) ||
  1294		     (channels.combined_count > max_rx))
> 1295		    return -EINVAL;
  1296	
  1297		return dev->ethtool_ops->set_channels(dev, &channels);
> 1298	}
  1299	
  1300	static int ethtool_get_pauseparam(struct net_device *dev, void __user *useraddr)
  1301	{

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44070 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8446 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44070 bytes --]

Re: [PATCH 2/4] ethtool: ensure channel counts are within bounds during SCHANNELS

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

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: implicit declaration of function 'ethtool_get_max_rxfh_channel' [-Werror=implicit-function-declaration]
         ethtool_get_max_rxfh_channel(dev, &max_rx) &&
         ^
   net/core/ethtool.c:1306:6: error: expected ')' before 'return'
         return -EINVAL;
         ^
   net/core/ethtool.c:1309:1: error: expected expression before '}' token
    }
    ^
   net/core/ethtool.c:1309:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^
   cc1: some warnings being treated as errors

vim +1289 net/core/ethtool.c

  1283		if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
  1284			return -EOPNOTSUPP;
  1285	
  1286		if (copy_from_user(&channels, useraddr, sizeof(channels)))
  1287			return -EFAULT;
  1288	
> 1289		ret = dev->ethtool_ops->get_channels(dev, &max);
  1290		if (ret)
  1291			return ret;
  1292	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44070 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9353 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 30591 bytes --]

Re: [PATCH 1/4] ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8053 bytes --]

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: x86_64-randconfig-x013-201606 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/core/ethtool.c: In function 'ethtool_set_channels':
>> net/core/ethtool.c:2143:0: error: unterminated argument list invoking macro "if"
    }
    ^
>> net/core/ethtool.c:1291:2: error: expected '(' at end of input
     if (netif_is_rxfh_configured(dev) &&
     ^
   net/core/ethtool.c:1291:2: error: expected declaration or statement at end of input
   net/core/ethtool.c:1281:6: warning: unused variable 'ret' [-Wunused-variable]
     int ret;
         ^
   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)
               ^

vim +/if +2143 net/core/ethtool.c

^1da177e Linus Torvalds    2005-04-16  2137  		dev->ethtool_ops->complete(dev);
d8a33ac4 Stephen Hemminger 2005-05-29  2138  
d8a33ac4 Stephen Hemminger 2005-05-29  2139  	if (old_features != dev->features)
d8a33ac4 Stephen Hemminger 2005-05-29  2140  		netdev_features_change(dev);
d8a33ac4 Stephen Hemminger 2005-05-29  2141  
^1da177e Linus Torvalds    2005-04-16  2142  	return rc;
^1da177e Linus Torvalds    2005-04-16 @2143  }

:::::: The code at line 2143 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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22482 bytes --]

Re: [PATCH 1/4] ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

[Jakub Kicinski]

Build bot seems upset so let me throw few stones as well.

On Mon,  8 Feb 2016 12:06:02 -0800, Jacob Keller wrote:
>
> +static inline int ethool_get_max_rxfh_channel(struct net_device *dev, u32 *max)

static inline in C sources is frowned upon.

> +	u32 dev_size, current_max = 0;
> +	u32 *indir;
> +	int ret, i;
> +
> +	if (!dev->ethtool_ops->get_rxfh_indir_size ||
> +	    !dev->ethtool_ops->get_rxfh)
> +		return -EOPNOTSUPP;
> +	dev_size = dev->ethtool_ops->get_rxfh_indir_size(dev);
> +	if (dev_size == 0)
> +		return -EOPNOTSUPP;
> +
> +	indir = kcalloc(dev_size, sizeof(indir[0]), GFP_USER);
> +	if (!indir)
> +		return -ENOMEM;
> +
> +	ret = dev->ethtool_ops->get_rxfh(dev, indir, NULL, NULL);
> +	if (ret)
> +		goto out;
> +
> +	for (i = dev_size; i--;) {
> +		if (indir[i] > current_max)
> +			current_max = indir[i];
> +	}

Could be

while (dev_size--)
	current_max = max(current_max, indir[dev_size]);

> +	/* ensure the new Rx count fits within the configured Rx flow
> +	 * indirection table settings */
> +	if (netif_is_rxfh_configured(dev) &&
> +	    ethtool_get_max_rxfh_channel(dev, &max_rx) &&
> +	    ((channels.rx_count > max_rx) ||
> +	     (channels.combined_count > max_rx))
> +	    return -EINVAL;
> +
>  	return dev->ethtool_ops->set_channels(dev, &channels);
>  }

The situation with rx_count vs combined count is unclear.  My current
understanding is that

channels.combined_count + channels.rx_count > max_rx

would be the safest way to go.

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

[Jakub Kicinski]

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.

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

[Keller, Jacob E]

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

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

[Keller, Jacob E]

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

[PATCH 2/4] ethtool: ensure channel counts are within bounds during SCHANNELS

[Jacob Keller]

Add a sanity check to ensure that all requested channel sizes are within
bounds, which should reduce errors in driver implementation.

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

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 59aebaf9ed54..dc4f6632f33b 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1274,15 +1274,24 @@ static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
 static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
-	struct ethtool_channels channels;
+	struct ethtool_channels channels, max;
 	u32 max_rx_in_use = 0;
 
-	if (!dev->ethtool_ops->set_channels)
+	if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&channels, useraddr, sizeof(channels)))
 		return -EFAULT;
 
+	dev->ethtool_ops->get_channels(dev, &max);
+
+	/* ensure new counts are within the maximums */
+	if ((channels.rx_count > max.max_rx) ||
+	    (channels.tx_count > max.max_tx) ||
+	    (channels.combined_count > max.max_combined) ||
+	    (channels.other_count > max.max_other))
+		return -EINVAL;
+
 	/* ensure the new Rx count fits within the configured Rx flow
 	 * indirection table settings */
 	if (netif_is_rxfh_configured(dev) &&
-- 
2.7.0.236.gda096a0.dirty