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 <[email protected]> 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: [email protected]
> > >
> > > Signed-off-by: Bruce Richardson <[email protected]>
> > > ---
> >
> > 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
