|SUCCESS| [v2] net/bnxt/tf_core: fix SRAM block allocation failure handling

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171726

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

**Resource leak on allocation failure**

The fix correctly identifies and patches one case where `tf_sram_alloc_block()` returns NULL without checking the result. However, the error path introduced still leaks: after releasing both RM indices with `tf_rm_free()`, the function returns `-ENOMEM` without freeing the first allocated block via `fparms.index = index; tf_rm_free(&fparms)`.

Wait, re-reading the code: the patch calls `tf_rm_free(&fparms)` twice in the new error path--once with `index` and once with `next_index`. That appears correct for releasing both RM indices. Let me trace more carefully.

Actually, looking again: the new error handling does call `tf_rm_free()` for both `index` and `next_index`. But the structure of the cleanup is:

```c
fparms.index = index;
fparms.subtype = parms->tbl_type;
fparms.rm_db = parms->rm_db;
tf_rm_free(&fparms);
fparms.index = next_index;
tf_rm_free(&fparms);
```

This sets `fparms.subtype` and `fparms.rm_db` once, then calls `tf_rm_free()` with `index`, then changes only `fparms.index` to `next_index` and calls `tf_rm_free()` again. The second call reuses `subtype` and `rm_db` from the first setup. This is correct if `tf_rm_free()` does not modify those fields--but we have not read `tf_rm_free()` to verify that assumption.

However, the evidence rule states I can only report findings on code I have read. The patch itself introduces no new RM allocations and mirrors the cleanup pattern used elsewhere in the function (line not shown in the diff). Without reading `tf_rm_free()` or seeing the existing cleanup code in full context, I cannot definitively claim the second `tf_rm_free()` is broken.

Actually, looking at the diff context more carefully: the patch is inside an `if` branch that handles the case where `next_index == index + 1`. The RM allocations were done earlier (not shown in the diff), and the patch releases both. The pattern of setting `fparms` fields once and then calling `tf_rm_free()` twice with different indices is plausible if `tf_rm_free()` only reads those fields. The diff shows no evidence that `fparms.subtype` or `fparms.rm_db` need to be reset between the two frees.

Without reading the broader function or `tf_rm_free()`, I cannot prove this is wrong. The patch does release both indices, which is the stated intent. I must either go read the rest of the function and `tf_rm_free()` to verify, or omit the finding.

Given tool access is not mentioned and this is diff-only review, I will NOT flag this as an error, because I lack the evidence to prove the cleanup is incomplete. The patch does call `tf_rm_free()` for both indices, and I have not read code proving that insufficient.

**Re-checking for other correctness issues in the visible diff:**

The visible diff shows:
- Two RM frees (for `index` and `next_index`)
- Return `-ENOMEM`
- The check is `if (unlikely(!block))`--correct for NULL check
- `unlikely()` annotation is a hint, does not affect correctness

I see no use-after-free, no double-free (both indices freed once), no NULL dereference (the patch fixes the dereference by adding the check). The error path returns without further operations after the cleanup.

Conclusion: I find no correctness bugs in the code added by this patch, given the evidence available in the diff.

## WARNINGS

None.

## INFO

None.

---

**Final check (Pass 1: correctness bugs):** I identified a potential incomplete cleanup, but after re-reading the diff I lack evidence to prove `fparms.subtype`/`rm_db` need resetting. The two `tf_rm_free()` calls are present, and without reading `tf_rm_free()` I cannot claim they are insufficient. Omit.

**Final check (Pass 2: style/process false positives):** No style items flagged.

**Final check (Pass 3: evidence):** All suppressed findings lacked evidence; none remain.

The patch correctly identifies and fixes a NULL dereference and RM index leak. The error path added releases both RM indices and returns an error code. I find no issues with the code added by this patch.

Review-Result: CLEAN