|WARNING| [v4] net/intel: fix idpf Tx payload corruption in split queue

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Errors

**1. Memory leak on split-queue setup error path**

In `cpfl_tx_queue_setup()` at line 621 and `idpf_tx_queue_setup()` at line 493:
```c
ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
if (ret != 0)
    goto err_sw_ring_alloc;
```
The error label `err_sw_ring_alloc` frees `txq->mz` but does not free the newly allocated `tx_pending_pkts` shadow ring. When `idpf_qc_split_tx_pending_alloc()` succeeds but a later allocation (completion queue setup) fails, the shadow ring leaks.

**Fix**: Add cleanup before `goto err_sw_ring_alloc`, or create a new error label after the shadow ring allocation that also frees `txq->tx_pending_pkts`.

---

**2. Missing bounds check on completion tag in production build**

In `idpf_split_tx_free()` at lines 860-867:
```c
uint16_t tag = q_head;

if (unlikely(tag >= txq->nb_tx_desc)) {
    TX_LOG(ERR, "invalid completion tag %u.", tag);
} else if (txq->tx_pending_pkts[tag] != NULL) {
    rte_pktmbuf_free(txq->tx_pending_pkts[tag]);
    txq->tx_pending_pkts[tag] = NULL;
}
```
When `tag >= txq->nb_tx_desc`, the code logs an error but continues without freeing anything and without returning early. On the next iteration or function call, the invalid tag is not handled, and if another completion uses it, there's potential for out-of-bounds access or silent data corruption.

**Fix**: Return early after logging the error, or at minimum skip processing this completion and continue to the next one.

---

**3. `use_vec_entry` flag misuse for split queue mbuf drain**

In `idpf_qc_split_tx_release_mbufs()` at line 246:
```c
if (txq->use_vec_entry) {
    ci_txq_release_all_mbufs(txq, false);
    return;
}
```
The code calls `ci_txq_release_all_mbufs(txq, false)` where the second parameter is `use_vec_entry`. However, it's hardcoded to `false`, which does not match `txq->use_vec_entry`. This means vector split queues will take the scalar code path in `ci_txq_release_all_mbufs()`, which expects `sw_ring` to be valid. But the patch places `tx_pending_pkts` in the union with `sw_ring`, so `sw_ring` is NULL and dereferencing it will segfault.

**Fix**: Pass `txq->use_vec_entry` as the second argument, not `false`.

---

## Warnings

**1. Inconsistent error propagation on split-queue shadow ring allocation failure**

In `cpfl_tx_queue_setup()` at line 621 and `idpf_tx_queue_setup()` at line 493, when `idpf_qc_split_tx_pending_alloc()` returns `-ENOMEM`, the code jumps to `err_sw_ring_alloc` which does not set `ret`. If a prior check left `ret` as 0, the function could return success despite the allocation failure.

**Fix**: Ensure `ret` is explicitly set before `goto`, or verify that all paths to the label have set `ret` appropriately. (In context, both functions appear to have `ret` set by the callee, so this may already be correct--verify the return value of `idpf_qc_split_tx_pending_alloc()` is always assigned.)

---

**2. Missing release notes**

The patch fixes a significant correctness bug (payload corruption) and changes internal API (`idpf_qc_split_tx_pending_free()` renamed to `idpf_qc_split_tx_release_mbufs()`), but the commit message does not indicate whether release notes were updated. User-visible bugs and API changes require release notes.

**Fix**: Update `doc/guides/rel_notes/release_26_03.rst` (or the appropriate current release file) to document the bug fix.

---

## Info

**1. Pre-existing: NULL check before loop in `idpf_qc_split_tx_release_mbufs()`**

At line 244:
```c
if (txq->tx_pending_pkts == NULL)
    return;
```
This is defensive code added by the patch. However, the callers (`cpfl_tx_queue_release()`, `idpf_qc_tx_queue_release()`, and `idpf_qc_split_tx_descq_reset()`) are all in contexts where `txq` itself has been validated and the queue was successfully set up. If `tx_pending_pkts` can be NULL here, it suggests a setup/teardown ordering issue that should be fixed rather than papered over.

Consider asserting instead, or documenting why this can occur (e.g., queue release called after partial setup failure).

---

**2. Magic number for cleanup iterations**

At line 1004 in `idpf_dp_splitq_xmit_pkts()`:
```c
nb_to_clean = 2 * txq->tx_rs_thresh;
```
The `2 *` multiplier is not documented. A comment explaining why twice the RS threshold is the cleanup bound would improve maintainability.

---

**3. Inconsistent style: variable declaration in outer scope**

At line 1002-1003:
```c
uint16_t tag = txq->tx_next_compl_tag;

if (unlikely(txq->tx_pending_pkts[tag] != NULL)) {
```
The `tag` variable is declared in the outer loop scope but is only used in this conditional block. DPDK style prefers declaring variables in the innermost scope where they are used (C99 style is acceptable per the guidelines). While not wrong, moving the declaration inside the `if` block or to the top of the loop would be more consistent.

---

Review-Result: ERROR