* [PATCH] net/bnxt: fix global table scope shutdown order
@ 2026-10-01 17:40 Manish Kurup
2026-10-04 14:20 ` [PATCH v2] " Manish Kurup
0 siblings, 1 reply; 3+ messages in thread
From: Manish Kurup @ 2026-10-01 17:40 UTC (permalink / raw)
To: dev; +Cc: kishore.padmanabha, Farah Smith, stable
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, with fid_rem no longer preceding mem_free.
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>
---
drivers/net/bnxt/tf_ulp/bnxt_ulp.h | 3 +
drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c | 162 ++++++++++++++++++++++---
drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h | 23 ++++
3 files changed, 172 insertions(+), 16 deletions(-)
diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp.h b/drivers/net/bnxt/tf_ulp/bnxt_ulp.h
index afd883df2c..ba526e838d 100644
--- a/drivers/net/bnxt/tf_ulp/bnxt_ulp.h
+++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp.h
@@ -15,6 +15,7 @@
#include "rte_mtr.h"
#include "bnxt.h"
+#include "cfa_types.h"
#include "ulp_template_db_enum.h"
#include "ulp_tun.h"
#include "bnxt_tf_common.h"
@@ -115,6 +116,8 @@ struct bnxt_ulp_vfr_rule_info {
struct bnxt_ulp_data {
uint32_t tbl_scope_id;
+ enum cfa_scope_type tbl_scope_type; /* for deinit */
+ uint16_t glb_tbl_scope_fid_cnt; /* only for GLOBAL scope */
struct bnxt_ulp_mark_tbl *mark_tbl;
uint32_t dev_id; /* Hardware device id */
uint32_t ref_cnt;
diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c
index d5ce4c3c18..dcd64fa598 100644
--- a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c
+++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c
@@ -111,6 +111,64 @@ bnxt_ulp_cntxt_tbl_scope_max_pools_set(struct bnxt_ulp_context *ulp_ctx,
return 0;
}
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_get(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type *scope_type)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL || scope_type == NULL)
+ return -EINVAL;
+ *scope_type = ulp_ctx->cfg_data->tbl_scope_type;
+ return 0;
+}
+
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_set(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type scope_type)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ ulp_ctx->cfg_data->tbl_scope_type = scope_type;
+ return 0;
+}
+
+uint16_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(struct bnxt_ulp_context *ulp_ctx)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return 0;
+ return ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt;
+}
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_set(struct bnxt_ulp_context *ulp_ctx,
+ uint16_t fid_cnt)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt = fid_cnt;
+ return 0;
+}
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc(struct bnxt_ulp_context *ulp_ctx)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt++;
+ return 0;
+}
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(struct bnxt_ulp_context *ulp_ctx)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ if (ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt == 0)
+ return -EINVAL;
+ ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt--;
+ return 0;
+}
+
enum tfc_tbl_scope_bucket_factor
bnxt_ulp_cntxt_em_mulitplier_get(struct bnxt_ulp_context *ulp_ctx)
{
@@ -299,39 +357,82 @@ ulp_tfc_dparms_init(struct bnxt *bp,
static void
ulp_tfc_tbl_scope_deinit(struct bnxt *bp)
{
- uint16_t fid = 0, fid_cnt = 0;
- struct tfc *tfcp;
+ uint16_t fid = 0;
+ uint16_t our_fid_cnt = 0;
+ struct tfc *tfcp = NULL;
uint8_t tsid = 0;
int32_t rc;
+ enum cfa_scope_type scope_type = CFA_SCOPE_TYPE_INVALID;
+ int32_t scope_rc;
+ bool have_scope = false;
tfcp = bnxt_ulp_cntxt_tfcp_get(bp->ulp_ctx);
if (tfcp == NULL)
- return;
+ goto cleanup;
rc = bnxt_ulp_cntxt_tsid_get(bp->ulp_ctx, &tsid);
- if (unlikely(rc))
- BNXT_DRV_DBG(ERR, "Failed to get the table scope\n");
+ if (rc) {
+ BNXT_DRV_DBG(ERR, "tsid_get failed rc=%d, skipping table-scope deinit", rc);
+ goto cleanup;
+ }
rc = bnxt_ulp_cntxt_fid_get(bp->ulp_ctx, &fid);
- if (rc)
+ if (rc) {
+ BNXT_DRV_DBG(ERR, "fid_get failed rc=%d, skipping table-scope deinit", rc);
+ goto cleanup;
+ }
+
+ have_scope = true;
+
+ if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {
+ BNXT_DRV_DBG(ERR, "acquire_fdb_lock failed, proceeding with teardown using conservative fid_cnt");
+ our_fid_cnt = 1; /* Conservative: avoid invalidating shared scope in mem_free */
+ } else {
+ scope_rc = bnxt_ulp_cntxt_tbl_scope_type_get(bp->ulp_ctx, &scope_type);
+ if (scope_rc) {
+ BNXT_DRV_DBG(ERR,
+ "tbl_scope_type_get failed rc=%d, proceeding with teardown using conservative fid_cnt",
+ scope_rc);
+ our_fid_cnt = 1; /* avoid invalidating shared scope in mem_free */
+ } else if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
+ rc = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(bp->ulp_ctx);
+ if (rc) {
+ BNXT_DRV_DBG(WARNING,
+ "glb_tbl_scope_fid_cnt dec failed (e.g. already 0), continuing teardown TSID:%d FID:%d",
+ tsid, fid);
+ /* Pass 1 so mem_free won't treat as last FID & invalidate scope */
+ our_fid_cnt = 1;
+ } else {
+ our_fid_cnt = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(bp->ulp_ctx);
+ }
+ } else {
+ our_fid_cnt = 0;
+ }
+ bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
+ }
+
+cleanup:
+ if (!have_scope)
return;
- rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, &fid_cnt);
+ /* Free this port's CPM before mem_free; mem_free invalidates tsid scope state */
+ rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
if (rc)
- BNXT_DRV_DBG(ERR, "Failed removing FID from TSID:%d FID:%d",
+ BNXT_DRV_DBG(ERR, "Failed Freeing CPM TSID:%d FID:%d",
tsid, fid);
else
- BNXT_DRV_DBG(DEBUG, "Removed FID from TSID:%d FID:%d",
- tsid, fid);
+ BNXT_DRV_DBG(DEBUG, "Freed CPM TSID:%d FID: %d", tsid, fid);
- rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
+ rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);
if (rc)
- BNXT_DRV_DBG(ERR, "Failed Freeing CPM TSID:%d FID:%d",
+ BNXT_DRV_DBG(ERR, "Failed removing FID from TSID:%d FID:%d",
tsid, fid);
else
- BNXT_DRV_DBG(DEBUG, "Freed CPM TSID:%d FID: %d", tsid, fid);
+ BNXT_DRV_DBG(DEBUG, "Removed FID from TSID:%d FID:%d, remaining FID count:%d",
+ tsid, fid, our_fid_cnt);
- rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, fid_cnt);
+ /* Still attempt mem_free and fid_rem to avoid FW/driver state divergence. */
+ rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, our_fid_cnt);
if (rc)
BNXT_DRV_DBG(ERR, "Failed freeing tscope mem TSID:%d FID:%d",
tsid, fid);
@@ -506,13 +607,42 @@ ulp_tfc_tbl_scope_init(struct bnxt *bp)
cparms.max_pools = max_pools;
rc = tfc_tbl_scope_cpm_alloc(tfcp, tsid, &cparms);
- if (rc)
+ if (rc) {
BNXT_DRV_DBG(ERR, "Failed to allocate CPM TSID:%d FID:%d\n",
tsid, fid);
- else
+ } else {
BNXT_DRV_DBG(DEBUG, "Allocated CPM TSID:%d FID:%d\n", tsid, fid);
+ /* Inc before setting type so type==GLOBAL never without count incremented. */
+ if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {
+ BNXT_DRV_DBG(ERR, "acquire_fdb_lock failed after CPM alloc, rolling back");
+ goto rollback;
+ }
+ if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
+ rc = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc(bp->ulp_ctx);
+ if (rc) {
+ BNXT_DRV_DBG(ERR, "Failed to increment glb_tbl_scope_fid_cnt");
+ bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
+ return rc;
+ }
+ }
+ rc = bnxt_ulp_cntxt_tbl_scope_type_set(bp->ulp_ctx, scope_type);
+ bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
+ }
return rc;
+
+rollback:
+ /* Rollback: only FID in scope (glb_tbl_scope_fid_cnt_inc never ran). */
+ rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
+ if (rc)
+ BNXT_DRV_DBG(INFO, "Rollback: cpm_free failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
+ rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, 0);
+ if (rc)
+ BNXT_DRV_DBG(INFO, "Rollback: mem_free failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
+ rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);
+ if (rc)
+ BNXT_DRV_DBG(INFO, "Rollback: fid_rem failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
+ return -1;
}
static int32_t
diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h
index ab6608ac74..2b73043ab8 100644
--- a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h
+++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h
@@ -7,6 +7,7 @@
#define _BNXT_ULP_TFC_H_
#include "bnxt.h"
+#include "cfa_types.h"
#include <inttypes.h>
bool
@@ -24,6 +25,28 @@ bnxt_ulp_cntxt_tbl_scope_max_pools_get(struct bnxt_ulp_context *ulp_ctx);
int32_t
bnxt_ulp_cntxt_tbl_scope_max_pools_set(struct bnxt_ulp_context *ulp_ctx,
uint32_t max);
+
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_get(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type *scope_type);
+
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_set(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type scope_type);
+
+uint16_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(struct bnxt_ulp_context *ulp_ctx);
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_set(struct bnxt_ulp_context *ulp_ctx,
+ uint16_t fid_cnt);
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc(struct bnxt_ulp_context *ulp_ctx);
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(struct bnxt_ulp_context *ulp_ctx);
+
enum tfc_tbl_scope_bucket_factor
bnxt_ulp_cntxt_em_mulitplier_get(struct bnxt_ulp_context *ulp_ctx);
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v2] net/bnxt: fix global table scope shutdown order
2026-10-01 17:40 [PATCH] net/bnxt: fix global table scope shutdown order Manish Kurup
@ 2026-10-04 14:20 ` Manish Kurup
2026-10-05 20:52 ` Stephen Hemminger
0 siblings, 1 reply; 3+ messages in thread
From: Manish Kurup @ 2026-10-04 14:20 UTC (permalink / raw)
To: dev; +Cc: kishore.padmanabha, Farah Smith, stable
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>
---
v2:
* Fix a CPM leak: the glb_tbl_scope_fid_cnt_inc() failure path in
ulp_tfc_tbl_scope_init() returned directly instead of going through
the rollback path, leaking the CPM just allocated a few lines above.
Changed to "goto rollback", matching the existing rollback handling
used by the other failure path in the same function.
* Fix an inaccurate claim in this commit message: the body previously
said "fid_rem no longer preceding mem_free", but fid_rem's position
relative to mem_free is unchanged from before this patch; only
cpm_free running first is new. Reworded for accuracy.
drivers/net/bnxt/tf_ulp/bnxt_ulp.h | 3 +
drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c | 162 ++++++++++++++++++++++---
drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h | 23 ++++
3 files changed, 172 insertions(+), 16 deletions(-)
diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp.h b/drivers/net/bnxt/tf_ulp/bnxt_ulp.h
index afd883df2c..ba526e838d 100644
--- a/drivers/net/bnxt/tf_ulp/bnxt_ulp.h
+++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp.h
@@ -15,6 +15,7 @@
#include "rte_mtr.h"
#include "bnxt.h"
+#include "cfa_types.h"
#include "ulp_template_db_enum.h"
#include "ulp_tun.h"
#include "bnxt_tf_common.h"
@@ -115,6 +116,8 @@ struct bnxt_ulp_vfr_rule_info {
struct bnxt_ulp_data {
uint32_t tbl_scope_id;
+ enum cfa_scope_type tbl_scope_type; /* for deinit */
+ uint16_t glb_tbl_scope_fid_cnt; /* only for GLOBAL scope */
struct bnxt_ulp_mark_tbl *mark_tbl;
uint32_t dev_id; /* Hardware device id */
uint32_t ref_cnt;
diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c
index d5ce4c3c18..5ef15ef7e1 100644
--- a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c
+++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c
@@ -111,6 +111,64 @@ bnxt_ulp_cntxt_tbl_scope_max_pools_set(struct bnxt_ulp_context *ulp_ctx,
return 0;
}
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_get(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type *scope_type)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL || scope_type == NULL)
+ return -EINVAL;
+ *scope_type = ulp_ctx->cfg_data->tbl_scope_type;
+ return 0;
+}
+
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_set(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type scope_type)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ ulp_ctx->cfg_data->tbl_scope_type = scope_type;
+ return 0;
+}
+
+uint16_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(struct bnxt_ulp_context *ulp_ctx)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return 0;
+ return ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt;
+}
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_set(struct bnxt_ulp_context *ulp_ctx,
+ uint16_t fid_cnt)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt = fid_cnt;
+ return 0;
+}
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc(struct bnxt_ulp_context *ulp_ctx)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt++;
+ return 0;
+}
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(struct bnxt_ulp_context *ulp_ctx)
+{
+ if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
+ return -EINVAL;
+ if (ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt == 0)
+ return -EINVAL;
+ ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt--;
+ return 0;
+}
+
enum tfc_tbl_scope_bucket_factor
bnxt_ulp_cntxt_em_mulitplier_get(struct bnxt_ulp_context *ulp_ctx)
{
@@ -299,39 +357,82 @@ ulp_tfc_dparms_init(struct bnxt *bp,
static void
ulp_tfc_tbl_scope_deinit(struct bnxt *bp)
{
- uint16_t fid = 0, fid_cnt = 0;
- struct tfc *tfcp;
+ uint16_t fid = 0;
+ uint16_t our_fid_cnt = 0;
+ struct tfc *tfcp = NULL;
uint8_t tsid = 0;
int32_t rc;
+ enum cfa_scope_type scope_type = CFA_SCOPE_TYPE_INVALID;
+ int32_t scope_rc;
+ bool have_scope = false;
tfcp = bnxt_ulp_cntxt_tfcp_get(bp->ulp_ctx);
if (tfcp == NULL)
- return;
+ goto cleanup;
rc = bnxt_ulp_cntxt_tsid_get(bp->ulp_ctx, &tsid);
- if (unlikely(rc))
- BNXT_DRV_DBG(ERR, "Failed to get the table scope\n");
+ if (rc) {
+ BNXT_DRV_DBG(ERR, "tsid_get failed rc=%d, skipping table-scope deinit", rc);
+ goto cleanup;
+ }
rc = bnxt_ulp_cntxt_fid_get(bp->ulp_ctx, &fid);
- if (rc)
+ if (rc) {
+ BNXT_DRV_DBG(ERR, "fid_get failed rc=%d, skipping table-scope deinit", rc);
+ goto cleanup;
+ }
+
+ have_scope = true;
+
+ if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {
+ BNXT_DRV_DBG(ERR, "acquire_fdb_lock failed, proceeding with teardown using conservative fid_cnt");
+ our_fid_cnt = 1; /* Conservative: avoid invalidating shared scope in mem_free */
+ } else {
+ scope_rc = bnxt_ulp_cntxt_tbl_scope_type_get(bp->ulp_ctx, &scope_type);
+ if (scope_rc) {
+ BNXT_DRV_DBG(ERR,
+ "tbl_scope_type_get failed rc=%d, proceeding with teardown using conservative fid_cnt",
+ scope_rc);
+ our_fid_cnt = 1; /* avoid invalidating shared scope in mem_free */
+ } else if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
+ rc = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(bp->ulp_ctx);
+ if (rc) {
+ BNXT_DRV_DBG(WARNING,
+ "glb_tbl_scope_fid_cnt dec failed (e.g. already 0), continuing teardown TSID:%d FID:%d",
+ tsid, fid);
+ /* Pass 1 so mem_free won't treat as last FID & invalidate scope */
+ our_fid_cnt = 1;
+ } else {
+ our_fid_cnt = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(bp->ulp_ctx);
+ }
+ } else {
+ our_fid_cnt = 0;
+ }
+ bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
+ }
+
+cleanup:
+ if (!have_scope)
return;
- rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, &fid_cnt);
+ /* Free this port's CPM before mem_free; mem_free invalidates tsid scope state */
+ rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
if (rc)
- BNXT_DRV_DBG(ERR, "Failed removing FID from TSID:%d FID:%d",
+ BNXT_DRV_DBG(ERR, "Failed Freeing CPM TSID:%d FID:%d",
tsid, fid);
else
- BNXT_DRV_DBG(DEBUG, "Removed FID from TSID:%d FID:%d",
- tsid, fid);
+ BNXT_DRV_DBG(DEBUG, "Freed CPM TSID:%d FID: %d", tsid, fid);
- rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
+ rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);
if (rc)
- BNXT_DRV_DBG(ERR, "Failed Freeing CPM TSID:%d FID:%d",
+ BNXT_DRV_DBG(ERR, "Failed removing FID from TSID:%d FID:%d",
tsid, fid);
else
- BNXT_DRV_DBG(DEBUG, "Freed CPM TSID:%d FID: %d", tsid, fid);
+ BNXT_DRV_DBG(DEBUG, "Removed FID from TSID:%d FID:%d, remaining FID count:%d",
+ tsid, fid, our_fid_cnt);
- rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, fid_cnt);
+ /* Still attempt mem_free and fid_rem to avoid FW/driver state divergence. */
+ rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, our_fid_cnt);
if (rc)
BNXT_DRV_DBG(ERR, "Failed freeing tscope mem TSID:%d FID:%d",
tsid, fid);
@@ -506,13 +607,42 @@ ulp_tfc_tbl_scope_init(struct bnxt *bp)
cparms.max_pools = max_pools;
rc = tfc_tbl_scope_cpm_alloc(tfcp, tsid, &cparms);
- if (rc)
+ if (rc) {
BNXT_DRV_DBG(ERR, "Failed to allocate CPM TSID:%d FID:%d\n",
tsid, fid);
- else
+ } else {
BNXT_DRV_DBG(DEBUG, "Allocated CPM TSID:%d FID:%d\n", tsid, fid);
+ /* Inc before setting type so type==GLOBAL never without count incremented. */
+ if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {
+ BNXT_DRV_DBG(ERR, "acquire_fdb_lock failed after CPM alloc, rolling back");
+ goto rollback;
+ }
+ if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
+ rc = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc(bp->ulp_ctx);
+ if (rc) {
+ BNXT_DRV_DBG(ERR, "Failed to increment glb_tbl_scope_fid_cnt");
+ bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
+ goto rollback;
+ }
+ }
+ rc = bnxt_ulp_cntxt_tbl_scope_type_set(bp->ulp_ctx, scope_type);
+ bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
+ }
return rc;
+
+rollback:
+ /* Rollback: only FID in scope (glb_tbl_scope_fid_cnt_inc never ran). */
+ rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
+ if (rc)
+ BNXT_DRV_DBG(INFO, "Rollback: cpm_free failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
+ rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, 0);
+ if (rc)
+ BNXT_DRV_DBG(INFO, "Rollback: mem_free failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
+ rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);
+ if (rc)
+ BNXT_DRV_DBG(INFO, "Rollback: fid_rem failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
+ return -1;
}
static int32_t
diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h
index ab6608ac74..2b73043ab8 100644
--- a/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h
+++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.h
@@ -7,6 +7,7 @@
#define _BNXT_ULP_TFC_H_
#include "bnxt.h"
+#include "cfa_types.h"
#include <inttypes.h>
bool
@@ -24,6 +25,28 @@ bnxt_ulp_cntxt_tbl_scope_max_pools_get(struct bnxt_ulp_context *ulp_ctx);
int32_t
bnxt_ulp_cntxt_tbl_scope_max_pools_set(struct bnxt_ulp_context *ulp_ctx,
uint32_t max);
+
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_get(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type *scope_type);
+
+int32_t
+bnxt_ulp_cntxt_tbl_scope_type_set(struct bnxt_ulp_context *ulp_ctx,
+ enum cfa_scope_type scope_type);
+
+uint16_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(struct bnxt_ulp_context *ulp_ctx);
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_set(struct bnxt_ulp_context *ulp_ctx,
+ uint16_t fid_cnt);
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc(struct bnxt_ulp_context *ulp_ctx);
+
+int32_t
+bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(struct bnxt_ulp_context *ulp_ctx);
+
enum tfc_tbl_scope_bucket_factor
bnxt_ulp_cntxt_em_mulitplier_get(struct bnxt_ulp_context *ulp_ctx);
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] net/bnxt: fix global table scope shutdown order
2026-10-04 14:20 ` [PATCH v2] " Manish Kurup
@ 2026-10-05 20:52 ` Stephen Hemminger
0 siblings, 0 replies; 3+ messages in thread
From: Stephen Hemminger @ 2026-10-05 20:52 UTC (permalink / raw)
To: Manish Kurup; +Cc: dev, kishore.padmanabha, Farah Smith, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-05 20:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-01 17:40 [PATCH] net/bnxt: fix global table scope shutdown order Manish Kurup
2026-10-04 14:20 ` [PATCH v2] " Manish Kurup
2026-10-05 20:52 ` Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox