|WARNING| net/bnxt: fix global table scope shutdown order

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170488

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-01

# Review of net/bnxt: fix global table scope shutdown order

## Summary

This patch fixes a crash during DPDK shutdown by correcting the table-scope teardown order and tracking per-port table-scope state. The patch introduces reference counting for GLOBAL scope and reorders cleanup operations.

---

## Errors

### 1. Missing error check on `bnxt_ulp_cntxt_acquire_fdb_lock()` rollback path

**File:** `drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c`  
**Location:** `ulp_tfc_tbl_scope_init()` rollback

```c
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;
}
```

**Problem:** After the `rollback:` label, the code calls `tfc_tbl_scope_cpm_free()`, `tfc_tbl_scope_mem_free()`, and `tfc_tbl_scope_fid_rem()` without holding any lock. These operations may require the same lock that just failed to acquire, or they may be racing with other threads operating on the same table scope. The lock acquisition failure indicates contention or lock state corruption; proceeding with cleanup without the lock is unsafe.

**Fix:** Either retry the lock with a timeout, or skip the rollback operations and return early (accepting that this port's CPM allocation will leak until firmware cleanup). Document the chosen strategy.

### 2. Resource leak on reference count decrement failure

**File:** `drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c`  
**Location:** `ulp_tfc_tbl_scope_deinit()`

```c
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;
}
```

**Problem:** When the reference count decrement fails (counter already at zero), the code continues teardown with `our_fid_cnt = 1` to avoid invalidating the scope. However, this path still calls `tfc_tbl_scope_cpm_free()` and `tfc_tbl_scope_fid_rem()` for this FID, which may have already been cleaned up by a previous call. This could free resources twice (double-free of CPM, double-removal of FID from firmware tables), or operate on stale firmware state.

The code attempts to handle this with "continuing teardown" but there is no verification that the tsid/fid combination is still valid in firmware. A decrement-failure suggests the port was already torn down once.

**Fix:** If `glb_tbl_scope_fid_cnt_dec()` fails, skip all cleanup operations for this port (early return). The counter being zero means another thread or previous invocation already cleaned up this port's share of the scope. Add a check or flag to detect double-deinit at a higher level and prevent the second call entirely.

### 3. Missing lock release on early return in deinit

**File:** `drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c`  
**Location:** `ulp_tfc_tbl_scope_deinit()` error paths after lock acquisition

The code acquires `fdb_lock` but several error paths return or goto cleanup without releasing it:

```c
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;
} else {
    scope_rc = bnxt_ulp_cntxt_tbl_scope_type_get(bp->ulp_ctx, &scope_type);
    if (scope_rc) {
        BNXT_DRV_DBG(ERR, ...);
        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, ...);
            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; /* LOCK NOT RELEASED */
```

**Problem:** When `acquire_fdb_lock()` succeeds, the `if (scope_rc)` and `if (rc)` error paths set `our_fid_cnt` but do not early-return; they fall through to `bnxt_ulp_cntxt_release_fdb_lock()`, which is correct. However, the `goto cleanup` at the top of the function (after `fid_get` failure) sets `have_scope = false` and jumps to `cleanup:`, where `return;` is executed **without checking whether the lock was acquired**. If the lock was acquired before the `fid_get()` call failed, the lock is never released.

Actually, re-reading the code: the `goto cleanup` happens **before** the lock is acquired. The lock acquisition block is only entered if `have_scope = true`. So this is **not** a lock leak on the current code flow.

**Correction:** On closer inspection, the `goto cleanup` after `fid_get()` failure occurs **before** the lock acquisition attempt (the lock is acquired inside the `if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx))` block which is after `have_scope = true`). Therefore, no lock leak exists. **Withdraw this item.**

---

## Warnings

### 1. Rollback logic incomplete -- init order different from deinit order

**File:** `drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c`  
**Location:** `ulp_tfc_tbl_scope_init()` rollback path

```c
rollback:
    rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
    rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, 0);
    rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);
    return -1;
```

**Problem:** The initialization order in `ulp_tfc_tbl_scope_init()` is:
1. `tfc_tbl_scope_mem_alloc()`
2. `tfc_tbl_scope_fid_add()`
3. `tfc_tbl_scope_cpm_alloc()`

The deinit order (established by this patch for correctness) is:
1. `cpm_free`
2. `fid_rem`
3. `mem_free`

The rollback path uses the deinit order, which is correct for preventing PXP errors. However, the rollback happens when only CPM allocation succeeded -- mem_alloc and fid_add already succeeded in the successful path before the `goto rollback`. If `acquire_fdb_lock()` fails **after** `tfc_tbl_scope_cpm_alloc()` but before the reference count increment, the rollback should mirror the full initialization (all three steps completed), not just the CPM step.

The rollback as written will call `mem_free()` and `fid_rem()` even though those were successful earlier in the function. This is likely correct (cleaning up the entire partial initialization), but the comment and structure should clarify that all three resources are being rolled back, not just CPM.

**Suggestion:** Add a comment above the rollback explaining that all three resources (CPM, FID, mem) are cleaned up because lock acquisition failed after all were allocated. Alternatively, restructure so lock acquisition happens earlier (before CPM alloc) to avoid needing rollback of all three.

### 2. `our_fid_cnt` usage asymmetry in init vs deinit

**File:** `drivers/net/bnxt/tf_ulp/bnxt_ulp_tfc.c`

In `ulp_tfc_tbl_scope_init()`:
- GLOBAL scope: increments `glb_tbl_scope_fid_cnt` **after** `tfc_tbl_scope_cpm_alloc()` succeeds

In `ulp_tfc_tbl_scope_deinit()`:
- GLOBAL scope: decrements `glb_tbl_scope_fid_cnt` **before** `tfc_tbl_scope_cpm_free()`

**Problem:** If a thread crashes or is killed between `cpm_alloc()` and the `glb_tbl_scope_fid_cnt_inc()`, the counter is not incremented but CPM is allocated. On deinit, the counter will be decremented below the correct value. Similarly, if deinit is called twice (e.g., driver unload then app cleanup), the second deinit attempt will decrement the counter to -1 (which the code detects and sets `our_fid_cnt = 1`), but the first deinit already freed CPM.

The asymmetry (inc after alloc, dec before free) is intentional to avoid freeing shared memory too early, but it opens a window where the counter is inconsistent with actual resource state.

**Suggestion:** Document this as a known race in a comment, or use a two-phase commit approach (mark FID as "teardown-in-progress" before decrementing, then clear the mark after CPM free). Alternatively, accept that double-deinit detection via counter underflow is sufficient and ensure double-deinit is prevented at a higher level (e.g., port close).

### 3. Error return value inconsistency in rollback

**Location:** `ulp_tfc_tbl_scope_init()` rollback path

```c
rollback:
    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;
```

**Problem:** The rollback path logs individual failures but always returns `-1`. Callers of `ulp_tfc_tbl_scope_init()` will see `-1` and cannot distinguish between "CPM alloc failed" (which didn't happen -- the rollback is after CPM alloc success) and "lock acquisition failed, resources cleaned up" vs "lock acquisition failed, cleanup also failed".

**Suggestion:** Return a distinct error code (e.g., `-EBUSY` for lock failure, `-EIO` for rollback failure) or at minimum change the return to `return rc;` so the last cleanup failure is propagated. Alternatively, keep `-1` but document that rollback failures are non-fatal (firmware will clean up on driver unload).

### 4. Missing release notes

**Location:** Patch-level

This patch fixes a crash ("unloading the L2 kernel driver with DPDK shutdown could crash") which is a significant bug fix. It should have a release notes entry in `doc/guides/rel_notes/release_XX_XX.rst` under "Fixed Issues" documenting the crash and the fix.

The patch is also tagged `Cc: stable at dpdk.org` indicating it should be backported to LTS releases; release notes are especially important for such fixes.

**Suggestion:** Add a release notes entry describing the crash scenario (GLOBAL table scope cleanup order) and the fix (reference counting and reordering cpm_free/fid_rem/mem_free).

---

## Info

### 1. Consider using `__rte_unused` for `rc` in deinit cleanup paths

**Location:** `ulp_tfc_tbl_scope_deinit()` cleanup section

```c
rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
if (rc)
    BNXT_DRV_DBG(ERR, "Failed Freeing CPM TSID:%d FID:%d", tsid, fid);
```

The return value of `tfc_tbl_scope_cpm_free()` and subsequent cleanup calls is checked only for logging; the function does not propagate the error. This is appropriate for teardown (best-effort cleanup), but if a compiler warning about unused `rc` appears (or if the checks are removed), consider annotating `rc` as `__rte_unused` in the cleanup-only section.

### 2. `goto cleanup` vs early return consistency

**Location:** `ulp_tfc_tbl_scope_deinit()`

The function uses both `goto cleanup` and early `return` for error handling. The `cleanup:` label only checks `if (!have_scope) return;` and then proceeds with teardown. This structure is correct but slightly unusual (the cleanup label is not actually cleaning up, it's a conditional execution point).

Consider renaming the label to `teardown:` or `do_teardown:` to clarify its purpose, or restructure to make the early returns explicit without a label (the code would be clearer with early returns throughout and the teardown section as a separate block at the end).

---

## Summary of Findings

**Errors (must fix):**
- Missing error check on `acquire_fdb_lock()` failure in rollback path -- cleanup operations proceed without lock
- Resource leak / double-free risk when reference count decrement fails (counter already zero)

**Warnings (should fix):**
- Rollback logic cleanup order should be documented as mirroring full init, not just CPM step
- Reference count increment/decrement asymmetry (init increments after alloc, deinit decrements before free) opens race window
- Rollback always returns `-1` regardless of individual failure reasons
- Missing release notes entry for crash fix

**Info (consider):**
- `__rte_unused` annotation for `rc` in cleanup-only paths
- Rename `cleanup:` label to `teardown:` for clarity