Re: [PATCH v6 net-next 8/9] net: ethtool: use the tracking array for get_rxfh on custom RSS contexts

On 20/06/2024 20:42, Simon Horman wrote:

On Thu, Jun 20, 2024 at 06:47:11AM +0100, edward.cree@amd.com wrote:

quoted

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

On 'ethtool -x' with rss_context != 0, instead of calling the driver to
 read the RSS settings for the context, just get the settings from the
 rss_ctx xarray, and return them to the user with no driver involvement.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 net/ethtool/ioctl.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 9d2d677770db..ac562ee3662e 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c

@@ -1199,6 +1199,7 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	struct ethtool_rxfh_param rxfh_dev = {};
 	u32 user_indir_size, user_key_size;
+	struct ethtool_rxfh_context *ctx;
 	struct ethtool_rxfh rxfh;
 	u32 indir_bytes;
 	u8 *rss_config;

@@ -1246,11 +1247,25 @@ static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	if (user_key_size)
 		rxfh_dev.key = rss_config + indir_bytes;
 
-	rxfh_dev.rss_context = rxfh.rss_context;
-
-	ret = dev->ethtool_ops->get_rxfh(dev, &rxfh_dev);
-	if (ret)
-		goto out;
+	if (rxfh.rss_context) {
+		ctx = xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context);
+		if (!ctx) {
+			ret = -ENOENT;
+			goto out;
+		}
+		if (rxfh_dev.indir)
+			memcpy(rxfh_dev.indir, ethtool_rxfh_context_indir(ctx),
+			       indir_bytes);
+		if (rxfh_dev.key)
+			memcpy(rxfh_dev.key, ethtool_rxfh_context_key(ctx),
+			       user_key_size);
+		rxfh_dev.hfunc = ctx->hfunc;
+		rxfh_dev.input_xfrm = ctx->input_xfrm;

Hi Edward,

The last line of this function is:

	return ret;

With this patch applied, Smatch complains that ret may be used there
when unintialised.

I think that occurs when the code reaches the line where this
commentary has been placed in this email.

You (and Smatch) are quite right.  Fixed for v7.
-ed

quoted

+	} else {
+		ret = dev->ethtool_ops->get_rxfh(dev, &rxfh_dev);
+		if (ret)
+			goto out;
+	}
 
 	if (copy_to_user(useraddr + offsetof(struct ethtool_rxfh, hfunc),
 			 &rxfh_dev.hfunc, sizeof(rxfh.hfunc))) {