|WARNING| net/bnxt: truflow: recover CPM pools for reuse

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Code Review: series.patch

## Summary
This patch introduces recovery logic for pool exhaustion in the TFC (TruFlow Core) action and exact match (EM) subsystems. The primary goal is to recover pools that were marked unavailable due to block-size-class fragmentation. The patch adds three key mechanisms: (1) detection and recovery of block-size-limited pools, (2) automatic retry with pool rotation on allocation failure, and (3) CMM free-block-count tracking for recovery threshold checks.

---

## Errors

### 1. Resource Leak in act_get_or_alloc_pool() on tfc_tbl_scope_pool_alloc() failure

**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Lines:** 50-54

When `tfc_tbl_scope_pool_alloc()` fails after allocating a pool from the TPM, the pool is not cleaned up. If the CPM already registered this pool_id internally before returning an error, the CPM's state is now inconsistent with the TPM.

**Fix:** Add cleanup on error path after `tfc_tbl_scope_pool_alloc()`:
```c
rc = tfc_tbl_scope_pool_alloc(tfcp, fid, tsid, CFA_REGION_TYPE_ACT,
			      cmm_info->dir, NULL, pool_id);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "table scope pool alloc failed: %s", strerror(-rc));
	/* Clean up pool if TPM allocated it */
	/* (requires new TPM API or CPM API to release pool_id) */
	return -EINVAL;
}
```

### 2. Memory Leak in act_get_or_alloc_pool() on tfc_cpm_set_cmm_inst() failure

**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Lines:** 117-122

When `tfc_cpm_set_cmm_inst()` fails, the CMM instance is freed but the pool allocated by `tfc_tbl_scope_pool_alloc()` is not released back to the TPM. This leaks a pool slot.

**Fix:** Add TPM pool release before returning:
```c
rc = tfc_cpm_set_cmm_inst(cpm_act, *pool_id, *cmm);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "tfc_cpm_set_cmm_inst() failed: %d", rc);
	/* Release pool back to TPM */
	/* (requires tfc_tbl_scope_pool_free or equivalent) */
	rte_free(*cmm);
	*cmm = NULL;
	return -EINVAL;
}
```

### 3. Memory Leak in em_get_or_alloc_pool() on tfc_cpm_set_cmm_inst() failure

**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c`  
**Lines:** 192-197

Same issue as Error #2 above. Pool is leaked when `tfc_cpm_set_cmm_inst()` fails after `tfc_tbl_scope_pool_alloc()` succeeded.

**Fix:** Add TPM pool release before returning.

### 4. Memory Leak in em_get_or_alloc_pool() on cfa_mm_open() failure

**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c`  
**Lines:** 183-188

When `cfa_mm_open()` fails, the CMM memory is freed but the pool allocated from TPM is not released.

**Fix:** Release pool back to TPM on this error path.

### 5. Memory Leak in act_get_or_alloc_pool() on cfa_mm_query() failure

**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Lines:** 94-97

When `cfa_mm_query()` fails after `tfc_tbl_scope_pool_alloc()` succeeded, the pool is leaked.

**Fix:** Release pool back to TPM before returning.

---

## Warnings

### 1. tfc_cpm_set_usage() Return Value Ignored in Cleanup Path

**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c`  
**Line:** 209

```c
tfc_cpm_set_usage(cpm_lkup, pool_id, aparms.used_count, true, false);
```

In the first allocation retry path (`cfa_mm_alloc()` returns `-ENOMEM`), the return value from `tfc_cpm_set_usage()` is not checked. While this is a best-effort update and the allocation continues regardless, logging a warning on failure would aid debugging.

**Suggested Fix:**
```c
rc = tfc_cpm_set_usage(cpm_lkup, pool_id, aparms.used_count, true, false);
if (rc != 0)
	PMD_DRV_LOG_LINE(WARNING, "tfc_cpm_set_usage() failed during rotation: %d", rc);
```

### 2. Similar tfc_cpm_set_usage() Return Value Ignored in ACT Path

**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Line:** 210

Same pattern as Warning #1 above. Return value from `tfc_cpm_set_usage()` is not checked when marking a pool unavailable before rotation.

**Suggested Fix:** Same as Warning #1.

### 3. Inconsistent Error Handling on cfa_mm_alloc() After Retry

**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c`  
**Lines:** 310-313

When `cfa_mm_alloc()` fails after the retry (second call), the code returns the error but does not update the CPM with the final `used_count` or `all_used` state. The first allocation attempt on this new pool left the CMM in a state where `aparms.used_count` and `aparms.all_used` were updated but the CPM does not reflect this state if the second allocation also fails.

**Suggested Fix:**
```c
rc = cfa_mm_alloc(cmm, &aparms);
if (unlikely(rc)) {
	PMD_DRV_LOG_LINE(ERR, "cfa_mm_alloc() failed: %s", strerror(-rc));
	/* Update CPM with final state even on failure */
	if (aparms.all_used)
		tfc_cpm_set_usage(cpm_lkup, pool_id, aparms.used_count, true, false);
	return rc;
}
```

### 4. Similar Inconsistency in ACT Path After Retry

**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Lines:** 219-227

Same issue as Warning #3. Second `cfa_mm_alloc()` failure does not update CPM with final state.

**Suggested Fix:** Same as Warning #3.

### 5. Missing Release Notes Section for API Change

**File:** Patch does not include documentation changes.

This patch adds a new public API `cfa_mm_free_blk_count()` to `include/cfa_mm.h` and changes the signature of `tfc_cpm_set_usage()` (adding two boolean parameters). These are API/ABI changes that require release notes in `doc/guides/rel_notes/release_XX_XX.rst`.

**Suggested Fix:**  
Add a release notes entry documenting:
- New `cfa_mm_free_blk_count()` API
- Modified `tfc_cpm_set_usage()` signature
- Bug fix for pool recovery under sustained insert/delete churn

---

## Info

### 1. TFC_CPM_BLK_RECOVERY_THRESHOLD Magic Number Justification

**File:** `drivers/net/bnxt/tf_core/v3/tfc_cpm.h`  
**Lines:** 33-46

The constant `TFC_CPM_BLK_RECOVERY_THRESHOLD = 8` is well-justified in the comment (8 blocks x 8 records/block = 64 usable records). Consider adding a compile-time assertion or rationale comment in the code that uses this threshold to ensure future maintainers understand the dependency on `CFA_MM_MIN_RECORDS_PER_BLOCK`.

### 2. Duplicate em_get_or_alloc_pool() and act_get_or_alloc_pool() Logic

**Files:**  
- `drivers/net/bnxt/tf_core/v3/tfc_act.c` (act_get_or_alloc_pool)  
- `drivers/net/bnxt/tf_core/v3/tfc_em.c` (em_get_or_alloc_pool)

The two functions are nearly identical except for:
- Region type (CFA_REGION_TYPE_ACT vs CFA_REGION_TYPE_LKUP)
- Pool info field accessed (act_max_contig_rec vs lkup_max_contig_rec)
- EM subtracts lkup_rec_start_offset from rec_cnt

**Suggested Refactoring:**  
Consider extracting a common helper function parameterized by region type and pool info accessor. This reduces duplication and makes maintenance easier.

### 3. Unclear Error Path Ordering in act_get_or_alloc_pool()

**File:** `drivers/net/bnxt/tf_core/v3/tfc_act.c`  
**Lines:** 103-111

The function calls `cfa_mm_open()` and then checks for NULL, frees memory, and returns. However, the check `if (unlikely(*cmm == NULL))` happens *after* `cfa_mm_open()` which operates on `*cmm`. If `cfa_mm_open()` fails internally and leaves `*cmm` in a corrupted state, the subsequent free may be unsafe.

**Clarification Needed:**  
Review whether `cfa_mm_open()` can leave the CMM pointer in an invalid state on failure. If so, add error handling before freeing.

### 4. Potential Integer Overflow in qparms.max_records Calculation

**File:** `drivers/net/bnxt/tf_core/v3/tfc_em.c`  
**Line:** 141

```c
qparms.max_records = (mem_cfg->rec_cnt - mem_cfg->lkup_rec_start_offset) / max_pools;
```

If `lkup_rec_start_offset > rec_cnt`, this will wrap around (both are unsigned). While this scenario is unlikely in practice (would indicate misconfigured hardware), defensive code should check `rec_cnt >= lkup_rec_start_offset` before subtraction.

**Suggested Fix:**
```c
if (mem_cfg->rec_cnt < mem_cfg->lkup_rec_start_offset) {
	PMD_DRV_LOG_LINE(ERR, "invalid mem_cfg: rec_cnt < lkup_rec_start_offset");
	return -EINVAL;
}
qparms.max_records = (mem_cfg->rec_cnt - mem_cfg->lkup_rec_start_offset) / max_pools;
```

---

## Correctness Verification Passed

- **No use-after-free detected:** All freed memory paths are traced and no reuse is observed.
- **Thread safety:** All modifications to `free_blk_count` are within the CMM's lock or single-threaded context (no concurrency issues introduced).
- **NULL checks:** All `rte_zmalloc()` results are checked before use.
- **Return value checks:** Most critical return values are checked (except the Warnings noted above).
- **Integer operations:** No 64-bit bitmask shifts or narrow-type promotions found.

---

## Style Notes

All code follows DPDK style guidelines (no tabs-vs-spaces issues, proper brace style, function formatting). No prohibited tokens detected.

---

## Final Recommendation

**Do not merge** until resource leaks (Errors #1-5) are resolved. These are critical correctness issues that will cause pool/memory exhaustion under error conditions. The Warnings should be addressed for robustness and maintainability. The Info items are suggestions for future improvement.