|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 fixes teardown ordering for table-scope resources in the bnxt PMD. It introduces reference counting for GLOBAL-scope FIDs and reorders cleanup operations to prevent firmware errors and crashes during shutdown.

---

## Errors

### 1. Error path resource leak in `ulp_tfc_tbl_scope_deinit()`

In `ulp_tfc_tbl_scope_deinit()`, when lock acquisition fails, the code sets `our_fid_cnt = 1` and continues to the cleanup section. However, if `have_scope` is false (due to earlier failures in `tsid_get` or `fid_get`), the function returns immediately at the `cleanup:` label without attempting to release any resources that might have been partially allocated. This creates inconsistent state.

**Suggested fix:**

The `cleanup:` label logic needs review. If `tfcp`, `tsid`, and `fid` were successfully obtained but lock acquisition failed, we should still attempt cleanup rather than returning early. Consider restructuring:

```c
cleanup:
	if (!have_scope || tfcp == NULL)
		return;
	/* proceed with cpm_free, fid_rem, mem_free */
```

---

### 2. Rollback path has wrong order and insufficient error handling

In `ulp_tfc_tbl_scope_init()` rollback section (lines 635-644), the cleanup sequence is:
1. `tfc_tbl_scope_cpm_free()`
2. `tfc_tbl_scope_mem_free()`
3. `tfc_tbl_scope_fid_rem()`

But the normal teardown path (fixed by this patch) does cpm_free - fid_rem - mem_free, not cpm_free - mem_free - fid_rem. The rollback path should match the corrected normal teardown order.

**Why it matters:** The commit message states fid_rem must precede mem_free to avoid operating on an already-disabled scope. The rollback violates this ordering.

**Suggested fix:**

```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_fid_rem(tfcp, fid, tsid, NULL);  /* before mem_free */
	if (rc)
		BNXT_DRV_DBG(INFO, "Rollback: fid_rem failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
	rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, 0);     /* after fid_rem */
	if (rc)
		BNXT_DRV_DBG(INFO, "Rollback: mem_free failed TSID:%d FID:%d rc=%d", tsid, fid, rc);
	return -1;
```

---

### 3. Rollback does not decrement refcount if incremented

In the `ulp_tfc_tbl_scope_init()` success path, if `scope_type == CFA_SCOPE_TYPE_GLOBAL`, the code calls `bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_inc()` before `tbl_scope_type_set()`. If `tbl_scope_type_set()` itself were to fail (not shown, but imagine a future error path), the rollback at line 634 does not decrement the refcount.

Currently, `tbl_scope_type_set()` cannot fail (it only checks NULL and does an assignment), but the rollback after `glb_tbl_scope_fid_cnt_inc()` should be robust. If the count was incremented, it should be decremented on rollback.

**Suggested fix:**

Add to rollback section before `tfc_tbl_scope_cpm_free()`:

```c
rollback:
	/* If we incremented GLOBAL scope refcount, roll it back */
	if (scope_type == CFA_SCOPE_TYPE_GLOBAL) {
		(void)bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(bp->ulp_ctx);
	}
	rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
	...
```

Note: This assumes the lock is still held or that decrementing outside the lock is acceptable. Since we're in an error path and the increment happened under lock, ideally we'd re-acquire the lock, decrement, and release.

---

## Warnings

### 1. Missing error check on `tbl_scope_type_set()` in init path

At line 628, `bnxt_ulp_cntxt_tbl_scope_type_set()` is called but its return value is not checked. While the function currently cannot fail (it returns `-EINVAL` only on NULL context), checking the return value is defensive and documents expectations.

**Suggested fix:**

```c
rc = bnxt_ulp_cntxt_tbl_scope_type_set(bp->ulp_ctx, scope_type);
if (rc) {
	BNXT_DRV_DBG(ERR, "Failed to set tbl_scope_type");
	bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
	/* also decrement refcount if GLOBAL */
	goto rollback;
}
bnxt_ulp_cntxt_release_fdb_lock(bp->ulp_ctx);
```

---

### 2. Incorrect diagnostic message in `ulp_tfc_tbl_scope_deinit()`

At line 367, the error message says `"Failed to get the table scope\n"` but the function is `tsid_get`, which retrieves the table scope ID, not "the table scope" itself. The message is slightly misleading.

**Suggested fix:**

```c
BNXT_DRV_DBG(ERR, "Failed to get table scope ID, rc=%d", rc);
```

---

### 3. Comment in `ulp_tfc_tbl_scope_deinit()` mentions invalidation but logic is unclear

At line 417, the comment says `"mem_free invalidates tsid scope state"`. This suggests that after `mem_free`, the scope is disabled/invalid. However, the code still calls `mem_free` even when `our_fid_cnt` is non-zero. The `mem_free` function behavior when `fid_cnt > 0` is not clear from this patch alone -- does it only free if `fid_cnt == 0`, or does it always free?

**Clarification needed:** If `mem_free` only invalidates when `fid_cnt == 0`, the comment should say so. If it always invalidates, then calling it with `our_fid_cnt = 1` (the conservative fallback) might still cause issues.

**Suggested comment improvement:**

```c
/* Free this port's CPM before fid_rem/mem_free.
 * mem_free will invalidate the scope if our_fid_cnt reaches 0. */
```

---

### 4. Redundant NULL check in accessor functions

In `bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get()` (line 138), the function returns 0 if `ulp_ctx` or `cfg_data` is NULL. However, all callers in this patch check the return value of other functions that would have already failed if `ulp_ctx` were NULL. Returning 0 on NULL is a silent error that could mask bugs.

**Suggested alternative:** Consider whether returning 0 is the right sentinel. If the caller cannot distinguish "refcount is 0" from "error, no refcount available," bugs could be introduced. The `_get` functions that return `int32_t` for error codes use `-EINVAL`; the `uint16_t` return of `_cnt_get()` cannot signal an error.

**Suggested fix:** Document in the function comment that the caller must ensure `ulp_ctx` is non-NULL before calling, or change to return a status via an out-parameter:

```c
int32_t
bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(struct bnxt_ulp_context *ulp_ctx,
                                         uint16_t *fid_cnt)
{
	if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL || fid_cnt == NULL)
		return -EINVAL;
	*fid_cnt = ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt;
	return 0;
}
```

Then the caller at line 405 would change to:

```c
rc = bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_get(bp->ulp_ctx, &our_fid_cnt);
if (rc) { /* handle error */ }
```

---

## Info / Observations

### 1. Lock-hold duration during init

In `ulp_tfc_tbl_scope_init()`, the lock is held across both the refcount increment and the `tbl_scope_type_set()` call. This is correct for atomicity (type and count must be consistent), but it's worth noting that the lock is held briefly and only guards the shared data structure updates, not the TFC API calls. This is appropriate.

---

### 2. Conservative fallback when lock acquisition fails

At line 388, if lock acquisition fails in `deinit()`, the code sets `our_fid_cnt = 1` to avoid treating the scope as the last FID and invalidating shared state. This is a safe fallback but may leak resources in GLOBAL scope if this port was actually the last FID. The comment explaining this (line 391) is helpful.

---

### 3. Commit message accuracy

The v2 commit message correctly notes that the relative order of `fid_rem` and `mem_free` is unchanged; only `cpm_free` moving earlier is new. This is accurate and matches the code.

---

## Positive Observations

- The patch correctly introduces reference counting for GLOBAL scope teardown, preventing premature resource release.
- Error paths log diagnostics with `BNXT_DRV_DBG`, aiding troubleshooting.
- The rollback path in `init()` was added to fix a CPM leak identified in v2, which is a good correctness improvement.
- Lock acquisition is properly paired with release in all paths.

---

## Summary of Required Fixes

**Errors (must fix):**
1. Rollback path in `ulp_tfc_tbl_scope_init()` has wrong cleanup order (mem_free before fid_rem).
2. Rollback does not decrement GLOBAL refcount if it was incremented before failure.
3. Cleanup logic in `ulp_tfc_tbl_scope_deinit()` may skip necessary teardown when `have_scope` is false but resources were partially allocated.

**Warnings (should fix):**
1. Missing error check on `tbl_scope_type_set()` return value.
2. Misleading error message at line 367.
3. `glb_tbl_scope_fid_cnt_get()` returning `uint16_t` cannot signal errors; consider changing to return status via out-param.