|WARNING| [v2] 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/170526

_AI Code Review_

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

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

## Summary

This patch addresses table-scope teardown ordering to prevent crashes during DPDK shutdown when used with the L2 kernel driver. It introduces reference counting for GLOBAL-scope shared resources and reorders cleanup steps to avoid operating on already-disabled firmware state.

---

## Errors

### 1. Missing error check on `tbl_scope_type_set` in success path

**Location:** `bnxt_ulp_tfc.c:629`

```c
rc = bnxt_ulp_cntxt_tbl_scope_type_set(bp->ulp_ctx, scope_type);
bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
```

The return value from `bnxt_ulp_cntxt_tbl_scope_type_set()` is stored in `rc` but never checked. If this function fails, the lock is released and the function returns `rc` (which could be an error), but the CPM allocation and FID count increment (for GLOBAL scope) remain in place without being rolled back. This creates an inconsistent state where resources are allocated but the scope type is not recorded.

**Fix:** Check `rc` after `tbl_scope_type_set()` and goto rollback if it fails. Also need to decrement `glb_tbl_scope_fid_cnt` in the rollback path if it was incremented.

```c
rc = bnxt_ulp_cntxt_tbl_scope_type_set(bp->ulp_ctx, scope_type);
if (rc) {
	if (scope_type == CFA_SCOPE_TYPE_GLOBAL)
		bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(bp->ulp_ctx);
	bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
	goto rollback;
}
bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
```

---

### 2. Rollback path does not decrement `glb_tbl_scope_fid_cnt` for GLOBAL scope

**Location:** `bnxt_ulp_tfc.c:634` (rollback label)

The rollback path frees CPM, removes FID, and frees memory, but never decrements `glb_tbl_scope_fid_cnt` if it was incremented before the rollback was triggered. The comment says "only FID in scope (glb_tbl_scope_fid_cnt_inc never ran)" but this is incorrect -- `glb_tbl_scope_fid_cnt_inc()` runs at line 621 and the rollback can be reached from line 624 (if `tbl_scope_type_set` fails per Error #1 above) after the increment.

**Fix:** Track whether the increment succeeded and decrement in rollback if needed.

```c
rollback:
	/* Undo FID count increment if it happened */
	if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
		if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx) == 0) {
			bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(bp->ulp_ctx);
			bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
		}
	}
	/* existing rollback code ... */
```

---

### 3. Missing return value check on `acquire_fdb_lock` success path

**Location:** `bnxt_ulp_tfc.c:616`

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

The lock acquisition is checked and rollback occurs on failure, but in the success case (lock acquired), if `glb_tbl_scope_fid_cnt_inc()` or `tbl_scope_type_set()` fails, the lock is released but the CPM remains allocated. The rollback is then executed without the lock held, meaning the decrement in the proposed fix for Error #2 would need to re-acquire the lock. This is correct if done carefully, but the current code does not track whether the lock is held.

**Recommendation:** Ensure rollback can safely decrement `glb_tbl_scope_fid_cnt` even if called without the lock held (acquire it again as shown in fix for Error #2).

---

## Warnings

### 1. Inconsistent NULL-check pattern in getter functions

**Location:** `bnxt_ulp_tfc.c:135`, `bnxt_ulp_tfc.c:154`

Functions `bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get()` and `bnxt_ulp_cntxt_em_mulitplier_get()` return 0 on NULL input, which makes it impossible to distinguish "context is NULL" from "count/multiplier is legitimately 0". The other new functions return `-EINVAL` on NULL, which is more consistent. However, `fid_cnt_get()` is called at line 395 in deinit without checking the return value, so it's being used as a "safe" getter.

**Suggestion:** Either consistently return error codes and require callers to check, or consistently return sentinel values and document the behavior. The current mix is confusing.

---

### 2. `goto cleanup` bypasses resource cleanup when no scope exists

**Location:** `bnxt_ulp_tfc.c:370`, `bnxt_ulp_tfc.c:375`, `bnxt_ulp_tfc.c:380`

If `tfcp` is NULL, `tsid_get` fails, or `fid_get` fails, the function jumps to `cleanup:` which immediately returns without attempting any cleanup. This is correct if no scope was ever set up, but the control flow is unclear -- the label is named `cleanup` but does not clean anything. A comment clarifying "nothing to clean up, scope was never initialized" would help.

---

### 3. FID count decrement continues teardown even on underflow

**Location:** `bnxt_ulp_tfc.c:396`

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

The decrement function (line 163) explicitly checks for underflow and returns `-EINVAL` if count is already 0. This suggests a bug in the calling code (double-decrement or missing increment), but the deinit path treats it as a warning and continues. If the count is 0, this FID was never added or was already removed, which likely indicates a double-free scenario. Consider logging this at ERROR level rather than WARNING, as it indicates a state inconsistency.

---

### 4. Rollback uses hardcoded `-1` return instead of propagating actual error code

**Location:** `bnxt_ulp_tfc.c:642`

```c
return -1;
```

The rollback path returns `-1` unconditionally. The actual error code from the operation that triggered rollback is lost (it was stored in `rc` from line 621 or line 629). Returning the actual error code would be more informative for debugging.

**Suggestion:**
```c
return rc;  /* Return the actual error that triggered rollback */
```

---

## Info

### 1. `our_fid_cnt` naming could be clearer

**Location:** `bnxt_ulp_tfc.c:360`

The variable name `our_fid_cnt` is used to hold the remaining FID count after decrementing for this port. The name suggests "our count" but it actually means "remaining count in the shared scope after we leave". A clearer name might be `remaining_fid_cnt` or `scope_fid_cnt_after_dec`.

---

### 2. Comment about scope invalidation could be more precise

**Location:** `bnxt_ulp_tfc.c:403`

```c
/* Pass 1 so mem_free won't treat as last FID & invalidate scope */
```

The comment correctly explains the intent, but doesn't clarify that `our_fid_cnt == 0` is the trigger for mem_free to invalidate. A more precise comment would be: "Pass 1 instead of 0 to prevent mem_free from invalidating the scope (which happens when fid_cnt is 0)."

---

### 3. Consider using symbolic constant instead of magic `1` for conservative FID count

**Location:** `bnxt_ulp_tfc.c:367`, `bnxt_ulp_tfc.c:377`, `bnxt_ulp_tfc.c:389`, `bnxt_ulp_tfc.c:403`

The value `1` is used multiple times to mean "not the last FID, don't invalidate scope". Defining a constant like `CONSERVATIVE_FID_CNT` would make the intent clearer.

---

## Additional Observations

### Documentation

- The patch adds new API functions to `bnxt_ulp_tfc.h` but they lack Doxygen comments. Since these are exported from the header (even though they are driver-internal), brief comments explaining the contract (what they return on error, when to call them) would improve maintainability.

### Testing

- The commit message mentions this fixes a crash on shutdown with the L2 kernel driver, but no test is added to `app/test` to verify the fix. Given the complexity of the reference counting and shutdown ordering, a functional test that exercises GLOBAL scope teardown with multiple FIDs would be valuable.

---

## Conclusion

The patch addresses a real correctness issue (table-scope teardown ordering and reference counting), but the error handling in the success path of `ulp_tfc_tbl_scope_init()` is incomplete:
- Missing check on `tbl_scope_type_set()` return value (Error #1)
- Rollback path does not undo `glb_tbl_scope_fid_cnt` increment (Error #2)

These must be fixed before merge. The warnings are lower-priority improvements to error handling consistency and code clarity.