[PATCH ethtool 0/5] RSS improvements + new speeds

[PATCH ethtool 0/5] RSS improvements + new speeds

[Amir Vadai]

Hi Ben,

This patchset contains some features and small bug fixes.
A patch by Eyal Perry to enable setting RSS hash function, kernel support was
added in commit 892311f ("ethtool: Support for configurable RSS hash
function").
Eyal Grossman added some missing advertised speeds and fixed some issues in
this area of code.

Patches were tested and applied on top of commit 4614098 ("Release version 3.16.")

Thanks,
Amir

Eyal Grossman (3):
  ethtool-copy.h: sync with net
  ethtool: Add missing Advertised speeds
  ethtool: Return bad status when send_ioctl fails

Eyal Perry (2):
  ethtool: Prettify RX flow hash indirection table print
  ethtool: Support for configurable RSS hash function

 ethtool-copy.h |  67 +++++++++++++++++++++++++++----
 ethtool.8.in   |   7 ++++
 ethtool.c      | 122 ++++++++++++++++++++++++++++++++++++++++-----------------
 3 files changed, 152 insertions(+), 44 deletions(-)

-- 
1.9.3

[PATCH ethtool 1/5] ethtool-copy.h: sync with net

[Amir Vadai]

From: Eyal Grossman <eyalgr@mellanox.com>

This covers kernel changes up to:

commit 892311f66f2411b813ca631009356891a0c2b0a1
Author: Eyal Perry <eyalpe@mellanox.com>
Date:   Tue Dec 2 18:12:10 2014 +0200

	ethtool: Support for configurable RSS hash function

Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 ethtool-copy.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 8 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 61b78fc..5f66d9c 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -10,8 +10,8 @@
  * Portions Copyright (C) Sun Microsystems 2008
  */
 
-#ifndef _LINUX_ETHTOOL_H
-#define _LINUX_ETHTOOL_H
+#ifndef _UAPI_LINUX_ETHTOOL_H
+#define _UAPI_LINUX_ETHTOOL_H
 
 #include <linux/types.h>
 #include <linux/if_ether.h>
@@ -110,7 +110,7 @@ struct ethtool_cmd {
 	__u32	reserved[2];
 };
 
-static __inline__ void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
+static inline void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
 					 __u32 speed)
 {
 
@@ -118,7 +118,7 @@ static __inline__ void ethtool_cmd_speed_set(struct ethtool_cmd *ep,
 	ep->speed_hi = (__u16)(speed >> 16);
 }
 
-static __inline__ __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
+static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
 {
 	return (ep->speed_hi << 16) | ep->speed;
 }
@@ -209,6 +209,33 @@ struct ethtool_value {
 	__u32	data;
 };
 
+enum tunable_id {
+	ETHTOOL_ID_UNSPEC,
+	ETHTOOL_RX_COPYBREAK,
+	ETHTOOL_TX_COPYBREAK,
+};
+
+enum tunable_type_id {
+	ETHTOOL_TUNABLE_UNSPEC,
+	ETHTOOL_TUNABLE_U8,
+	ETHTOOL_TUNABLE_U16,
+	ETHTOOL_TUNABLE_U32,
+	ETHTOOL_TUNABLE_U64,
+	ETHTOOL_TUNABLE_STRING,
+	ETHTOOL_TUNABLE_S8,
+	ETHTOOL_TUNABLE_S16,
+	ETHTOOL_TUNABLE_S32,
+	ETHTOOL_TUNABLE_S64,
+};
+
+struct ethtool_tunable {
+	__u32	cmd;
+	__u32	id;
+	__u32	type_id;
+	__u32	len;
+	void	*data[0];
+};
+
 /**
  * struct ethtool_regs - hardware register dump
  * @cmd: Command number = %ETHTOOL_GREGS
@@ -507,6 +534,7 @@ struct ethtool_pauseparam {
  * @ETH_SS_NTUPLE_FILTERS: Previously used with %ETHTOOL_GRXNTUPLE;
  *	now deprecated
  * @ETH_SS_FEATURES: Device feature names
+ * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
  */
 enum ethtool_stringset {
 	ETH_SS_TEST		= 0,
@@ -514,6 +542,7 @@ enum ethtool_stringset {
 	ETH_SS_PRIV_FLAGS,
 	ETH_SS_NTUPLE_FILTERS,
 	ETH_SS_FEATURES,
+	ETH_SS_RSS_HASH_FUNCS,
 };
 
 /**
@@ -857,6 +886,8 @@ struct ethtool_rxfh_indir {
  * @key_size: On entry, the array size of the user buffer for the hash key,
  *	which may be zero.  On return from %ETHTOOL_GRSSH, the size of the
  *	hardware hash key.
+ * @hfunc: Defines the current RSS hash function used by HW (or to be set to).
+ *	Valid values are one of the %ETH_RSS_HASH_*.
  * @rsvd:	Reserved for future extensions.
  * @rss_config: RX ring/queue index for each hash value i.e., indirection table
  *	of @indir_size __u32 elements, followed by hash key of @key_size
@@ -866,14 +897,16 @@ struct ethtool_rxfh_indir {
  * size should be returned.  For %ETHTOOL_SRSSH, an @indir_size of
  * %ETH_RXFH_INDIR_NO_CHANGE means that indir table setting is not requested
  * and a @indir_size of zero means the indir table should be reset to default
- * values.
+ * values. An hfunc of zero means that hash function setting is not requested.
  */
 struct ethtool_rxfh {
 	__u32   cmd;
 	__u32	rss_context;
 	__u32   indir_size;
 	__u32   key_size;
-	__u32	rsvd[2];
+	__u8	hfunc;
+	__u8	rsvd8[3];
+	__u32	rsvd32;
 	__u32   rss_config[0];
 };
 #define ETH_RXFH_INDIR_NO_CHANGE	0xffffffff
@@ -1152,6 +1185,8 @@ enum ethtool_sfeatures_retval_bits {
 
 #define ETHTOOL_GRSSH		0x00000046 /* Get RX flow hash configuration */
 #define ETHTOOL_SRSSH		0x00000047 /* Set RX flow hash configuration */
+#define ETHTOOL_GTUNABLE	0x00000048 /* Get tunable configuration */
+#define ETHTOOL_STUNABLE	0x00000049 /* Set tunable configuration */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -1184,6 +1219,10 @@ enum ethtool_sfeatures_retval_bits {
 #define SUPPORTED_40000baseCR4_Full	(1 << 24)
 #define SUPPORTED_40000baseSR4_Full	(1 << 25)
 #define SUPPORTED_40000baseLR4_Full	(1 << 26)
+#define SUPPORTED_56000baseKR4_Full	(1 << 27)
+#define SUPPORTED_56000baseCR4_Full	(1 << 28)
+#define SUPPORTED_56000baseSR4_Full	(1 << 29)
+#define SUPPORTED_56000baseLR4_Full	(1 << 30)
 
 #define ADVERTISED_10baseT_Half		(1 << 0)
 #define ADVERTISED_10baseT_Full		(1 << 1)
@@ -1212,6 +1251,10 @@ enum ethtool_sfeatures_retval_bits {
 #define ADVERTISED_40000baseCR4_Full	(1 << 24)
 #define ADVERTISED_40000baseSR4_Full	(1 << 25)
 #define ADVERTISED_40000baseLR4_Full	(1 << 26)
+#define ADVERTISED_56000baseKR4_Full	(1 << 27)
+#define ADVERTISED_56000baseCR4_Full	(1 << 28)
+#define ADVERTISED_56000baseSR4_Full	(1 << 29)
+#define ADVERTISED_56000baseLR4_Full	(1 << 30)
 
 /* The following are all involved in forcing a particular link
  * mode for the device for setting things.  When getting the
@@ -1219,12 +1262,16 @@ enum ethtool_sfeatures_retval_bits {
  * it was forced up into this mode or autonegotiated.
  */
 
-/* The forced speed, 10Mb, 100Mb, gigabit, 2.5Gb, 10GbE. */
+/* The forced speed, 10Mb, 100Mb, gigabit, [2.5|10|20|40|56]GbE. */
 #define SPEED_10		10
 #define SPEED_100		100
 #define SPEED_1000		1000
 #define SPEED_2500		2500
 #define SPEED_10000		10000
+#define SPEED_20000		20000
+#define SPEED_40000		40000
+#define SPEED_56000		56000
+
 #define SPEED_UNKNOWN		-1
 
 /* Duplex, half or full. */
@@ -1314,6 +1361,10 @@ enum ethtool_sfeatures_retval_bits {
 #define ETH_MODULE_SFF_8079_LEN		256
 #define ETH_MODULE_SFF_8472		0x2
 #define ETH_MODULE_SFF_8472_LEN		512
+#define ETH_MODULE_SFF_8636		0x3
+#define ETH_MODULE_SFF_8636_LEN		256
+#define ETH_MODULE_SFF_8436		0x4
+#define ETH_MODULE_SFF_8436_LEN		256
 
 /* Reset flags */
 /* The reset() operation must clear the flags for the components which
@@ -1345,4 +1396,4 @@ enum ethtool_reset_flags {
 };
 #define ETH_RESET_SHARED_SHIFT	16
 
-#endif /* _LINUX_ETHTOOL_H */
+#endif /* _UAPI_LINUX_ETHTOOL_H */
-- 
1.9.3

Re: [PATCH ethtool 1/5] ethtool-copy.h: sync with net

[Ben Hutchings]

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

On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> From: Eyal Grossman <eyalgr@mellanox.com>
> 
> This covers kernel changes up to:
> 
> commit 892311f66f2411b813ca631009356891a0c2b0a1
> Author: Eyal Perry <eyalpe@mellanox.com>
> Date:   Tue Dec 2 18:12:10 2014 +0200
> 
> 	ethtool: Support for configurable RSS hash function
> 
> Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>  ethtool-copy.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 59 insertions(+), 8 deletions(-)
> 
> diff --git a/ethtool-copy.h b/ethtool-copy.h
> index 61b78fc..5f66d9c 100644
> --- a/ethtool-copy.h
> +++ b/ethtool-copy.h
> @@ -10,8 +10,8 @@
>   * Portions Copyright (C) Sun Microsystems 2008
>   */
>  
> -#ifndef _LINUX_ETHTOOL_H
> -#define _LINUX_ETHTOOL_H
> +#ifndef _UAPI_LINUX_ETHTOOL_H
> +#define _UAPI_LINUX_ETHTOOL_H
[...]

You've copied from include/uapi/linux/ethtool.h, which is not quite
right.  The include/uapi directory contains definitions shared with
userland, but the headers still need some processing (like removing the
_UAPI prefix from header guards).  You should use 'make headers_install'
to generate the userland version in usr/include/linux/ethtool.h.
Anyway, I've done that myself based on today's net tree.

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH ethtool 1/5] ethtool-copy.h: sync with net

[Amir Vadai]

On Sun, Apr 5, 2015 at 4:00 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
>> From: Eyal Grossman <eyalgr@mellanox.com>
>>
>> This covers kernel changes up to:
>>
>> commit 892311f66f2411b813ca631009356891a0c2b0a1
>> Author: Eyal Perry <eyalpe@mellanox.com>
>> Date:   Tue Dec 2 18:12:10 2014 +0200
>>
>>       ethtool: Support for configurable RSS hash function
>>
>> Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>  ethtool-copy.h | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 59 insertions(+), 8 deletions(-)
>>
>> diff --git a/ethtool-copy.h b/ethtool-copy.h
>> index 61b78fc..5f66d9c 100644
>> --- a/ethtool-copy.h
>> +++ b/ethtool-copy.h
>> @@ -10,8 +10,8 @@
>>   * Portions Copyright (C) Sun Microsystems 2008
>>   */
>>
>> -#ifndef _LINUX_ETHTOOL_H
>> -#define _LINUX_ETHTOOL_H
>> +#ifndef _UAPI_LINUX_ETHTOOL_H
>> +#define _UAPI_LINUX_ETHTOOL_H
> [...]
>
> You've copied from include/uapi/linux/ethtool.h, which is not quite
> right.  The include/uapi directory contains definitions shared with
> userland, but the headers still need some processing (like removing the
> _UAPI prefix from header guards).  You should use 'make headers_install'
> to generate the userland version in usr/include/linux/ethtool.h.
> Anyway, I've done that myself based on today's net tree.
>
> Ben.

Hi,

This is a long weekend in Israel (Passover).
I will address the issues and send a V1 on Sunday.

Thanks,
Amir

>
> --
> Ben Hutchings
> Quantity is no substitute for quality, but it's the only one we've got.

Re: [PATCH ethtool 1/5] ethtool-copy.h: sync with net

[Amir Vadai]

On Sun, Apr 5, 2015 at 4:00 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
>> From: Eyal Grossman <eyalgr@mellanox.com>

[...]

>
> You've copied from include/uapi/linux/ethtool.h, which is not quite
> right.  The include/uapi directory contains definitions shared with
> userland, but the headers still need some processing (like removing the
> _UAPI prefix from header guards).  You should use 'make headers_install'
> to generate the userland version in usr/include/linux/ethtool.h.
Sorry, didn't know.

> Anyway, I've done that myself based on today's net tree.
Is it in your git [1] ? Because I couldn't find the updated version there.

[1] - git://git.kernel.org/pub/scm/network/ethtool/ethtool.git

Thanks,
Amir
>
> Ben.
>
> --
> Ben Hutchings
> Quantity is no substitute for quality, but it's the only one we've got.

Re: [PATCH ethtool 1/5] ethtool-copy.h: sync with net

[Ben Hutchings]

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

On Tue, 2015-04-14 at 17:34 +0300, Amir Vadai wrote:
> On Sun, Apr 5, 2015 at 4:00 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> >> From: Eyal Grossman <eyalgr@mellanox.com>
> 
> [...]
> 
> >
> > You've copied from include/uapi/linux/ethtool.h, which is not quite
> > right.  The include/uapi directory contains definitions shared with
> > userland, but the headers still need some processing (like removing the
> > _UAPI prefix from header guards).  You should use 'make headers_install'
> > to generate the userland version in usr/include/linux/ethtool.h.
> Sorry, didn't know.
> 
> > Anyway, I've done that myself based on today's net tree.
> Is it in your git [1] ? Because I couldn't find the updated version there.
> 
> [1] - git://git.kernel.org/pub/scm/network/ethtool/ethtool.git

Sorry, it is now.

Ben.

-- 
Ben Hutchings
Editing code like this is akin to sticking plasters on the bleeding stump
of a severed limb. - me, 29 June 1999

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH ethtool 2/5] ethtool: Add missing Advertised speeds

[Amir Vadai]

From: Eyal Grossman <eyalgr@mellanox.com>

Added the following missing advertised speed modes:
- ADVERTISED_10000baseT_Full
- ADVERTISED_56000baseKR4_Full
- ADVERTISED_56000baseCR4_Full
- ADVERTISED_56000baseSR4_Full
- ADVERTISED_56000baseLR4_Full
- ADVERTISED_10000baseKX4_Full

In order to reduce code duplication we added a macro to
ALL_ADVERTISED_FLAGS in line 88 ALL_ADVERTISED_MODES,
in addition the changed we made added speed that were
missing from ALL_ADVERTISED_MODES
(e.g. ADVERTISED_10000baseKX4_Full).

Added ADVERTISED_10000baseR_FEC to mode_defs.

Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 ethtool.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index bf583f3..7b873d3 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -53,7 +53,9 @@
 	 ADVERTISED_100baseT_Full |		\
 	 ADVERTISED_1000baseT_Half |		\
 	 ADVERTISED_1000baseT_Full |		\
+	 ADVERTISED_1000baseKX_Full|		\
 	 ADVERTISED_2500baseX_Full |		\
+	 ADVERTISED_10000baseT_Full |		\
 	 ADVERTISED_10000baseKX4_Full |		\
 	 ADVERTISED_10000baseKR_Full |		\
 	 ADVERTISED_10000baseR_FEC |		\
@@ -62,36 +64,23 @@
 	 ADVERTISED_40000baseKR4_Full |		\
 	 ADVERTISED_40000baseCR4_Full |		\
 	 ADVERTISED_40000baseSR4_Full |		\
-	 ADVERTISED_40000baseLR4_Full)
+	 ADVERTISED_40000baseLR4_Full |		\
+	 ADVERTISED_56000baseKR4_Full |		\
+	 ADVERTISED_56000baseCR4_Full |		\
+	 ADVERTISED_56000baseSR4_Full |		\
+	 ADVERTISED_56000baseLR4_Full)
 
 #define ALL_ADVERTISED_FLAGS			\
-	(ADVERTISED_10baseT_Half |		\
-	 ADVERTISED_10baseT_Full |		\
-	 ADVERTISED_100baseT_Half |		\
-	 ADVERTISED_100baseT_Full |		\
-	 ADVERTISED_1000baseT_Half |		\
-	 ADVERTISED_1000baseT_Full |		\
-	 ADVERTISED_Autoneg |			\
+	(ADVERTISED_Autoneg |			\
 	 ADVERTISED_TP |			\
 	 ADVERTISED_AUI |			\
 	 ADVERTISED_MII |			\
 	 ADVERTISED_FIBRE |			\
 	 ADVERTISED_BNC |			\
-	 ADVERTISED_10000baseT_Full |		\
 	 ADVERTISED_Pause |			\
 	 ADVERTISED_Asym_Pause |		\
-	 ADVERTISED_2500baseX_Full |		\
 	 ADVERTISED_Backplane |			\
-	 ADVERTISED_1000baseKX_Full |		\
-	 ADVERTISED_10000baseKX4_Full |		\
-	 ADVERTISED_10000baseKR_Full |		\
-	 ADVERTISED_10000baseR_FEC |		\
-	 ADVERTISED_20000baseMLD2_Full |	\
-	 ADVERTISED_20000baseKR2_Full |		\
-	 ADVERTISED_40000baseKR4_Full |		\
-	 ADVERTISED_40000baseCR4_Full |		\
-	 ADVERTISED_40000baseSR4_Full |		\
-	 ADVERTISED_40000baseLR4_Full)
+	 ALL_ADVERTISED_MODES)
 
 #ifndef HAVE_NETIF_MSG
 enum {
@@ -527,15 +516,20 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
 		{ 1, ADVERTISED_1000baseT_Full,     "1000baseT/Full" },
 		{ 0, ADVERTISED_1000baseKX_Full,    "1000baseKX/Full" },
 		{ 0, ADVERTISED_2500baseX_Full,     "2500baseX/Full" },
-		{ 0, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },
+		{ 1, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },
 		{ 0, ADVERTISED_10000baseKX4_Full,  "10000baseKX4/Full" },
 		{ 0, ADVERTISED_10000baseKR_Full,   "10000baseKR/Full" },
+		{ 0, ADVERTISED_10000baseR_FEC,     "10000baseR/FEC" },
 		{ 0, ADVERTISED_20000baseMLD2_Full, "20000baseMLD2/Full" },
 		{ 0, ADVERTISED_20000baseKR2_Full,  "20000baseKR2/Full" },
 		{ 0, ADVERTISED_40000baseKR4_Full,  "40000baseKR4/Full" },
 		{ 0, ADVERTISED_40000baseCR4_Full,  "40000baseCR4/Full" },
 		{ 0, ADVERTISED_40000baseSR4_Full,  "40000baseSR4/Full" },
 		{ 0, ADVERTISED_40000baseLR4_Full,  "40000baseLR4/Full" },
+		{ 0, ADVERTISED_56000baseKR4_Full,  "56000baseKR4/Full" },
+		{ 0, ADVERTISED_56000baseCR4_Full,  "56000baseCR4/Full" },
+		{ 0, ADVERTISED_56000baseSR4_Full,  "56000baseSR4/Full" },
+		{ 0, ADVERTISED_56000baseLR4_Full,  "56000baseLR4/Full" },
 	};
 	int indent;
 	int did1, new_line_pend, i;
-- 
1.9.3

Re: [PATCH ethtool 2/5] ethtool: Add missing Advertised speeds

[Ben Hutchings]

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

On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> From: Eyal Grossman <eyalgr@mellanox.com>
> 
> Added the following missing advertised speed modes:
> - ADVERTISED_10000baseT_Full
> - ADVERTISED_56000baseKR4_Full
> - ADVERTISED_56000baseCR4_Full
> - ADVERTISED_56000baseSR4_Full
> - ADVERTISED_56000baseLR4_Full
> - ADVERTISED_10000baseKX4_Full
> 
> In order to reduce code duplication we added a macro to
> ALL_ADVERTISED_FLAGS in line 88 ALL_ADVERTISED_MODES,
> in addition the changed we made added speed that were
> missing from ALL_ADVERTISED_MODES
> (e.g. ADVERTISED_10000baseKX4_Full).

Well spotted.

> Added ADVERTISED_10000baseR_FEC to mode_defs.
>
> Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
[...]
> @@ -527,15 +516,20 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
>  		{ 1, ADVERTISED_1000baseT_Full,     "1000baseT/Full" },
>  		{ 0, ADVERTISED_1000baseKX_Full,    "1000baseKX/Full" },
>  		{ 0, ADVERTISED_2500baseX_Full,     "2500baseX/Full" },
> -		{ 0, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },
> +		{ 1, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },

Why should this be on the same line as 2500baseX/Full?

>  		{ 0, ADVERTISED_10000baseKX4_Full,  "10000baseKX4/Full" },
>  		{ 0, ADVERTISED_10000baseKR_Full,   "10000baseKR/Full" },
> +		{ 0, ADVERTISED_10000baseR_FEC,     "10000baseR/FEC" },

This is not a link mode, it's a separate capability that applies to all
10GBASE-R modes.  It's also meaningful for 40G and 100G modes despite
the way we've named the flags.  Please put it in a separate section
below the link modes.

Ben.

>  		{ 0, ADVERTISED_20000baseMLD2_Full, "20000baseMLD2/Full" },
>  		{ 0, ADVERTISED_20000baseKR2_Full,  "20000baseKR2/Full" },
>  		{ 0, ADVERTISED_40000baseKR4_Full,  "40000baseKR4/Full" },
>  		{ 0, ADVERTISED_40000baseCR4_Full,  "40000baseCR4/Full" },
>  		{ 0, ADVERTISED_40000baseSR4_Full,  "40000baseSR4/Full" },
>  		{ 0, ADVERTISED_40000baseLR4_Full,  "40000baseLR4/Full" },
> +		{ 0, ADVERTISED_56000baseKR4_Full,  "56000baseKR4/Full" },
> +		{ 0, ADVERTISED_56000baseCR4_Full,  "56000baseCR4/Full" },
> +		{ 0, ADVERTISED_56000baseSR4_Full,  "56000baseSR4/Full" },
> +		{ 0, ADVERTISED_56000baseLR4_Full,  "56000baseLR4/Full" },
>  	};
>  	int indent;
>  	int did1, new_line_pend, i;

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH ethtool 2/5] ethtool: Add missing Advertised speeds

[Ben Hutchings]

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

On Sun, 2015-04-05 at 02:37 +0100, Ben Hutchings wrote:
> On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> > From: Eyal Grossman <eyalgr@mellanox.com>
> > 
> > Added the following missing advertised speed modes:
> > - ADVERTISED_10000baseT_Full
> > - ADVERTISED_56000baseKR4_Full
> > - ADVERTISED_56000baseCR4_Full
> > - ADVERTISED_56000baseSR4_Full
> > - ADVERTISED_56000baseLR4_Full
> > - ADVERTISED_10000baseKX4_Full
> > 
> > In order to reduce code duplication we added a macro to
> > ALL_ADVERTISED_FLAGS in line 88 ALL_ADVERTISED_MODES,
> > in addition the changed we made added speed that were
> > missing from ALL_ADVERTISED_MODES
> > (e.g. ADVERTISED_10000baseKX4_Full).
> 
> Well spotted.
> 
> > Added ADVERTISED_10000baseR_FEC to mode_defs.
> >
> > Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
> > Signed-off-by: Amir Vadai <amirv@mellanox.com>
> > ---
> [...]
> > @@ -527,15 +516,20 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
> >  		{ 1, ADVERTISED_1000baseT_Full,     "1000baseT/Full" },
> >  		{ 0, ADVERTISED_1000baseKX_Full,    "1000baseKX/Full" },
> >  		{ 0, ADVERTISED_2500baseX_Full,     "2500baseX/Full" },
> > -		{ 0, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },
> > +		{ 1, ADVERTISED_10000baseT_Full,    "10000baseT/Full" },
> 
> Why should this be on the same line as 2500baseX/Full?
> 
> >  		{ 0, ADVERTISED_10000baseKX4_Full,  "10000baseKX4/Full" },
> >  		{ 0, ADVERTISED_10000baseKR_Full,   "10000baseKR/Full" },
> > +		{ 0, ADVERTISED_10000baseR_FEC,     "10000baseR/FEC" },
> 
> This is not a link mode, it's a separate capability that applies to all
> 10GBASE-R modes.  It's also meaningful for 40G and 100G modes despite
> the way we've named the flags.  Please put it in a separate section
> below the link modes.

I've applied this without the two bits I commented on.

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH ethtool 2/5] ethtool: Add missing Advertised speeds

[Ben Hutchings]

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

[Adding in maintainers of drivers that can report FEC ability.]

On Sun, 2015-04-05 at 02:37 +0100, Ben Hutchings wrote:
[...]
> >  		{ 0, ADVERTISED_10000baseKX4_Full,  "10000baseKX4/Full" },
> >  		{ 0, ADVERTISED_10000baseKR_Full,   "10000baseKR/Full" },
> > +		{ 0, ADVERTISED_10000baseR_FEC,     "10000baseR/FEC" },
> 
> This is not a link mode, it's a separate capability that applies to all
> 10GBASE-R modes.  It's also meaningful for 40G and 100G modes despite
> the way we've named the flags.  Please put it in a separate section
> below the link modes.

So how about reporting FEC ability like this, using a slightly generic
heading to allow for other similar options in future?

diff --git a/ethtool.c b/ethtool.c
index 7e5fa9d..3badf5e 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -560,6 +560,12 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
 	fprintf(stdout, "\n");
 
 	if (!link_mode_only) {
+		fprintf(stdout, "	%s link options: ", prefix);
+		if (mask & ADVERTISED_10000baseR_FEC)
+			fprintf(stdout, "baseR-FEC\n");
+		else
+			fprintf(stdout, "None\n");
+
 		fprintf(stdout, "	%s pause frame use: ", prefix);
 		if (mask & ADVERTISED_Pause) {
 			fprintf(stdout, "Symmetric");
--- END ---

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH ethtool 3/5] ethtool: Return bad status when send_ioctl fails

[Amir Vadai]

From: Eyal Grossman <eyalgr@mellanox.com>

Added to ethtool return code(rc != 0) when an error occurs after send_ioctl has fails.

Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 ethtool.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 7b873d3..8d53a53 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -2352,6 +2352,7 @@ static int do_sset(struct cmd_context *ctx)
 	char **argp = ctx->argp;
 	int i;
 	int err;
+	int status = 0;
 
 	for (i = 0; i < ARRAY_SIZE(flags_msglvl); i++)
 		flag_to_cmdline_info(flags_msglvl[i].name,
@@ -2523,6 +2524,7 @@ static int do_sset(struct cmd_context *ctx)
 		err = send_ioctl(ctx, &ecmd);
 		if (err < 0) {
 			perror("Cannot get current device settings");
+			status += err;
 		} else {
 			/* Change everything the user specified. */
 			if (speed_wanted != -1)
@@ -2593,8 +2595,10 @@ static int do_sset(struct cmd_context *ctx)
 			/* Try to perform the update. */
 			ecmd.cmd = ETHTOOL_SSET;
 			err = send_ioctl(ctx, &ecmd);
-			if (err < 0)
+			if (err < 0) {
 				perror("Cannot set new settings");
+				status += err;
+			}
 		}
 		if (err < 0) {
 			if (speed_wanted != -1)
@@ -2621,6 +2625,7 @@ static int do_sset(struct cmd_context *ctx)
 		err = send_ioctl(ctx, &wol);
 		if (err < 0) {
 			perror("Cannot get current wake-on-lan settings");
+			status += err;
 		} else {
 			/* Change everything the user specified. */
 			if (wol_change) {
@@ -2636,8 +2641,10 @@ static int do_sset(struct cmd_context *ctx)
 			/* Try to perform the update. */
 			wol.cmd = ETHTOOL_SWOL;
 			err = send_ioctl(ctx, &wol);
-			if (err < 0)
+			if (err < 0) {
 				perror("Cannot set new wake-on-lan settings");
+				status += err;
+			}
 		}
 		if (err < 0) {
 			if (wol_change)
@@ -2654,17 +2661,20 @@ static int do_sset(struct cmd_context *ctx)
 		err = send_ioctl(ctx, &edata);
 		if (err < 0) {
 			perror("Cannot get msglvl");
+			status += err;
 		} else {
 			edata.cmd = ETHTOOL_SMSGLVL;
 			edata.data = ((edata.data & ~msglvl_mask) |
 				      msglvl_wanted);
 			err = send_ioctl(ctx, &edata);
-			if (err < 0)
+			if (err < 0) {
 				perror("Cannot set new msglvl");
+				status += err;
+			}
 		}
 	}
 
-	return 0;
+	return status;
 }
 
 static int do_gregs(struct cmd_context *ctx)
-- 
1.9.3

Re: [PATCH ethtool 3/5] ethtool: Return bad status when send_ioctl fails

[Ben Hutchings]

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

On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> From: Eyal Grossman <eyalgr@mellanox.com>
> 
> Added to ethtool return code(rc != 0) when an error occurs after send_ioctl has fails.
> 
> Signed-off-by: Eyal Grossman <eyalgr@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>

I wish we could do this, but it's been returning 0 on failure for as
long as git history lasts (10 years) and I would expect this change to
break some scripts.

Maybe we could provide some way to opt-in to this behaviour?

[..]
> @@ -2654,17 +2661,20 @@ static int do_sset(struct cmd_context *ctx)
>  		err = send_ioctl(ctx, &edata);
>  		if (err < 0) {
>  			perror("Cannot get msglvl");
> +			status += err;
>  		} else {
>  			edata.cmd = ETHTOOL_SMSGLVL;
>  			edata.data = ((edata.data & ~msglvl_mask) |
>  				      msglvl_wanted);
>  			err = send_ioctl(ctx, &edata);
> -			if (err < 0)
> +			if (err < 0) {
>  				perror("Cannot set new msglvl");
> +				status += err;
> +			}
>  		}
>  	}
>  
> -	return 0;
> +	return status;

This is going to be a negative number on error, which is not a valid
exit code.  We should generally return 1 on error.

Ben.

>  }
>  
>  static int do_gregs(struct cmd_context *ctx)

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH ethtool 4/5] ethtool: Prettify RX flow hash indirection table print

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

When indirection table size is not a multiple of 8, a new line is
missing at the last row of the table.
In addition, make a distinction between the attribute name and its value
by adding an indentation on the beginning of each line which contains
values.

Before the changne:
[user@host]# ./ethtool-3.16 -x enp5s0
RX flow hash indirection table for enp5s0 with 20 RX ring(s):
    0:      0     1     2     3     4     5     6     7
    8:      8     9    10    11    12    13    14    15
   16:      0     1     2     3RSS hash key:
16:d5:5a:31:21:8d:0e:2b:55:ea:ca:70:a8:19:5e:72:2e:c0:f9:0f:9b:6c:94:8f:59:ca:42:d1:c3:58:91:4a:3d:77:a1:e5:ab:8b:6f:68

After:
[user@host]# ./ethtool-3.16+ -x enp5s0
RX flow hash indirection table for enp5s0 with 20 RX ring(s):
    0:      0     1     2     3     4     5     6     7
    8:      8     9    10    11    12    13    14    15
   16:      0     1     2     3
RSS hash key:
    16:d5:5a:31:21:8d:0e:2b:55:ea:ca:70:a8:19:5e:72:2e:c0:f9:0f:9b:6c:94:8f:59:ca:42:d1:c3:58:91:4a:3d:77:a1:e5:ab:8b:6f:68

Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 ethtool.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 8d53a53..c2f4164 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3120,19 +3120,19 @@ static void print_indir_table(struct cmd_context *ctx,
 {
 	u32 i;
 
-	printf("RX flow hash indirection table for %s with %llu RX ring(s):\n",
+	printf("RX flow hash indirection table for %s with %llu RX ring(s):",
 	       ctx->devname, ring_count->data);
 
 	if (!indir_size)
-		printf("Operation not supported\n");
+		printf("\n  Operation not supported");
 
 	for (i = 0; i < indir_size; i++) {
 		if (i % 8 == 0)
-			printf("%5u: ", i);
+			printf("\n%5u: ", i);
 		printf(" %5u", indir[i]);
-		if (i % 8 == 7)
-			fputc('\n', stdout);
 	}
+	fputc('\n', stdout);
+
 }
 
 static int do_grxfhindir(struct cmd_context *ctx,
@@ -3220,7 +3220,7 @@ static int do_grxfh(struct cmd_context *ctx)
 	indir_bytes = rss->indir_size * sizeof(rss->rss_config[0]);
 	hkey = ((char *)rss->rss_config + indir_bytes);
 
-	printf("RSS hash key:\n");
+	printf("RSS hash key:\n    ");
 	if (!rss->key_size)
 		printf("Operation not supported\n");
 
-- 
1.9.3

Re: [PATCH ethtool 4/5] ethtool: Prettify RX flow hash indirection table print

[Ben Hutchings]

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

On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> From: Eyal Perry <eyalpe@mellanox.com>
> 
> When indirection table size is not a multiple of 8, a new line is
> missing at the last row of the table.

Yes, good point.

> In addition, make a distinction between the attribute name and its value
> by adding an indentation on the beginning of each line which contains
> values.

We've never consistently indented like that, so I don't see the point in
changing the output of just this one sub-command.

I'll just fix the first thing.

Ben.

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH ethtool] Fix formatting of RX flow hash indirection table when size % 8 != 0

[Ben Hutchings]

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

We only print a new-line after every 8 entries, but we need one
at the end of the table even if the table size is not a multiple
of 8.

Reported-by: Amir Vadai <amirv@mellanox.com>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This is what I've applied - hope it works for you as I don't have a test
case.

Ben.

 ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ethtool.c b/ethtool.c
index bf583f3..996efb9 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3126,7 +3126,7 @@ static void print_indir_table(struct cmd_context *ctx,
 		if (i % 8 == 0)
 			printf("%5u: ", i);
 		printf(" %5u", indir[i]);
-		if (i % 8 == 7)
+		if (i % 8 == 7 || i == indir_size - 1)
 			fputc('\n', stdout);
 	}
 }


-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH ethtool] Fix formatting of RX flow hash indirection table when size % 8 != 0

[Amir Vadai]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 4/5/2015 5:14 AM, Ben Hutchings wrote:
> We only print a new-line after every 8 entries, but we need one at
> the end of the table even if the table size is not a multiple of
> 8.
> 
> Reported-by: Amir Vadai <amirv@mellanox.com> Signed-off-by: Ben
> Hutchings <ben@decadent.org.uk> --- This is what I've applied -
> hope it works for you as I don't have a test case.
> 
Yes it does :)

Amir

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVM4YZAAoJEAgXBixSko7zCDMP/0xLY9ts5/TDPK8t0ZCHBjik
8OOEasSjjpg8ysLMuBhK4mZETwDZOMZvKGd/ZTFy0UW/xPSDW2PiB1cGrBUIJ+0p
kJ7K+pMeL6RsucRlc+DwRE7mR3/cHPjj33gIJoyW0zzJj9NssP40RAP3aNgL8N0n
6mxNTpoNfncyF7gFL7km1BMxhjAy5cgp/CZ3YdTyxTosNn8udxHQPVVYIgn/UQMQ
+JfIxXB/1i/G4jO0SUENXb+Y/biB98lhItYLHYsVLiDMTMczxb48oF8TA9adQuA9
ke27s9nQTg3oc5ZObL93YBSnr9XRCC94/d3HBAviQmzOKZiFoIkz1vX4RrL8MOXC
xFAwR7F/ozskEUqt8YxRsZpgoyT3zi+MMAHr+c8JtfMWN7ycLpIvQ0/GDuzv7Xjf
lG4VorsSbsdleWpno6aKudzggKjT5ogGifOiAXR4R5CI8DSpvkfMQxHimfm3u3dR
h57mbP18YQnFdEAKnXthdH6s4bUWFKjqjS0Qy0XYy7pLDXowirYz19hvmlgyMwju
NMYQGB+hlUB0pgPgllHGHtWXuI+Dn0OHOscll6DRo2RFeii/1ApQVE0OUxjbbdSL
Nn2yEsqQkb8FiSSGQ83oRZeABscMNgAbXcPZD9ytXiBGwFFKfATbxjc44kIROfZW
y9g7m9glc+CTyUJu8Pok
=1Zqx
-----END PGP SIGNATURE-----

[PATCH ethtool 5/5] ethtool: Support for configurable RSS hash function

[Amir Vadai]

From: Eyal Perry <eyalpe@mellanox.com>

This ethtool patch adds support to set and get the current RSS hash
function for the device through through the new hfunc mask field in the
ethtool_rxfh struct. Kernel supported hash function names are queried
with ETHTOOL_GSTRINGS - each string is corresponding with a bit in hfunc
mask according to its index in the string-set.

Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 ethtool.8.in |  7 +++++++
 ethtool.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index ae56293..bdc77e0 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -297,6 +297,8 @@ ethtool \- query or control network driver and hardware settings
 .BI weight\  W0
 .IR W1
 .RB ...\ ]
+.RB [ hfunc
+.IR FUNC ]
 .HP
 .B ethtool \-f|\-\-flash
 .I devname file
@@ -796,6 +798,11 @@ Sets RSS hash key of the specified network device. RSS hash key should be of dev
 Hash key format must be in xx:yy:zz:aa:bb:cc format meaning both the nibbles of a byte should be mentioned
 even if a nibble is zero.
 .TP
+.BI hfunc
+Sets RSS hash function of the specified network device. Requested hash function
+should be supported by the kernel and the device. List of RSS hash functions
+which kernel supports is shown as a part of the --show-rxfh comand output.
+.TP
 .BI equal\  N
 Sets the receive flow hash indirection table to spread flows evenly
 between the first \fIN\fR receive queues.
diff --git a/ethtool.c b/ethtool.c
index c2f4164..c584333 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3176,10 +3176,11 @@ static int do_grxfh(struct cmd_context *ctx)
 {
 	struct ethtool_rxfh rss_head = {0};
 	struct ethtool_rxnfc ring_count;
+	struct ethtool_gstrings *hfuncs;
 	struct ethtool_rxfh *rss;
 	u32 i, indir_bytes;
 	char *hkey;
-	int err;
+	int err, cur_len, max_len = 0;
 
 	ring_count.cmd = ETHTOOL_GRXRINGS;
 	err = send_ioctl(ctx, &ring_count);
@@ -3231,6 +3232,23 @@ static int do_grxfh(struct cmd_context *ctx)
 			printf("%02x:", (u8) hkey[i]);
 	}
 
+	printf("RSS hash function:\n");
+	if (!rss->hfunc) {
+		printf("    Operation not supported\n");
+		goto out;
+	}
+	hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
+	for (i = 0; i < hfuncs->len; i++) {
+		cur_len = strlen((const char *)hfuncs->data +
+				 i * ETH_GSTRING_LEN);
+		if (cur_len > max_len)
+			max_len = cur_len;
+	}
+	for (i = 0; i < hfuncs->len; i++)
+		printf("    %-*s: %s\n", max_len,
+		       (const char *)hfuncs->data + i * ETH_GSTRING_LEN,
+		       (rss->hfunc & (1 << i)) ? "on" : "off");
+out:
 	free(rss);
 	return 0;
 }
@@ -3330,12 +3348,15 @@ static int do_srxfh(struct cmd_context *ctx)
 	struct ethtool_rxfh rss_head = {0};
 	struct ethtool_rxfh *rss;
 	struct ethtool_rxnfc ring_count;
+	struct ethtool_gstrings *hfuncs;
 	int rxfhindir_equal = 0;
 	char **rxfhindir_weight = NULL;
 	char *rxfhindir_key = NULL;
+	char *req_hfunc_name = NULL;
+	char *hfunc_name = NULL;
 	char *hkey = NULL;
-	int err = 0;
-	u32 arg_num = 0, indir_bytes = 0;
+	int i, err = 0;
+	u32 arg_num = 0, indir_bytes = 0, req_hfunc = 0;
 	u32 entry_size = sizeof(rss_head.rss_config[0]);
 	u32 num_weights = 0;
 
@@ -3364,6 +3385,12 @@ static int do_srxfh(struct cmd_context *ctx)
 			if (!rxfhindir_key)
 				exit_bad_args();
 			++arg_num;
+		} else if (!strcmp(ctx->argp[arg_num], "hfunc")) {
+			++arg_num;
+			req_hfunc_name = ctx->argp[arg_num];
+			if (!req_hfunc_name)
+				exit_bad_args();
+			++arg_num;
 		} else {
 			exit_bad_args();
 		}
@@ -3384,7 +3411,8 @@ static int do_srxfh(struct cmd_context *ctx)
 
 	rss_head.cmd = ETHTOOL_GRSSH;
 	err = send_ioctl(ctx, &rss_head);
-	if (err < 0 && errno == EOPNOTSUPP && !rxfhindir_key) {
+	if (err < 0 && errno == EOPNOTSUPP &&
+	    !rxfhindir_key && !req_hfunc_name) {
 		return do_srxfhindir(ctx, rxfhindir_equal, rxfhindir_weight,
 				     num_weights);
 	} else if (err < 0) {
@@ -3402,6 +3430,22 @@ static int do_srxfh(struct cmd_context *ctx)
 	if (rxfhindir_equal || rxfhindir_weight)
 		indir_bytes = rss_head.indir_size * entry_size;
 
+	if (rss_head.hfunc && req_hfunc_name) {
+		hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
+		for (i = 0; i < hfuncs->len && !req_hfunc ; i++) {
+			hfunc_name = (char *)(hfuncs->data +
+					      i * ETH_GSTRING_LEN);
+			if (!strncmp(hfunc_name, req_hfunc_name,
+				     ETH_GSTRING_LEN))
+				req_hfunc = (u32)1 << i;
+		}
+		if (!req_hfunc) {
+			fprintf(stderr,
+				"Unknown hash function: %s\n", req_hfunc_name);
+			return 1;
+		}
+	}
+
 	rss = calloc(1, sizeof(*rss) + indir_bytes + rss_head.key_size);
 	if (!rss) {
 		perror("Cannot allocate memory for RX flow hash config");
@@ -3410,6 +3454,7 @@ static int do_srxfh(struct cmd_context *ctx)
 	rss->cmd = ETHTOOL_SRSSH;
 	rss->indir_size = rss_head.indir_size;
 	rss->key_size = rss_head.key_size;
+	rss->hfunc = req_hfunc ? req_hfunc : 0;
 
 	if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
 			     rxfhindir_weight, num_weights)) {
@@ -4119,7 +4164,8 @@ static const struct option {
 	{ "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
 	  "Set Rx flow hash indirection and/or hash key",
 	  "		[ equal N | weight W0 W1 ... ]\n"
-	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n" },
+	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n"
+	  "		[ hfunc FUNC ]\n"},
 	{ "-f|--flash", 1, do_flash,
 	  "Flash firmware image from the specified file to a region on the device",
 	  "               FILENAME [ REGION-NUMBER-TO-FLASH ]\n" },
-- 
1.9.3

Re: [PATCH ethtool 5/5] ethtool: Support for configurable RSS hash function

[Ben Hutchings]

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

On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
> From: Eyal Perry <eyalpe@mellanox.com>
> 
> This ethtool patch adds support to set and get the current RSS hash
> function for the device through through the new hfunc mask field in the
> ethtool_rxfh struct. Kernel supported hash function names are queried
> with ETHTOOL_GSTRINGS - each string is corresponding with a bit in hfunc
> mask according to its index in the string-set.
> 
> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>  ethtool.8.in |  7 +++++++
>  ethtool.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 58 insertions(+), 5 deletions(-)
> 
> diff --git a/ethtool.8.in b/ethtool.8.in
> index ae56293..bdc77e0 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -297,6 +297,8 @@ ethtool \- query or control network driver and hardware settings
>  .BI weight\  W0
>  .IR W1
>  .RB ...\ ]
> +.RB [ hfunc
> +.IR FUNC ]
>  .HP
>  .B ethtool \-f|\-\-flash
>  .I devname file
> @@ -796,6 +798,11 @@ Sets RSS hash key of the specified network device. RSS hash key should be of dev
>  Hash key format must be in xx:yy:zz:aa:bb:cc format meaning both the nibbles of a byte should be mentioned
>  even if a nibble is zero.
>  .TP
> +.BI hfunc
> +Sets RSS hash function of the specified network device. Requested hash function
> +should be supported by the kernel and the device.

I think the second sentence is unneeded - obviously requesting an
unsupported hash function will fail.

>  List of RSS hash functions

'A list of ...'

> +which kernel supports is shown as a part of the --show-rxfh comand output.

'... which the device supports ...' (since this depends on the hardware
and driver as well as the kernel).

> +.TP
>  .BI equal\  N
>  Sets the receive flow hash indirection table to spread flows evenly
>  between the first \fIN\fR receive queues.
> diff --git a/ethtool.c b/ethtool.c
> index c2f4164..c584333 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3176,10 +3176,11 @@ static int do_grxfh(struct cmd_context *ctx)
>  {
>  	struct ethtool_rxfh rss_head = {0};
>  	struct ethtool_rxnfc ring_count;
> +	struct ethtool_gstrings *hfuncs;
>  	struct ethtool_rxfh *rss;
>  	u32 i, indir_bytes;
>  	char *hkey;
> -	int err;
> +	int err, cur_len, max_len = 0;
>  
>  	ring_count.cmd = ETHTOOL_GRXRINGS;
>  	err = send_ioctl(ctx, &ring_count);
> @@ -3231,6 +3232,23 @@ static int do_grxfh(struct cmd_context *ctx)
>  			printf("%02x:", (u8) hkey[i]);
>  	}
>  
> +	printf("RSS hash function:\n");
> +	if (!rss->hfunc) {
> +		printf("    Operation not supported\n");
> +		goto out;
> +	}
> +	hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
> +	for (i = 0; i < hfuncs->len; i++) {
> +		cur_len = strlen((const char *)hfuncs->data +
> +				 i * ETH_GSTRING_LEN);
> +		if (cur_len > max_len)
> +			max_len = cur_len;
> +	}
> +	for (i = 0; i < hfuncs->len; i++)
> +		printf("    %-*s: %s\n", max_len,
> +		       (const char *)hfuncs->data + i * ETH_GSTRING_LEN,
> +		       (rss->hfunc & (1 << i)) ? "on" : "off");

I'm puzzled by this.  Do you remember why the hash functions were given
flag numbers rather than serial numbers?  Would it ever make sense to
have multiple hash functions enabled?

If only one hash function can be enabled (which seems to be true at
least for current drivers) then this only needs to print the one hash
function that is enabled.  (But then, the user needs to be able to list
the available hash functions somehow.)

If multiple hash functions can be enabled, then we should support that
in do_srxfh(), don't we?

> +out:
>  	free(rss);
>  	return 0;
>  }
[...]
> @@ -3410,6 +3454,7 @@ static int do_srxfh(struct cmd_context *ctx)
>  	rss->cmd = ETHTOOL_SRSSH;
>  	rss->indir_size = rss_head.indir_size;
>  	rss->key_size = rss_head.key_size;
> +	rss->hfunc = req_hfunc ? req_hfunc : 0;

That's a complicated way to write '= req_hfunc;'.

Ben.

>  	if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
>  			     rxfhindir_weight, num_weights)) {
> @@ -4119,7 +4164,8 @@ static const struct option {
>  	{ "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
>  	  "Set Rx flow hash indirection and/or hash key",
>  	  "		[ equal N | weight W0 W1 ... ]\n"
> -	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n" },
> +	  "		[ hkey %x:%x:%x:%x:%x:.... ]\n"
> +	  "		[ hfunc FUNC ]\n"},
>  	{ "-f|--flash", 1, do_flash,
>  	  "Flash firmware image from the specified file to a region on the device",
>  	  "               FILENAME [ REGION-NUMBER-TO-FLASH ]\n" },

-- 
Ben Hutchings
Quantity is no substitute for quality, but it's the only one we've got.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH ethtool 5/5] ethtool: Support for configurable RSS hash function

[Amir Vadai]

On Sun, Apr 5, 2015 at 5:55 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Sun, 2015-01-25 at 15:51 +0200, Amir Vadai wrote:
>> From: Eyal Perry <eyalpe@mellanox.com>
>>
>> This ethtool patch adds support to set and get the current RSS hash
>> function for the device through through the new hfunc mask field in the
>> ethtool_rxfh struct. Kernel supported hash function names are queried
>> with ETHTOOL_GSTRINGS - each string is corresponding with a bit in hfunc
>> mask according to its index in the string-set.
>>
>> Signed-off-by: Eyal Perry <eyalpe@mellanox.com>
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>  ethtool.8.in |  7 +++++++
>>  ethtool.c    | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 58 insertions(+), 5 deletions(-)
>>
>> diff --git a/ethtool.8.in b/ethtool.8.in
>> index ae56293..bdc77e0 100644
>> --- a/ethtool.8.in
>> +++ b/ethtool.8.in
>> @@ -297,6 +297,8 @@ ethtool \- query or control network driver and hardware settings
>>  .BI weight\  W0
>>  .IR W1
>>  .RB ...\ ]
>> +.RB [ hfunc
>> +.IR FUNC ]
>>  .HP
>>  .B ethtool \-f|\-\-flash
>>  .I devname file
>> @@ -796,6 +798,11 @@ Sets RSS hash key of the specified network device. RSS hash key should be of dev
>>  Hash key format must be in xx:yy:zz:aa:bb:cc format meaning both the nibbles of a byte should be mentioned
>>  even if a nibble is zero.
>>  .TP
>> +.BI hfunc
>> +Sets RSS hash function of the specified network device. Requested hash function
>> +should be supported by the kernel and the device.
>
> I think the second sentence is unneeded - obviously requesting an
> unsupported hash function will fail.
Will be fixed for V1

>
>>  List of RSS hash functions
>
> 'A list of ...'
Ack

>
>> +which kernel supports is shown as a part of the --show-rxfh comand output.
>
> '... which the device supports ...' (since this depends on the hardware
> and driver as well as the kernel).
Ack

>
>> +.TP
>>  .BI equal\  N
>>  Sets the receive flow hash indirection table to spread flows evenly
>>  between the first \fIN\fR receive queues.
>> diff --git a/ethtool.c b/ethtool.c
>> index c2f4164..c584333 100644
>> --- a/ethtool.c
>> +++ b/ethtool.c
>> @@ -3176,10 +3176,11 @@ static int do_grxfh(struct cmd_context *ctx)
>>  {
>>       struct ethtool_rxfh rss_head = {0};
>>       struct ethtool_rxnfc ring_count;
>> +     struct ethtool_gstrings *hfuncs;
>>       struct ethtool_rxfh *rss;
>>       u32 i, indir_bytes;
>>       char *hkey;
>> -     int err;
>> +     int err, cur_len, max_len = 0;
>>
>>       ring_count.cmd = ETHTOOL_GRXRINGS;
>>       err = send_ioctl(ctx, &ring_count);
>> @@ -3231,6 +3232,23 @@ static int do_grxfh(struct cmd_context *ctx)
>>                       printf("%02x:", (u8) hkey[i]);
>>       }
>>
>> +     printf("RSS hash function:\n");
>> +     if (!rss->hfunc) {
>> +             printf("    Operation not supported\n");
>> +             goto out;
>> +     }
>> +     hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
>> +     for (i = 0; i < hfuncs->len; i++) {
>> +             cur_len = strlen((const char *)hfuncs->data +
>> +                              i * ETH_GSTRING_LEN);
>> +             if (cur_len > max_len)
>> +                     max_len = cur_len;
>> +     }
>> +     for (i = 0; i < hfuncs->len; i++)
>> +             printf("    %-*s: %s\n", max_len,
>> +                    (const char *)hfuncs->data + i * ETH_GSTRING_LEN,
>> +                    (rss->hfunc & (1 << i)) ? "on" : "off");
>
> I'm puzzled by this.  Do you remember why the hash functions were given
> flag numbers rather than serial numbers?  Would it ever make sense to
> have multiple hash functions enabled?
I don't think it will make sense to have multiple hash function
enabled at the same time, but using flag numbers could be useful for
setting an attribute in addition to the hash function.
Currently there is no such use case that I can think of.

>
> If only one hash function can be enabled (which seems to be true at
> least for current drivers) then this only needs to print the one hash
> function that is enabled.  (But then, the user needs to be able to list
> the available hash functions somehow.)
>
> If multiple hash functions can be enabled, then we should support that
> in do_srxfh(), don't we?
Right, will fix it for V1

>
>> +out:
>>       free(rss);
>>       return 0;
>>  }
> [...]
>> @@ -3410,6 +3454,7 @@ static int do_srxfh(struct cmd_context *ctx)
>>       rss->cmd = ETHTOOL_SRSSH;
>>       rss->indir_size = rss_head.indir_size;
>>       rss->key_size = rss_head.key_size;
>> +     rss->hfunc = req_hfunc ? req_hfunc : 0;
>
> That's a complicated way to write '= req_hfunc;'.
Will fix in V1.

Thanks,
Amir
>
> Ben.
>
>>       if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
>>                            rxfhindir_weight, num_weights)) {
>> @@ -4119,7 +4164,8 @@ static const struct option {
>>       { "-X|--set-rxfh-indir|--rxfh", 1, do_srxfh,
>>         "Set Rx flow hash indirection and/or hash key",
>>         "             [ equal N | weight W0 W1 ... ]\n"
>> -       "             [ hkey %x:%x:%x:%x:%x:.... ]\n" },
>> +       "             [ hkey %x:%x:%x:%x:%x:.... ]\n"
>> +       "             [ hfunc FUNC ]\n"},
>>       { "-f|--flash", 1, do_flash,
>>         "Flash firmware image from the specified file to a region on the device",
>>         "               FILENAME [ REGION-NUMBER-TO-FLASH ]\n" },
>
> --
> Ben Hutchings
> Quantity is no substitute for quality, but it's the only one we've got.

Re: [PATCH ethtool 0/5] RSS improvements + new speeds

[Or Gerlitz]

On Sun, Jan 25, 2015 at 3:51 PM, Amir Vadai <amirv@mellanox.com> wrote:
> Hi Ben,
>
> This patchset contains some features and small bug fixes.
> A patch by Eyal Perry to enable setting RSS hash function, kernel support was
> added in commit 892311f ("ethtool: Support for configurable RSS hash
> function").
> Eyal Grossman added some missing advertised speeds and fixed some issues in
> this area of code.

Ben, Amir, where this series stands?

>
> Patches were tested and applied on top of commit 4614098 ("Release version 3.16.")
>
> Thanks,
> Amir
>
> Eyal Grossman (3):
>   ethtool-copy.h: sync with net
>   ethtool: Add missing Advertised speeds
>   ethtool: Return bad status when send_ioctl fails
>
> Eyal Perry (2):
>   ethtool: Prettify RX flow hash indirection table print
>   ethtool: Support for configurable RSS hash function
>
>  ethtool-copy.h |  67 +++++++++++++++++++++++++++----
>  ethtool.8.in   |   7 ++++
>  ethtool.c      | 122 ++++++++++++++++++++++++++++++++++++++++-----------------
>  3 files changed, 152 insertions(+), 44 deletions(-)
>
> --
> 1.9.3
>
> --
> 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

Re: [PATCH ethtool 0/5] RSS improvements + new speeds

[Amir Vadai]

On Thu, Mar 19, 2015 at 4:17 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Sun, Jan 25, 2015 at 3:51 PM, Amir Vadai <amirv@mellanox.com> wrote:
>> Hi Ben,
>>
>> This patchset contains some features and small bug fixes.
>> A patch by Eyal Perry to enable setting RSS hash function, kernel support was
>> added in commit 892311f ("ethtool: Support for configurable RSS hash
>> function").
>> Eyal Grossman added some missing advertised speeds and fixed some issues in
>> this area of code.
>
> Ben, Amir, where this series stands?
>

Hi Ben,

Is there something you need from us to make it in?

Amir