[PATCH v2 net 0/2] net: ethtool: fix rxfh_max_context_id

[PATCH v2 net 0/2] net: ethtool: fix rxfh_max_context_id

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Changes in v2:
* Removed unnecessary limit==0 check, since this can never happen
* Renamed rxfh_max_context_id to rxfh_max_num_contexts
* Incremented bnxt's max to keep user-facing behaviour the same
* Added patch #2 to validate the max is sane

Patch #2 doesn't have a fixes tag because it's strictly supererogatory;
 idk if it should go to net-next instead.

Edward Cree (2):
  net: ethtool: fix off-by-one error in max RSS context IDs
  net: ethtool: check rxfh_max_num_contexts != 1 at register time

 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  2 +-
 include/linux/ethtool.h                           | 10 +++++-----
 net/ethtool/common.c                              |  2 ++
 net/ethtool/ioctl.c                               |  5 +++--
 4 files changed, 11 insertions(+), 8 deletions(-)

[PATCH v2 net 1/2] net: ethtool: fix off-by-one error in max RSS context IDs

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

Both ethtool_ops.rxfh_max_context_id and the default value used when
 it's not specified are supposed to be exclusive maxima (the former
 is documented as such; the latter, U32_MAX, cannot be used as an ID
 since it equals ETH_RXFH_CONTEXT_ALLOC), but xa_alloc() expects an
 inclusive maximum.
Subtract one from 'limit' to produce an inclusive maximum, and pass
 that to xa_alloc().
Increase bnxt's max by one to prevent a (very minor) regression, as
 BNXT_MAX_ETH_RSS_CTX is an inclusive max.  This is safe since bnxt
 is not actually hard-limited; BNXT_MAX_ETH_RSS_CTX is just a
 leftover from old driver code that managed context IDs itself.
Rename rxfh_max_context_id to rxfh_max_num_contexts to make its
 semantics (hopefully) more obvious.

Fixes: 847a8ab18676 ("net: ethtool: let the core choose RSS context IDs")
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  2 +-
 include/linux/ethtool.h                           | 10 +++++-----
 net/ethtool/ioctl.c                               |  5 +++--
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index ab8e3f197e7b..9dadc89378f0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -5290,7 +5290,7 @@ void bnxt_ethtool_free(struct bnxt *bp)
 const struct ethtool_ops bnxt_ethtool_ops = {
 	.cap_link_lanes_supported	= 1,
 	.cap_rss_ctx_supported		= 1,
-	.rxfh_max_context_id		= BNXT_MAX_ETH_RSS_CTX,
+	.rxfh_max_num_contexts		= BNXT_MAX_ETH_RSS_CTX + 1,
 	.rxfh_indir_space		= BNXT_MAX_RSS_TABLE_ENTRIES_P5,
 	.rxfh_priv_size			= sizeof(struct bnxt_rss_ctx),
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 303fda54ef17..989c94eddb2b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -736,10 +736,10 @@ struct kernel_ethtool_ts_info {
  * @rxfh_key_space: same as @rxfh_indir_space, but for the key.
  * @rxfh_priv_size: size of the driver private data area the core should
  *	allocate for an RSS context (in &struct ethtool_rxfh_context).
- * @rxfh_max_context_id: maximum (exclusive) supported RSS context ID.  If this
- *	is zero then the core may choose any (nonzero) ID, otherwise the core
- *	will only use IDs strictly less than this value, as the @rss_context
- *	argument to @create_rxfh_context and friends.
+ * @rxfh_max_num_contexts: maximum (exclusive) supported RSS context ID.
+ *	If this is zero then the core may choose any (nonzero) ID, otherwise
+ *	the core will only use IDs strictly less than this value, as the
+ *	@rss_context argument to @create_rxfh_context and friends.
  * @supported_coalesce_params: supported types of interrupt coalescing.
  * @supported_ring_params: supported ring params.
  * @get_drvinfo: Report driver/device information. Modern drivers no
@@ -954,7 +954,7 @@ struct ethtool_ops {
 	u32	rxfh_indir_space;
 	u16	rxfh_key_space;
 	u16	rxfh_priv_size;
-	u32	rxfh_max_context_id;
+	u32	rxfh_max_num_contexts;
 	u32	supported_coalesce_params;
 	u32	supported_ring_params;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 8ca13208d240..a8e276ecf723 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1449,12 +1449,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		}
 
 		if (ops->create_rxfh_context) {
-			u32 limit = ops->rxfh_max_context_id ?: U32_MAX;
+			u32 limit = ops->rxfh_max_num_contexts ?: U32_MAX;
 			u32 ctx_id;
 
 			/* driver uses new API, core allocates ID */
 			ret = xa_alloc(&dev->ethtool->rss_ctx, &ctx_id, ctx,
-				       XA_LIMIT(1, limit), GFP_KERNEL_ACCOUNT);
+				       XA_LIMIT(1, limit - 1),
+				       GFP_KERNEL_ACCOUNT);
 			if (ret < 0) {
 				kfree(ctx);
 				goto out;

[PATCH v2 net 2/2] net: ethtool: check rxfh_max_num_contexts != 1 at register time

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

A value of 1 doesn't make sense, as it implies the only allowed
 context ID is 0, which is reserved for the default context - in
 which case the driver should just not claim to support custom
 RSS contexts at all.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 net/ethtool/common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 07032babd1b6..5f714bbbef19 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -654,6 +654,8 @@ int ethtool_check_ops(const struct ethtool_ops *ops)
 {
 	if (WARN_ON(ops->set_coalesce && !ops->supported_coalesce_params))
 		return -EINVAL;
+	if (WARN_ON(ops->rxfh_max_num_contexts == 1))
+		return -EINVAL;
 	/* NOTE: sufficiently insane drivers may swap ethtool_ops at runtime,
 	 * the fact that ops are checked at registration time does not
 	 * mean the ops attached to a netdev later on are sane.

Re: [PATCH v2 net 0/2] net: ethtool: fix rxfh_max_context_id

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 7 Aug 2024 17:06:11 +0100 you wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Changes in v2:
> * Removed unnecessary limit==0 check, since this can never happen
> * Renamed rxfh_max_context_id to rxfh_max_num_contexts
> * Incremented bnxt's max to keep user-facing behaviour the same
> * Added patch #2 to validate the max is sane
> 
> [...]

Here is the summary with links:
  - [v2,net,1/2] net: ethtool: fix off-by-one error in max RSS context IDs
    https://git.kernel.org/netdev/net/c/b54de55990b0
  - [v2,net,2/2] net: ethtool: check rxfh_max_num_contexts != 1 at register time
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 net 0/2] net: ethtool: fix rxfh_max_context_id

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 7 Aug 2024 17:06:11 +0100 you wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> Changes in v2:
> * Removed unnecessary limit==0 check, since this can never happen
> * Renamed rxfh_max_context_id to rxfh_max_num_contexts
> * Incremented bnxt's max to keep user-facing behaviour the same
> * Added patch #2 to validate the max is sane
> 
> [...]

Here is the summary with links:
  - [v2,net,1/2] net: ethtool: fix off-by-one error in max RSS context IDs
    (no matching commit)
  - [v2,net,2/2] net: ethtool: check rxfh_max_num_contexts != 1 at register time
    https://git.kernel.org/netdev/net-next/c/ceb627435b00

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html