|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

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

## Summary

This patch adds reference counting for GLOBAL-scope table resources and reorders teardown to prevent crashes and firmware errors during shutdown. The changes introduce tracking state (`tbl_scope_type`, `glb_tbl_scope_fid_cnt`) and modify both init and deinit paths.

---

## Errors

### 1. `ulp_tfc_tbl_scope_deinit`: resource leak on early error paths

**Error path analysis:**

- If `tsid_get` fails - goto cleanup - early return, but `have_scope = false` so no cleanup attempted
- If `fid_get` fails - goto cleanup - early return, but `have_scope = false` so no cleanup attempted
- If `acquire_fdb_lock` fails - proceeds with teardown using conservative `our_fid_cnt = 1`

The first two error paths skip all cleanup operations (cpm_free, fid_rem, mem_free) even though `tsid_get` or `fid_get` failure may occur *after* those resources were allocated. If the context was initialized but these getters fail due to corruption or invalid state, the resources remain allocated in firmware/hardware.

**Suggested fix:**

```c
/* At function entry, record whether context appears valid */
bool ctx_was_initialized = (tfcp != NULL && bp->ulp_ctx != NULL);

/* ... existing tsid_get and fid_get logic ... */

cleanup:
	if (!have_scope) {
		/* If context was initialized but we couldn't get tsid/fid,
		 * log the leak but don't attempt cleanup with invalid state */
		if (ctx_was_initialized)
			BNXT_DRV_DBG(WARNING,
				     "Context initialized but tsid/fid unavailable; cannot clean up scope resources");
		return;
	}
```

Alternatively, if `tsid_get`/`fid_get` failure always means "no resources to clean up", document that invariant in a comment so reviewers understand the early return is safe.

---

### 2. `ulp_tfc_tbl_scope_init`: rollback path resource leak

**Error:**

The rollback path calls three cleanup operations but ignores all their return codes (overwrites `rc` each time) and then returns generic `-1`. If any rollback operation fails, those failures are logged but the function does not communicate *which* cleanup succeeded/failed to the caller, making recovery or diagnostics difficult.

More critically, **each rollback step overwrites `rc` without checking it**, so if `cpm_free` fails but `mem_free` succeeds, the `cpm_free` failure is lost and the function returns `-1` (generic failure) with no indication that partial cleanup happened.

**Suggested fix:**

Track rollback failures separately:

```c
rollback:
	/* Rollback: only FID in scope (glb_tbl_scope_fid_cnt_inc never ran). */
	{
		int rollback_rc;
		bool rollback_failed = false;

		rollback_rc = tfc_tbl_scope_cpm_free(tfcp, tsid);
		if (rollback_rc) {
			BNXT_DRV_DBG(ERR, "Rollback: cpm_free failed TSID:%d FID:%d rc=%d",
				     tsid, fid, rollback_rc);
			rollback_failed = true;
		}
		rollback_rc = tfc_tbl_scope_mem_free(tfcp, fid, tsid, 0);
		if (rollback_rc) {
			BNXT_DRV_DBG(ERR, "Rollback: mem_free failed TSID:%d FID:%d rc=%d",
				     tsid, fid, rollback_rc);
			rollback_failed = true;
		}
		rollback_rc = tfc_tbl_scope_fid_rem(tfcp, fid, tsid, NULL);
		if (rollback_rc) {
			BNXT_DRV_DBG(ERR, "Rollback: fid_rem failed TSID:%d FID:%d rc=%d",
				     tsid, fid, rollback_rc);
			rollback_failed = true;
		}

		if (rollback_failed)
			BNXT_DRV_DBG(ERR, "Rollback incomplete: some resources may be leaked TSID:%d FID:%d",
				     tsid, fid);
	}
	return -EAGAIN; /* or other meaningful code */
```

---

### 3. `ulp_tfc_tbl_scope_deinit`: double cleanup attempt if lock acquisition fails

**Error:**

When `acquire_fdb_lock` fails, the code sets `our_fid_cnt = 1` (conservative) and proceeds to cleanup. However, the lock failure may indicate the lock is corrupted or the context is in an inconsistent state. Proceeding with cleanup operations (cpm_free, fid_rem, mem_free) without the lock could race with another thread that successfully acquired the lock and is also performing cleanup, leading to double-free or use-after-free if both threads call `mem_free` on the same tsid.

**Suggested fix:**

If lock acquisition fails, either:
- Skip cleanup entirely and return early (accepting the resource leak but avoiding double-free), or
- Retry lock acquisition a few times before proceeding, and if retries fail, log a critical error and skip cleanup:

```c
if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {
	BNXT_DRV_DBG(ERR,
		     "acquire_fdb_lock failed; cannot safely perform teardown (concurrent cleanup may be in progress)");
	/* Do not proceed without lock; cleanup may be unsafe */
	return;
}
```

---

### 4. `ulp_tfc_tbl_scope_init`: lock held across blocking operations

**Error:**

The lock acquired via `acquire_fdb_lock` is held while calling `glb_tbl_scope_fid_cnt_inc` and `tbl_scope_type_set`. If these operations block (e.g., due to internal locking or I/O), or if an error in `fid_cnt_inc` causes an early return before `release_fdb_lock`, the lock is held longer than necessary or leaked.

Additionally, if `fid_cnt_inc` fails, the code does **not** goto `rollback` -- it returns immediately after releasing the lock, leaving the CPM allocated (via `tfc_tbl_scope_cpm_alloc`) but never adding the FID to the reference count. This is a **resource leak**: the CPM is allocated but the error path does not clean it up.

**Suggested fix:**

```c
if (bnxt_ulp_cntxt_acquire_fdb_lock(bp->ulp_ctx)) {
	BNXT_DRV_DBG(ERR, "acquire_fdb_lock failed after CPM alloc");
	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;  /* MUST rollback CPM allocation */
	}
}
rc = bnxt_ulp_cntxt_tbl_scope_type_set(bp->ulp_ctx, scope_type);
if (rc) {
	/* Decrement count we just incremented before releasing lock */
	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);
```

---

### 5. `bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec`: underflow not prevented, only detected

**Error:**

The function checks if `fid_cnt == 0` and returns `-EINVAL`, but this is **after** the damage is done if the check is bypassed or if concurrent decrements race. The current check prevents decrement-past-zero only when called correctly, but does not use atomics, so two threads could both read `fid_cnt=1`, both pass the check, and both decrement, leaving `fid_cnt` at `-1` (underflow).

Additionally, the `-EINVAL` return is checked only in one place (`ulp_tfc_tbl_scope_deinit`), where the code logs a warning and continues. If the count is already zero, `dec` should not be called at all -- this is a logic error in the caller, and continuing with `our_fid_cnt = 1` is a workaround that masks the bug.

**Suggested fix:**

If the reference count is meant to be protected by `acquire_fdb_lock`, document that lock must be held when calling `_dec` and `_inc`. If concurrent access is possible, use atomics:

```c
int32_t
bnxt_ulp_cntxt_glb_tbl_scope_fid_cnt_dec(struct bnxt_ulp_context *ulp_ctx)
{
	uint16_t old_val;
	if (ulp_ctx == NULL || ulp_ctx->cfg_data == NULL)
		return -EINVAL;
	old_val = rte_atomic_load_explicit(&ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt,
					   rte_memory_order_acquire);
	if (old_val == 0)
		return -EINVAL;
	rte_atomic_fetch_sub_explicit(&ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt, 1,
				      rte_memory_order_acq_rel);
	return 0;
}
```

Or, if the lock is always held, add an assertion:

```c
/* REQUIRES: caller holds acquire_fdb_lock */
if (ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt == 0) {
	/* BUG: caller logic error - count already zero */
	RTE_ASSERT(0);
	return -EINVAL;
}
ulp_ctx->cfg_data->glb_tbl_scope_fid_cnt--;
```

---

## Warnings

### 1. `ulp_tfc_tbl_scope_deinit`: log level inconsistency

**Warning:**

`fid_get` failure logs `ERR`, but `acquire_fdb_lock` failure logs `ERR` and proceeds with teardown. The lock failure is treated as non-fatal (code continues), but `fid_get` failure is treated as fatal (early return). This inconsistency is confusing: if lock failure is recoverable, why not `fid_get` failure?

**Suggested fix:**

Ensure log levels match the code path. If lock failure is truly an error but the code proceeds with a conservative fallback, log it as `WARNING` instead of `ERR`:

```c
BNXT_DRV_DBG(WARNING, "acquire_fdb_lock failed, proceeding with teardown using conservative fid_cnt");
```

Or, if lock failure should prevent teardown, make it an `ERR` and `goto cleanup;`.

---

### 2. Missing release notes update

The commit message references `Fixes:` and `Cc: stable at dpdk.org`, indicating this is a bug fix for a previously released feature. However, there is no update to `doc/guides/rel_notes/` documenting the fix. While stable backports may not always require release notes in the current release, significant fixes that affect production stability (crash prevention) should be documented.

**Suggested fix:**

Add a "Fixed Issues" entry in the current release notes (e.g., `doc/guides/rel_notes/release_25_03.rst`):

```rst
* **net/bnxt: Fixed crash during global table scope teardown.**

  Corrected the shutdown order for GLOBAL table scopes to prevent crashes
  and firmware errors when unloading the driver with active DPDK ports.
```

---

### 3. `ulp_tfc_tbl_scope_init`: return code `-1` instead of meaningful errno

**Warning:**

On rollback failure, the function returns `-1` (generic failure). DPDK convention prefers specific negative errno values (`-EAGAIN`, `-EIO`, `-ENOMEM`, etc.) to help callers diagnose the failure.

**Suggested fix:**

```c
return -EAGAIN; /* or -EIO if hardware/firmware error */
```

---

### 4. New global state not zeroed on context allocation

**Warning:**

The patch adds `tbl_scope_type` and `glb_tbl_scope_fid_cnt` to `struct bnxt_ulp_data` but does not show initialization to known values (zero / `CFA_SCOPE_TYPE_INVALID`) at context creation. If the context memory is not zeroed, these fields may contain garbage, leading to incorrect ref-count logic or scope-type checks.

**Suggested fix:**

Verify that `bnxt_ulp_data` is allocated with `rte_zmalloc` or explicitly zeroed. If this patch is part of a series where initialization is added in a later patch, disregard this warning.

---

## Info

### 1. Comment style: code is well-documented but could benefit from function-level Doxygen

**Info:**

The new helper functions (`tbl_scope_type_get`, `fid_cnt_inc`, etc.) in `bnxt_ulp_tfc.h` are declared but lack Doxygen comments explaining their purpose, parameters, and return values. While internal APIs do not strictly require Doxygen, adding brief comments would improve maintainability, especially for ref-count increment/decrement which have specific lock requirements.

**Suggested addition:**

```c
/**
 * Get the table scope type for the ULP context.
 *
 * @param ulp_ctx ULP context pointer.
 * @param scope_type Pointer to store the scope type.
 * @return 0 on success, -EINVAL if parameters are NULL.
 */
int32_t
bnxt_ulp_cntxt_tbl_scope_type_get(struct bnxt_ulp_context *ulp_ctx,
				  enum cfa_scope_type *scope_type);
```

---

### 2. Consider adding a state machine diagram in commit message

**Info:**

The commit message explains the teardown order change (`cpm_free -> fid_rem -> mem_free`) but a visual diagram showing state transitions during init and deinit would help future reviewers understand the fix. This is optional but would be valuable given the complexity.

---

## Conclusion

**Critical issues that must be fixed before merge:**
- Error #1: Resource leak on early error paths in deinit
- Error #2: Rollback path ignores partial cleanup failures
- Error #3: Double cleanup risk if lock acquisition fails
- Error #4: Lock held during blocking ops; missing rollback on fid_cnt_inc failure
- Error #5: Reference count underflow not prevented atomically

**Should fix:**
- Warning #1: Log level inconsistency
- Warning #2: Missing release notes
- Warning #3: Return `-1` instead of specific errno
- Warning #4: Global state initialization not shown

**Consider:**
- Info #1: Add Doxygen comments
- Info #2: Add state diagram to commit message

The patch addresses a real correctness bug (crash during teardown), but the error handling in both init and deinit paths introduces new potential bugs (leaks, double-cleanup, race conditions) that must be resolved.