[PATCH net 0/2] net/ethtool, mlx4_en: RSS hash function setting fixes

[PATCH net 0/2] net/ethtool, mlx4_en: RSS hash function setting fixes

[Amir Vadai]

Hi Dave,

This small patchset fixes a small issue in the ethtool User/kernel API.

Hash function is represented as an eight bit field, every bit represents a
function.  Currently possible values are: 1 for Toeplitz and 2 for XOR.
This commit changes the representation from bit flags into a value. Because
only one hash function will be used at a time, there is no sense using bit
flags for it.
Since today only 2 functions are supported, and there is no user space ethtool
that supports this feature, we can switch into values without causing any
backward compatibility issues.

After this patchset is applied, I will send the user space patch.

In this occasion I added a missing validation to the hash function value.

This patchset was applied and tested over commit 2ea2f62 ("net: fix crash in build_skb()")

Please push this patchset to stable 3.19.y in which those two issues were introduced.

Thanks,
Amir

Amir Vadai (2):
  ethtool: Use values instead of bit flags for RSS hash function
  net/mlx4_en: Prevent setting invalid RSS hash function

 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 29 ++++++++++++++-----------
 include/linux/ethtool.h                         |  9 ++------
 net/core/ethtool.c                              |  4 ++--
 3 files changed, 20 insertions(+), 22 deletions(-)

-- 
1.9.3

[PATCH net 1/2] ethtool: Use values instead of bit flags for RSS hash function

[Amir Vadai]

The RX indirection hash function is eight bits. Using more than one hash
function doesn't make sense, therefore using this field as a value
instead of a bit flag.
Moving from bit flags into value shouldn't introduce any backward
compatibility issues, since currently there are only 2 functions,
therfore the actual value in the field hfunc is the same like before 1
for Toeplitz and 2 for XOR.

Fixes: 892311f ("ethtool: Support for configurable RSS hash function")
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 include/linux/ethtool.h | 9 ++-------
 net/core/ethtool.c      | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 653dc9c..d19f31a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -60,8 +60,8 @@ enum ethtool_phys_id_state {
 };
 
 enum {
-	ETH_RSS_HASH_TOP_BIT, /* Configurable RSS hash function - Toeplitz */
-	ETH_RSS_HASH_XOR_BIT, /* Configurable RSS hash function - Xor */
+	ETH_RSS_HASH_TOP = 1, /* Configurable RSS hash function - Toeplitz */
+	ETH_RSS_HASH_XOR = 2, /* Configurable RSS hash function - Xor */
 
 	/*
 	 * Add your fresh new hash function bits above and remember to update
@@ -70,11 +70,6 @@ enum {
 	ETH_RSS_HASH_FUNCS_COUNT
 };
 
-#define __ETH_RSS_HASH_BIT(bit)	((u32)1 << (bit))
-#define __ETH_RSS_HASH(name)	__ETH_RSS_HASH_BIT(ETH_RSS_HASH_##name##_BIT)
-
-#define ETH_RSS_HASH_TOP	__ETH_RSS_HASH(TOP)
-#define ETH_RSS_HASH_XOR	__ETH_RSS_HASH(XOR)
 
 #define ETH_RSS_HASH_UNKNOWN	0
 #define ETH_RSS_HASH_NO_CHANGE	0
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1d00b89..1a1db95 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -103,8 +103,8 @@ static const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN]
 
 static const char
 rss_hash_func_strings[ETH_RSS_HASH_FUNCS_COUNT][ETH_GSTRING_LEN] = {
-	[ETH_RSS_HASH_TOP_BIT] =	"toeplitz",
-	[ETH_RSS_HASH_XOR_BIT] =	"xor",
+	[ETH_RSS_HASH_TOP] =		"toeplitz",
+	[ETH_RSS_HASH_XOR] =		"xor",
 };
 
 static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
-- 
1.9.3

[PATCH net 2/2] net/mlx4_en: Prevent setting invalid RSS hash function

[Amir Vadai]

mlx4_en_check_rxfh_func() was checking for hardware support before
setting a known RSS hash function, but didn't do any check before
setting unknown RSS hash function. Need to make it fail on such values.
In this occasion, moved the actual setting of the new value from the
check function into mlx4_en_set_rxfh().

Fixes: 947cbb0 ("net/mlx4_en: Support for configurable RSS hash function")
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 29 ++++++++++++++-----------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 3f44e2b..a2ddf3d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1102,20 +1102,21 @@ static int mlx4_en_check_rxfh_func(struct net_device *dev, u8 hfunc)
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 
 	/* check if requested function is supported by the device */
-	if ((hfunc == ETH_RSS_HASH_TOP &&
-	     !(priv->mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP)) ||
-	    (hfunc == ETH_RSS_HASH_XOR &&
-	     !(priv->mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR)))
-		return -EINVAL;
+	if (hfunc == ETH_RSS_HASH_TOP) {
+		if (!(priv->mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_TOP))
+			return -EINVAL;
+		if (!(dev->features & NETIF_F_RXHASH))
+			en_warn(priv, "Toeplitz hash function should be used in conjunction with RX hashing for optimal performance\n");
+		return 0;
+	} else if (hfunc == ETH_RSS_HASH_XOR) {
+		if (!(priv->mdev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS_XOR))
+			return -EINVAL;
+		if (dev->features & NETIF_F_RXHASH)
+			en_warn(priv, "Enabling both XOR Hash function and RX Hashing can limit RPS functionality\n");
+		return 0;
+	}
 
-	priv->rss_hash_fn = hfunc;
-	if (hfunc == ETH_RSS_HASH_TOP && !(dev->features & NETIF_F_RXHASH))
-		en_warn(priv,
-			"Toeplitz hash function should be used in conjunction with RX hashing for optimal performance\n");
-	if (hfunc == ETH_RSS_HASH_XOR && (dev->features & NETIF_F_RXHASH))
-		en_warn(priv,
-			"Enabling both XOR Hash function and RX Hashing can limit RPS functionality\n");
-	return 0;
+	return -EINVAL;
 }
 
 static int mlx4_en_get_rxfh(struct net_device *dev, u32 *ring_index, u8 *key,
@@ -1189,6 +1190,8 @@ static int mlx4_en_set_rxfh(struct net_device *dev, const u32 *ring_index,
 		priv->prof->rss_rings = rss_rings;
 	if (key)
 		memcpy(priv->rss_key, key, MLX4_EN_RSS_KEY_SIZE);
+	if (hfunc !=  ETH_RSS_HASH_NO_CHANGE)
+		priv->rss_hash_fn = hfunc;
 
 	if (port_up) {
 		err = mlx4_en_start_port(dev);
-- 
1.9.3

Re: [PATCH net 0/2] net/ethtool, mlx4_en: RSS hash function setting fixes

[Or Gerlitz]

On Sun, Apr 26, 2015 at 5:36 PM, Amir Vadai <amirv@mellanox.com> wrote:
> Hi Dave,
>
> This small patchset fixes a small issue in the ethtool User/kernel API.
>
> Hash function is represented as an eight bit field, every bit represents a
> function.  Currently possible values are: 1 for Toeplitz and 2 for XOR.
> This commit changes the representation from bit flags into a value. Because
> only one hash function will be used at a time, there is no sense using bit
> flags for it.
> Since today only 2 functions are supported, and there is no user space ethtool
> that supports this feature, we can switch into values without causing any
> backward compatibility issues.
>
> After this patchset is applied, I will send the user space patch.
>
> In this occasion I added a missing validation to the hash function value.
>
> This patchset was applied and tested over commit 2ea2f62 ("net: fix crash in build_skb()")
>
> Please push this patchset to stable 3.19.y in which those two issues were introduced.

and 4.0.y too I guess?

Re: [PATCH net 0/2] net/ethtool, mlx4_en: RSS hash function setting fixes

[David Miller]

From: Amir Vadai <amirv@mellanox.com>
Date: Sun, 26 Apr 2015 17:36:04 +0300

> Hash function is represented as an eight bit field, every bit represents a
> function.  Currently possible values are: 1 for Toeplitz and 2 for XOR.
> This commit changes the representation from bit flags into a value. Because
> only one hash function will be used at a time, there is no sense using bit
> flags for it.

If we ever want to add some kind of control attributes that might
apply to any hash, we'll want flags again.

You can just enforce that only one of these hash function bits can be
set at one time, and be done with it.

Re: [PATCH net 0/2] net/ethtool, mlx4_en: RSS hash function setting fixes

[Amir Vadai]

On 4/27/2015 6:02 AM, David Miller wrote:
> From: Amir Vadai <amirv@mellanox.com>
> Date: Sun, 26 Apr 2015 17:36:04 +0300
> 
>> Hash function is represented as an eight bit field, every bit represents a
>> function.  Currently possible values are: 1 for Toeplitz and 2 for XOR.
>> This commit changes the representation from bit flags into a value. Because
>> only one hash function will be used at a time, there is no sense using bit
>> flags for it.
> 
> If we ever want to add some kind of control attributes that might
> apply to any hash, we'll want flags again.
> 
> You can just enforce that only one of these hash function bits can be
> set at one time, and be done with it.
> 
Ok, re-spinning with the fix to mlx4_en only.