DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net/ring: remove internal devarg
@ 2026-09-22 11:36 Bruce Richardson
  2026-09-22 13:34 ` Stephen Hemminger
  2026-09-22 14:24 ` [PATCH v2] " Bruce Richardson
  0 siblings, 2 replies; 6+ messages in thread
From: Bruce Richardson @ 2026-09-22 11:36 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, stable

The devarg "internal" was used to pass a pointer to the creation
function to reuse set parameters. However, this meant it could also be
used as a devarg on the commandline leading to incorrect behaviour.
Redesign the passing of the args parameters to use a per-thread global
instead. The wrapper function sets the global to a valid value before
calling the main creation function, skipping passing a devarg, before
resetting the pointer back to NULL when the function returns.

Bugzilla ID: 1687
Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/ring/rte_eth_ring.c | 144 ++++++++++++--------------------
 1 file changed, 53 insertions(+), 91 deletions(-)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index b639544eab..d5530bcfec 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -11,6 +11,7 @@
 #include <rte_malloc.h>
 #include <rte_memcpy.h>
 #include <rte_os_shim.h>
+#include <rte_per_lcore.h>
 #include <rte_string_fns.h>
 #include <bus_vdev_driver.h>
 #include <rte_kvargs.h>
@@ -20,12 +21,9 @@
 #define ETH_RING_ACTION_CREATE		"CREATE"
 #define ETH_RING_ACTION_ATTACH		"ATTACH"
 #define ETH_RING_ACTION_MAX_LEN		8 /* CREATE | ACTION */
-#define ETH_RING_INTERNAL_ARG		"internal"
-#define ETH_RING_INTERNAL_ARG_MAX_LEN	19 /* "0x..16chars..\0" */
 
 static const char *valid_arguments[] = {
 	ETH_RING_NUMA_NODE_ACTION_ARG,
-	ETH_RING_INTERNAL_ARG,
 	NULL
 };
 
@@ -35,9 +33,11 @@ struct ring_internal_args {
 	struct rte_ring * const *tx_queues;
 	const unsigned int nb_tx_queues;
 	const unsigned int numa_node;
-	void *addr; /* self addr for sanity check */
 };
 
+/* rte_eth_from_rings() stashes a pointer to its on-stack args here  */
+static RTE_DEFINE_PER_LCORE(struct ring_internal_args *, eth_ring_internal_args);
+
 enum dev_action {
 	DEV_CREATE,
 	DEV_ATTACH
@@ -474,9 +474,7 @@ rte_eth_from_rings(const char *name, struct rte_ring *const rx_queues[],
 		.tx_queues = tx_queues,
 		.nb_tx_queues = nb_tx_queues,
 		.numa_node = numa_node,
-		.addr = &args,
 	};
-	char args_str[32];
 	char ring_name[RTE_RING_NAMESIZE];
 	uint16_t port_id = RTE_MAX_ETHPORTS;
 	int ret;
@@ -495,16 +493,16 @@ rte_eth_from_rings(const char *name, struct rte_ring *const rx_queues[],
 		return -1;
 	}
 
-	snprintf(args_str, sizeof(args_str), "%s=%p",
-		 ETH_RING_INTERNAL_ARG, &args);
-
 	ret = snprintf(ring_name, sizeof(ring_name), "net_ring_%s", name);
 	if (ret >= (int)sizeof(ring_name)) {
 		rte_errno = ENAMETOOLONG;
 		return -1;
 	}
 
-	ret = rte_vdev_init(ring_name, args_str);
+	/* mark this thread's pending request; consumed and cleared by probe */
+	RTE_PER_LCORE(eth_ring_internal_args) = &args;
+	ret = rte_vdev_init(ring_name, NULL);
+	RTE_PER_LCORE(eth_ring_internal_args) = NULL;
 	if (ret) {
 		rte_errno = EINVAL;
 		return -1;
@@ -649,36 +647,6 @@ static int parse_kvlist(const char *key __rte_unused,
 	return ret;
 }
 
-static int
-parse_internal_args(const char *key __rte_unused, const char *value,
-		void *data)
-{
-	struct ring_internal_args **internal_args = data;
-	void *args;
-	int ret, n;
-
-	/* make sure 'value' is valid pointer length */
-	if (strnlen(value, ETH_RING_INTERNAL_ARG_MAX_LEN) >=
-			ETH_RING_INTERNAL_ARG_MAX_LEN) {
-		PMD_LOG(ERR, "Error parsing internal args, argument is too long");
-		return -1;
-	}
-
-	ret = sscanf(value, "%p%n", &args, &n);
-	if (ret == 0 || (size_t)n != strlen(value)) {
-		PMD_LOG(ERR, "Error parsing internal args");
-
-		return -1;
-	}
-
-	*internal_args = args;
-
-	if ((*internal_args)->addr != args)
-		return -1;
-
-	return 0;
-}
-
 static int
 rte_pmd_ring_probe(struct rte_vdev_device *dev)
 {
@@ -711,6 +679,20 @@ rte_pmd_ring_probe(struct rte_vdev_device *dev)
 		return 0;
 	}
 
+	/* set only by rte_eth_from_rings(), never derived from user devargs */
+	internal_args = RTE_PER_LCORE(eth_ring_internal_args);
+	if (internal_args != NULL) {
+		ret = do_eth_dev_ring_create(name, dev,
+			internal_args->rx_queues,
+			internal_args->nb_rx_queues,
+			internal_args->tx_queues,
+			internal_args->nb_tx_queues,
+			internal_args->numa_node,
+			DEV_ATTACH,
+			&eth_dev);
+		return ret >= 0 ? 0 : ret;
+	}
+
 	if (params == NULL || params[0] == '\0') {
 		ret = eth_dev_ring_create(name, dev, rte_socket_id(), DEV_CREATE,
 				&eth_dev);
@@ -739,57 +721,37 @@ rte_pmd_ring_probe(struct rte_vdev_device *dev)
 			return ret;
 		}
 
-		if (rte_kvargs_count(kvlist, ETH_RING_INTERNAL_ARG) == 1) {
-			ret = rte_kvargs_process(kvlist, ETH_RING_INTERNAL_ARG,
-						 parse_internal_args,
-						 &internal_args);
-			if (ret < 0)
-				goto out_free;
-
-			ret = do_eth_dev_ring_create(name, dev,
-				internal_args->rx_queues,
-				internal_args->nb_rx_queues,
-				internal_args->tx_queues,
-				internal_args->nb_tx_queues,
-				internal_args->numa_node,
-				DEV_ATTACH,
-				&eth_dev);
-			if (ret >= 0)
-				ret = 0;
-		} else {
-			ret = rte_kvargs_count(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG);
-			info = rte_zmalloc("struct node_action_list",
-					   sizeof(struct node_action_list) +
-					   (sizeof(struct node_action_pair) * ret),
-					   0);
-			if (!info)
-				goto out_free;
-
-			info->total = ret;
-			info->list = (struct node_action_pair *)(info + 1);
-
-			ret = rte_kvargs_process(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG,
-						 parse_kvlist, info);
-
-			if (ret < 0)
-				goto out_free;
-
-			for (info->count = 0; info->count < info->total; info->count++) {
-				ret = eth_dev_ring_create(info->list[info->count].name,
-							  dev,
-							  info->list[info->count].node,
-							  info->list[info->count].action,
-							  &eth_dev);
-				if ((ret == -1) &&
-				    (info->list[info->count].action == DEV_CREATE)) {
-					PMD_LOG(INFO,
-						"Attach to pmd_ring for %s",
-						name);
-					ret = eth_dev_ring_create(name, dev,
-							info->list[info->count].node,
-							DEV_ATTACH,
-							&eth_dev);
-				}
+		ret = rte_kvargs_count(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG);
+		info = rte_zmalloc("struct node_action_list",
+				   sizeof(struct node_action_list) +
+				   (sizeof(struct node_action_pair) * ret),
+				   0);
+		if (!info)
+			goto out_free;
+
+		info->total = ret;
+		info->list = (struct node_action_pair *)(info + 1);
+
+		ret = rte_kvargs_process(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG,
+					 parse_kvlist, info);
+
+		if (ret < 0)
+			goto out_free;
+
+		for (info->count = 0; info->count < info->total; info->count++) {
+			ret = eth_dev_ring_create(info->list[info->count].name,
+						  dev,
+						  info->list[info->count].node,
+						  info->list[info->count].action,
+						  &eth_dev);
+			if (ret == -1 && info->list[info->count].action == DEV_CREATE) {
+				PMD_LOG(INFO,
+					"Attach to pmd_ring for %s",
+					name);
+				ret = eth_dev_ring_create(name, dev,
+						info->list[info->count].node,
+						DEV_ATTACH,
+						&eth_dev);
 			}
 		}
 	}
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/ring: remove internal devarg
  2026-09-22 11:36 [PATCH] net/ring: remove internal devarg Bruce Richardson
@ 2026-09-22 13:34 ` Stephen Hemminger
  2026-09-22 14:09   ` Bruce Richardson
  2026-09-22 14:24 ` [PATCH v2] " Bruce Richardson
  1 sibling, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2026-09-22 13:34 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, stable

On Tue, 22 Sep 2026 12:36:14 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The devarg "internal" was used to pass a pointer to the creation
> function to reuse set parameters. However, this meant it could also be
> used as a devarg on the commandline leading to incorrect behaviour.
> Redesign the passing of the args parameters to use a per-thread global
> instead. The wrapper function sets the global to a valid value before
> calling the main creation function, skipping passing a devarg, before
> resetting the pointer back to NULL when the function returns.
> 
> Bugzilla ID: 1687
> Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Minor nits from Opus review:

Review: [PATCH] net/ring: remove internal devarg
Patchwork: 169914

Applies cleanly to main, builds with -Dwerror=true. Fixes tag hash
96cb19521147 verified. Removing a user-reachable devarg that is
dereferenced as a raw pointer is the right fix.

Warning

The per-lcore pointer is still set while user callbacks run.
do_eth_dev_ring_create() calls rte_eth_dev_probing_finish(), which
fires RTE_ETH_EVENT_NEW callbacks synchronously on the same thread.
If a callback calls rte_vdev_init("net_ring_foo", "nodeaction=...")
the nested probe sees eth_ring_internal_args != NULL, ignores the
user's devargs, and attaches net_ring_foo to the outer caller's
rings.

Clear the pointer in probe right after reading it:

	internal_args = RTE_PER_LCORE(eth_ring_internal_args);
	RTE_PER_LCORE(eth_ring_internal_args) = NULL;
	if (internal_args != NULL) {

Keep the reset after rte_vdev_init() in rte_eth_from_rings() to
cover the paths where probe is never reached (-EEXIST, no driver
match). This also makes the comment "consumed and cleared by probe"
true; as written, probe does not clear it.

Alternatively add the vdev name to struct ring_internal_args and
require a match in probe. Either works; clearing is simpler.

Info

No test coverage. app/test/test_pmd_ring.c could add a case that
"net_ring_x,internal=0x1" no longer dereferences the value (it now
falls into the "Ignoring unsupported parameters" path).

Double space before "*/" in the comment above
RTE_DEFINE_PER_LCORE.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/ring: remove internal devarg
  2026-09-22 13:34 ` Stephen Hemminger
@ 2026-09-22 14:09   ` Bruce Richardson
  2026-09-22 14:25     ` Bruce Richardson
  0 siblings, 1 reply; 6+ messages in thread
From: Bruce Richardson @ 2026-09-22 14:09 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, stable

On Tue, Sep 22, 2026 at 06:34:01AM -0700, Stephen Hemminger wrote:
> On Tue, 22 Sep 2026 12:36:14 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > The devarg "internal" was used to pass a pointer to the creation
> > function to reuse set parameters. However, this meant it could also be
> > used as a devarg on the commandline leading to incorrect behaviour.
> > Redesign the passing of the args parameters to use a per-thread global
> > instead. The wrapper function sets the global to a valid value before
> > calling the main creation function, skipping passing a devarg, before
> > resetting the pointer back to NULL when the function returns.
> > 
> > Bugzilla ID: 1687
> > Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> 
> Minor nits from Opus review:
> 
> Review: [PATCH] net/ring: remove internal devarg
> Patchwork: 169914
> 
> Applies cleanly to main, builds with -Dwerror=true. Fixes tag hash
> 96cb19521147 verified. Removing a user-reachable devarg that is
> dereferenced as a raw pointer is the right fix.
> 
> Warning
> 
> The per-lcore pointer is still set while user callbacks run.
> do_eth_dev_ring_create() calls rte_eth_dev_probing_finish(), which
> fires RTE_ETH_EVENT_NEW callbacks synchronously on the same thread.
> If a callback calls rte_vdev_init("net_ring_foo", "nodeaction=...")
> the nested probe sees eth_ring_internal_args != NULL, ignores the
> user's devargs, and attaches net_ring_foo to the outer caller's
> rings.
> 

That's an unrealistic scenario, is it not? Having an ethdev creation
callback which creates another ethdev just leads to an infinite chain of
callbacks.

> Clear the pointer in probe right after reading it:
> 
> 	internal_args = RTE_PER_LCORE(eth_ring_internal_args);
> 	RTE_PER_LCORE(eth_ring_internal_args) = NULL;
> 	if (internal_args != NULL) {
> 
> Keep the reset after rte_vdev_init() in rte_eth_from_rings() to
> cover the paths where probe is never reached (-EEXIST, no driver
> match). This also makes the comment "consumed and cleared by probe"
> true; as written, probe does not clear it.
> 
> Alternatively add the vdev name to struct ring_internal_args and
> require a match in probe. Either works; clearing is simpler.
> 
> Info
> 
> No test coverage. app/test/test_pmd_ring.c could add a case that
> "net_ring_x,internal=0x1" no longer dereferences the value (it now
> falls into the "Ignoring unsupported parameters" path).
> 

Since there is no "internal" parameter any more, I don't see this as being
necessary.

> Double space before "*/" in the comment above
> RTE_DEFINE_PER_LCORE.

Something that can be fixed on apply, I think, if it's a concern.

/Bruce

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] net/ring: remove internal devarg
  2026-09-22 11:36 [PATCH] net/ring: remove internal devarg Bruce Richardson
  2026-09-22 13:34 ` Stephen Hemminger
@ 2026-09-22 14:24 ` Bruce Richardson
  2026-09-22 15:32   ` Stephen Hemminger
  1 sibling, 1 reply; 6+ messages in thread
From: Bruce Richardson @ 2026-09-22 14:24 UTC (permalink / raw)
  To: dev; +Cc: stephen, Bruce Richardson, stable

The devarg "internal" was used to pass a pointer to the creation
function to reuse set parameters. However, this meant it could also be
used as a devarg on the commandline leading to incorrect behaviour.
Redesign the passing of the args parameters to use a per-thread global
instead. The wrapper function sets the global to a valid value before
calling the main creation function, skipping passing a devarg, before
resetting the pointer back to NULL when the function returns.

Bugzilla ID: 1687
Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v2
* clear the args settings immediately after read, in case of a
  reentrant call
* minor comment cleanups
---
 drivers/net/ring/rte_eth_ring.c | 145 ++++++++++++--------------------
 1 file changed, 54 insertions(+), 91 deletions(-)

diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index b639544eab..3b476b8587 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -11,6 +11,7 @@
 #include <rte_malloc.h>
 #include <rte_memcpy.h>
 #include <rte_os_shim.h>
+#include <rte_per_lcore.h>
 #include <rte_string_fns.h>
 #include <bus_vdev_driver.h>
 #include <rte_kvargs.h>
@@ -20,12 +21,9 @@
 #define ETH_RING_ACTION_CREATE		"CREATE"
 #define ETH_RING_ACTION_ATTACH		"ATTACH"
 #define ETH_RING_ACTION_MAX_LEN		8 /* CREATE | ACTION */
-#define ETH_RING_INTERNAL_ARG		"internal"
-#define ETH_RING_INTERNAL_ARG_MAX_LEN	19 /* "0x..16chars..\0" */
 
 static const char *valid_arguments[] = {
 	ETH_RING_NUMA_NODE_ACTION_ARG,
-	ETH_RING_INTERNAL_ARG,
 	NULL
 };
 
@@ -35,9 +33,11 @@ struct ring_internal_args {
 	struct rte_ring * const *tx_queues;
 	const unsigned int nb_tx_queues;
 	const unsigned int numa_node;
-	void *addr; /* self addr for sanity check */
 };
 
+/* rte_eth_from_rings() stashes a pointer to its on-stack args here */
+static RTE_DEFINE_PER_LCORE(struct ring_internal_args *, eth_ring_internal_args);
+
 enum dev_action {
 	DEV_CREATE,
 	DEV_ATTACH
@@ -474,9 +474,7 @@ rte_eth_from_rings(const char *name, struct rte_ring *const rx_queues[],
 		.tx_queues = tx_queues,
 		.nb_tx_queues = nb_tx_queues,
 		.numa_node = numa_node,
-		.addr = &args,
 	};
-	char args_str[32];
 	char ring_name[RTE_RING_NAMESIZE];
 	uint16_t port_id = RTE_MAX_ETHPORTS;
 	int ret;
@@ -495,16 +493,16 @@ rte_eth_from_rings(const char *name, struct rte_ring *const rx_queues[],
 		return -1;
 	}
 
-	snprintf(args_str, sizeof(args_str), "%s=%p",
-		 ETH_RING_INTERNAL_ARG, &args);
-
 	ret = snprintf(ring_name, sizeof(ring_name), "net_ring_%s", name);
 	if (ret >= (int)sizeof(ring_name)) {
 		rte_errno = ENAMETOOLONG;
 		return -1;
 	}
 
-	ret = rte_vdev_init(ring_name, args_str);
+	/* mark this thread's pending request; consumed and cleared by probe */
+	RTE_PER_LCORE(eth_ring_internal_args) = &args;
+	ret = rte_vdev_init(ring_name, NULL);
+	RTE_PER_LCORE(eth_ring_internal_args) = NULL;
 	if (ret) {
 		rte_errno = EINVAL;
 		return -1;
@@ -649,36 +647,6 @@ static int parse_kvlist(const char *key __rte_unused,
 	return ret;
 }
 
-static int
-parse_internal_args(const char *key __rte_unused, const char *value,
-		void *data)
-{
-	struct ring_internal_args **internal_args = data;
-	void *args;
-	int ret, n;
-
-	/* make sure 'value' is valid pointer length */
-	if (strnlen(value, ETH_RING_INTERNAL_ARG_MAX_LEN) >=
-			ETH_RING_INTERNAL_ARG_MAX_LEN) {
-		PMD_LOG(ERR, "Error parsing internal args, argument is too long");
-		return -1;
-	}
-
-	ret = sscanf(value, "%p%n", &args, &n);
-	if (ret == 0 || (size_t)n != strlen(value)) {
-		PMD_LOG(ERR, "Error parsing internal args");
-
-		return -1;
-	}
-
-	*internal_args = args;
-
-	if ((*internal_args)->addr != args)
-		return -1;
-
-	return 0;
-}
-
 static int
 rte_pmd_ring_probe(struct rte_vdev_device *dev)
 {
@@ -711,6 +679,21 @@ rte_pmd_ring_probe(struct rte_vdev_device *dev)
 		return 0;
 	}
 
+	/* set only by rte_eth_from_rings() */
+	internal_args = RTE_PER_LCORE(eth_ring_internal_args);
+	if (internal_args != NULL) {
+		RTE_PER_LCORE(eth_ring_internal_args) = NULL;
+		ret = do_eth_dev_ring_create(name, dev,
+			internal_args->rx_queues,
+			internal_args->nb_rx_queues,
+			internal_args->tx_queues,
+			internal_args->nb_tx_queues,
+			internal_args->numa_node,
+			DEV_ATTACH,
+			&eth_dev);
+		return ret >= 0 ? 0 : ret;
+	}
+
 	if (params == NULL || params[0] == '\0') {
 		ret = eth_dev_ring_create(name, dev, rte_socket_id(), DEV_CREATE,
 				&eth_dev);
@@ -739,57 +722,37 @@ rte_pmd_ring_probe(struct rte_vdev_device *dev)
 			return ret;
 		}
 
-		if (rte_kvargs_count(kvlist, ETH_RING_INTERNAL_ARG) == 1) {
-			ret = rte_kvargs_process(kvlist, ETH_RING_INTERNAL_ARG,
-						 parse_internal_args,
-						 &internal_args);
-			if (ret < 0)
-				goto out_free;
-
-			ret = do_eth_dev_ring_create(name, dev,
-				internal_args->rx_queues,
-				internal_args->nb_rx_queues,
-				internal_args->tx_queues,
-				internal_args->nb_tx_queues,
-				internal_args->numa_node,
-				DEV_ATTACH,
-				&eth_dev);
-			if (ret >= 0)
-				ret = 0;
-		} else {
-			ret = rte_kvargs_count(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG);
-			info = rte_zmalloc("struct node_action_list",
-					   sizeof(struct node_action_list) +
-					   (sizeof(struct node_action_pair) * ret),
-					   0);
-			if (!info)
-				goto out_free;
-
-			info->total = ret;
-			info->list = (struct node_action_pair *)(info + 1);
-
-			ret = rte_kvargs_process(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG,
-						 parse_kvlist, info);
-
-			if (ret < 0)
-				goto out_free;
-
-			for (info->count = 0; info->count < info->total; info->count++) {
-				ret = eth_dev_ring_create(info->list[info->count].name,
-							  dev,
-							  info->list[info->count].node,
-							  info->list[info->count].action,
-							  &eth_dev);
-				if ((ret == -1) &&
-				    (info->list[info->count].action == DEV_CREATE)) {
-					PMD_LOG(INFO,
-						"Attach to pmd_ring for %s",
-						name);
-					ret = eth_dev_ring_create(name, dev,
-							info->list[info->count].node,
-							DEV_ATTACH,
-							&eth_dev);
-				}
+		ret = rte_kvargs_count(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG);
+		info = rte_zmalloc("struct node_action_list",
+				   sizeof(struct node_action_list) +
+				   (sizeof(struct node_action_pair) * ret),
+				   0);
+		if (!info)
+			goto out_free;
+
+		info->total = ret;
+		info->list = (struct node_action_pair *)(info + 1);
+
+		ret = rte_kvargs_process(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG,
+					 parse_kvlist, info);
+
+		if (ret < 0)
+			goto out_free;
+
+		for (info->count = 0; info->count < info->total; info->count++) {
+			ret = eth_dev_ring_create(info->list[info->count].name,
+						  dev,
+						  info->list[info->count].node,
+						  info->list[info->count].action,
+						  &eth_dev);
+			if (ret == -1 && info->list[info->count].action == DEV_CREATE) {
+				PMD_LOG(INFO,
+					"Attach to pmd_ring for %s",
+					name);
+				ret = eth_dev_ring_create(name, dev,
+						info->list[info->count].node,
+						DEV_ATTACH,
+						&eth_dev);
 			}
 		}
 	}
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] net/ring: remove internal devarg
  2026-09-22 14:09   ` Bruce Richardson
@ 2026-09-22 14:25     ` Bruce Richardson
  0 siblings, 0 replies; 6+ messages in thread
From: Bruce Richardson @ 2026-09-22 14:25 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, stable

On Tue, Sep 22, 2026 at 03:09:59PM +0100, Bruce Richardson wrote:
> On Tue, Sep 22, 2026 at 06:34:01AM -0700, Stephen Hemminger wrote:
> > On Tue, 22 Sep 2026 12:36:14 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > > The devarg "internal" was used to pass a pointer to the creation
> > > function to reuse set parameters. However, this meant it could also be
> > > used as a devarg on the commandline leading to incorrect behaviour.
> > > Redesign the passing of the args parameters to use a per-thread global
> > > instead. The wrapper function sets the global to a valid value before
> > > calling the main creation function, skipping passing a devarg, before
> > > resetting the pointer back to NULL when the function returns.
> > > 
> > > Bugzilla ID: 1687
> > > Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > 
> > Minor nits from Opus review:
> > 
> > Review: [PATCH] net/ring: remove internal devarg
> > Patchwork: 169914
> > 
> > Applies cleanly to main, builds with -Dwerror=true. Fixes tag hash
> > 96cb19521147 verified. Removing a user-reachable devarg that is
> > dereferenced as a raw pointer is the right fix.
> > 
> > Warning
> > 
> > The per-lcore pointer is still set while user callbacks run.
> > do_eth_dev_ring_create() calls rte_eth_dev_probing_finish(), which
> > fires RTE_ETH_EVENT_NEW callbacks synchronously on the same thread.
> > If a callback calls rte_vdev_init("net_ring_foo", "nodeaction=...")
> > the nested probe sees eth_ring_internal_args != NULL, ignores the
> > user's devargs, and attaches net_ring_foo to the outer caller's
> > rings.
> > 
> 
> That's an unrealistic scenario, is it not? Having an ethdev creation
> callback which creates another ethdev just leads to an infinite chain of
> callbacks.
> 

Since it's a one-line change, decided to fix in v2 anyway.

> > Clear the pointer in probe right after reading it:
> > 
> > 	internal_args = RTE_PER_LCORE(eth_ring_internal_args);
> > 	RTE_PER_LCORE(eth_ring_internal_args) = NULL;
> > 	if (internal_args != NULL) {
> > 
> > Keep the reset after rte_vdev_init() in rte_eth_from_rings() to
> > cover the paths where probe is never reached (-EEXIST, no driver
> > match). This also makes the comment "consumed and cleared by probe"
> > true; as written, probe does not clear it.
> > 
> > Alternatively add the vdev name to struct ring_internal_args and
> > require a match in probe. Either works; clearing is simpler.
> > 
> > Info
> > 
> > No test coverage. app/test/test_pmd_ring.c could add a case that
> > "net_ring_x,internal=0x1" no longer dereferences the value (it now
> > falls into the "Ignoring unsupported parameters" path).
> > 
> 
> Since there is no "internal" parameter any more, I don't see this as being
> necessary.
> 
> > Double space before "*/" in the comment above
> > RTE_DEFINE_PER_LCORE.
> 
> Something that can be fixed on apply, I think, if it's a concern.
> 
Fixed also in v2.

/Bruce

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] net/ring: remove internal devarg
  2026-09-22 14:24 ` [PATCH v2] " Bruce Richardson
@ 2026-09-22 15:32   ` Stephen Hemminger
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2026-09-22 15:32 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, stable

On Tue, 22 Sep 2026 15:24:11 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The devarg "internal" was used to pass a pointer to the creation
> function to reuse set parameters. However, this meant it could also be
> used as a devarg on the commandline leading to incorrect behaviour.
> Redesign the passing of the args parameters to use a per-thread global
> instead. The wrapper function sets the global to a valid value before
> calling the main creation function, skipping passing a devarg, before
> resetting the pointer back to NULL when the function returns.
> 
> Bugzilla ID: 1687
> Fixes: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Looks good, applied to next-net.

Did manual validation that internal arg was gone.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-22 15:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-22 11:36 [PATCH] net/ring: remove internal devarg Bruce Richardson
2026-09-22 13:34 ` Stephen Hemminger
2026-09-22 14:09   ` Bruce Richardson
2026-09-22 14:25     ` Bruce Richardson
2026-09-22 14:24 ` [PATCH v2] " Bruce Richardson
2026-09-22 15:32   ` Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox