Re: [PATCH v2] net/bnxt: fix global table scope shutdown order

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun,  4 Oct 2026 09:20:37 -0500
Manish Kurup <manish.kurup@broadcom.com> wrote:

> From: Farah Smith <farah.smith@broadcom.com>
> 
> Track per-port table-scope teardown state (scope type and, for GLOBAL
> scope, a shared FID reference count) and clean up in the correct
> order on bnxt_en/DPDK shutdown.
> 
> Previously, unloading the L2 kernel driver with DPDK shutdown could
> crash because table-scope resources were torn down without properly
> tracking which FID was the last user of a GLOBAL scope. Fix this by
> introducing glb_tbl_scope_fid_cnt bookkeeping and freeing the per-port
> CPM before removing the FID from the scope, and only fully retiring
> shared GLOBAL-scope memory once the reference count reaches zero.
> 
> The teardown order was initially fid_rem -> mem_free, but that
> sequence was found to trigger PXP errors: fid_rem disables the scope
> in firmware, so any subsequent mem_free on that scope is operating on
> an already-disabled scope. Reordered to cpm_free -> fid_rem -> mem_free
> so the scope is only disabled and its memory released after this
> port's CPM is torn down. The relative order of fid_rem and mem_free
> is unchanged from before; the new step is cpm_free running first.
> 
> Fixes: 23e0dc62d19e ("net/bnxt/tf_core: add global table scope")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Farah Smith <farah.smith@broadcom.com>
> Signed-off-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
> Signed-off-by: Manish Kurup <manish.kurup@broadcom.com>
> ---

AI finds lots of issues here.
My gut feeling is you are using AI and not looking enough at the underlying
design and architecture problem. AI will build a house of cards if you ask it
and it will not tell you what you are trying to do is wrong.

You need to rethink the ownership model.


[PATCH v2] net/bnxt: fix global table scope shutdown order
patchwork 170526

Errors
------

1. flow_db_lock used outside its lifetime

ulp_tfc_tbl_scope_deinit() now takes the fdb lock:

	if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {

but ulp_tfc_deinit(), which runs for the last port in a session,
destroys that mutex just before calling it:

	pthread_mutex_destroy(&bp->ulp_ctx->cfg_data->flow_db_lock);

	ulp_tfc_tbl_scope_deinit(bp);

Locking a destroyed mutex is undefined. On glibc it fails with
EINVAL, so the last port always takes the "conservative" branch and
mem_free() is called with fid_cnt 1. The last user of the GLOBAL
scope, the case this patch is for, never retires it. Before the
patch that port passed the firmware count from fid_rem.

Init has the mirror problem. ulp_tfc_init() calls
ulp_tfc_tbl_scope_init() before

	rte_thread_mutex_init_shared(&bp->ulp_ctx->cfg_data->flow_db_lock);

so the first port locks a mutex that has not been initialized. It
works only because zeroed rte_zmalloc() memory matches the default
initializer on glibc.

Call ulp_tfc_tbl_scope_deinit() before pthread_mutex_destroy() in
ulp_tfc_deinit(), and initialize flow_db_lock before
ulp_tfc_tbl_scope_init() in ulp_tfc_init().

2. Reference taken per port, tracked per session

tbl_scope_type and glb_tbl_scope_fid_cnt are added to struct
bnxt_ulp_data, i.e. cfg_data, shared by every port in the session.
Deinit drops a reference based on the shared type, not on whether
this port took one:

	} else if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
		rc = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(bp->ulp_ctx);

ulp_tfc_ctx_attach() does ref_cnt++ before ulp_tfc_tbl_scope_init().
If a second port fails after bnxt_ulp_cntxt_tsid_set() (config state
timeout, mem_alloc, cpm_alloc), bnxt_ulp_port_deinit() sends it
through ulp_ctx_detach() to ulp_tfc_tbl_scope_deinit(). It sees the
GLOBAL type set by the first port and decrements the first port's
reference to 0. mem_free() then gets fid_cnt 0 and, on a VF, resets
the global tsid state while the first port is still using it.

Keep the per-port state in struct bnxt_ulp_context next to tsid,
e.g. a flag set when this port incremented the count, and decrement
only when it is set. The commit message already calls this per-port
state.

Warnings
--------

3. Counter scope does not match the state it protects

The GLOBAL tsid state that mem_free() resets is tfc_global in
tf_core/v3/tfo.c, one per process. The new count is in cfg_data, one
per session, and ulp_get_session() keys sessions on PCI domain and
bus unless BNXT_FLAGS2_MULTIROOT_EN is set. Two ports on different
buses using the global scope each run their own count to 0, and the
first to close resets state the other still uses.

The patch also discards the firmware count:

	rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);

The HSI defines fid_rem's fid_cnt as the FIDs remaining in the scope.
The commit message needs to say why a driver-side count replaces it,
and the count needs to live at the same scope as tfc_global.

4. v2 rollback is not needed and is wrong

The rollback is reached only if bnxt_ulp_cntxt_acquire_fdb_lock() or
bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc() fails. Once the lock is
held, ulp_ctx and cfg_data are non-NULL, so the increment cannot
fail. If it is reached:

- v1 did not leak the CPM. Both callers of ulp_tfc_tbl_scope_init()
  fall into an error path that runs ulp_tfc_tbl_scope_deinit() with
  tsid still set, so with the rollback cpm_free, fid_rem and mem_free
  run twice.

- mem_free(..., 0) tells tf_core this is the last FID. The comment
  "only FID in scope (glb_tbl_scope_fid_cnt_inc never ran)" does not
  follow: this port not incrementing says nothing about ports already
  attached (first == false).

- The order is cpm_free, mem_free, fid_rem, not the order deinit and
  the commit message say is required.

- return -1 discards rc.

Drop the rollback and return rc as v1 did.

5. Commit message and comments do not match the change

The third paragraph says fid_rem -> mem_free causes PXP errors
because fid_rem disables the scope, then gives cpm_free -> fid_rem ->
mem_free as the fix, which still has fid_rem -> mem_free. What moved
is cpm_free ahead of fid_rem; say why cpm_free must run before
firmware deconfigures the scope.

On a PF, tfc_tbl_scope_mem_free() ignores fid_cnt and uses the TPM
pool count, so for PFs the reorder is the whole fix. The count only
matters on the VF GLOBAL path; say so.

The comments are off the same way:

	/* Free this port's CPM before mem_free; mem_free invalidates tsid scope state */

	/* Still attempt mem_free and fid_rem to avoid FW/driver state divergence. */

The first names the wrong ordering constraint. The second sits after
fid_rem has already run.

Info
----

6. Six non-static accessors are added for two fields used only in
bnxt_ulp_tfc.c, and bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_set() has no
caller. Every call is made with the fdb lock held, so the NULL checks
cannot fire; they only create the unreachable failure branches in
init and deinit. Access the fields directly under the lock, or make
the helpers static.

7. In ulp_tfc_tbl_scope_deinit() every "goto cleanup" is taken with
have_scope false, so the label and flag are a plain return. scope_rc
and the "our_fid_cnt = 0" branch are also redundant.

Review-Result: ERROR