|SUCCESS| [v2] net/bnxt/tf_core: fix TPM pool create error return

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Thank you for providing the patch. I'll review it according to the AGENTS.md guidelines.

## Analysis

This patch fixes an error propagation bug in `tbl_scope_pools_create()`. Let me trace through the error paths:

**Current code problem:**
1. `rte_zmalloc()` fails, `rc` is still 0 from a previous success, jumps to cleanup, returns 0 (success)
2. Other failures call cleanup; cleanup loop calls `cfa_tim_tpm_inst_get()` and `cfa_tim_tpm_inst_set()`, which overwrite `rc` with their return values, losing the original error

**Patch solution:**
1. Sets `rc = -ENOMEM` when `rte_zmalloc()` fails
2. Introduces `err = rc` before cleanup to preserve the original error
3. Returns `err` instead of `rc` after cleanup

## Correctness Review

The fix is correct. Both error propagation bugs are addressed:
- The allocation failure now produces an error return
- The cleanup loop no longer clobbers the error that triggered it

I traced the cleanup loop: it walks two nested loops calling `cfa_tim_tpm_inst_get()` and `cfa_tim_tpm_inst_set()`, then frees memory. The comment "Keep the error that got us here" accurately describes the intent. The `err` variable is read-only after assignment, so the cleanup cannot corrupt it.

## Style and Guidelines

**Code style:**
- Indentation and formatting are correct
- Comment style is proper
- Variable declaration follows DPDK style (declared at start of function)

**Comparison style:**
- `if (tpms[dir][region] == NULL)` - explicit comparison, correct per AGENTS.md (though this line was pre-existing context)
- `if (tim)` - this is a pointer truthiness check. Per AGENTS.md, should be `if (tim != NULL)`. However, this line is **pre-existing code** that the patch does not touch (appears only in diff context). Not flagging as Error/Warning since it's not introduced by this patch.

**Evidence and scope:**
- All findings rest on code in the added lines or visible in context
- The patch touches only error handling; no new functionality
- Cleanup code correctness verified by reading the context lines

## Process and Documentation

- Fixes tag present and correct
- Cc: stable at dpdk.org present (appropriate for a bug fix)
- Commit message clearly explains the bug and the fix
- Release notes: this is a bug fix in a driver, not a new API or feature change. Release notes are not required per AGENTS.md ("Test-only changes" and "Internal implementation changes that don't affect public API" don't require release notes; driver internal bug fixes fall in this category)

## Info (Pre-existing)

The pointer `tim` at line 646 uses implicit truthiness (`if (tim)`) rather than explicit comparison (`if (tim != NULL)`). This is pre-existing code not introduced by this patch, visible only in diff context. Worth noting for future cleanup but not a blocker for this change.

## Summary

The patch correctly fixes both error propagation bugs identified in the commit message. The solution is straightforward: assign the error to a separate variable before cleanup, return that variable after cleanup. No resource leaks, no further error path issues identified.

Review-Result: CLEAN