|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. Incorrect completion tag validity check

**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`  
**Line:** `if (unlikely(tag >= txq->nb_tx_desc))`

The completion tag is the sw_id/queue head from the hardware completion descriptor (`q_head`), not a tag index. The check `tag >= txq->nb_tx_desc` is wrong because `q_head` is already bounds-checked earlier in the function against `txq->nb_tx_desc`, and this "tag" is being used to index the shadow ring which is also sized to `nb_tx_desc`. However, the fundamental error is that the code assigns `tag = q_head` when it should be reading the tag from the descriptor's `compl_tag` field.

```c
/* BAD - q_head is the descriptor index, not the completion tag */
uint16_t tag = q_head;
if (unlikely(tag >= txq->nb_tx_desc)) {

/* GOOD - read the actual compl_tag field from the completion descriptor */
uint16_t tag = txd->compl_tag;
if (unlikely(tag >= txq->nb_tx_desc)) {
```

The patch claims to use "a software-defined compl_tag written to all data descriptors of a packet... and echoed back by HW on RS", but then reads `q_head` instead of `txd->compl_tag`. This breaks the entire shadow ring mechanism -- it will free the wrong mbuf, and the actual pending packet will leak.

### 2. Missing release notes update

The patch fixes a data corruption bug (Tx payload corruption) and changes the split-queue memory footprint (40 KiB -> 8 KiB per queue). Both are significant enough to warrant a release notes entry, but no `doc/guides/rel_notes/` changes are present.

**Required:** Add a "Fixed Issues" entry describing the Tx corruption fix and a note about reduced split-queue memory usage under the drivers/net/idpf section.

---

## WARNINGS

### 1. Incorrect `use_vec_entry` logic for split queues

**Files:** `drivers/net/intel/cpfl/cpfl_rxtx.c`, `drivers/net/intel/idpf/idpf_rxtx.c`  
**Lines:** 
```c
txq->use_vec_entry = (ad->tx_func_type != IDPF_TX_SINGLEQ &&
		ad->tx_func_type != IDPF_TX_DEFAULT);
```

For split queues, the patch routes mbuf drain through `use_vec_entry`: vector path calls `ci_txq_release_all_mbufs()`, scalar path drains the shadow ring. However, `use_vec_entry` is only set when a vector transmit function is selected (`ad->tx_func_type` is a vectorized variant). The scalar split-queue path (`IDPF_TX_DEFAULT`) will therefore have `use_vec_entry = false` and `idpf_qc_split_tx_release_mbufs()` will drain the shadow ring, which is correct.

But the condition is inverted in meaning: the code sets `use_vec_entry = true` when `tx_func_type` is NOT single-queue AND NOT default, which for split-queue mode means only when a vector function is selected. The naming is confusing because for split queues, `use_vec_entry = false` actually means "use scalar shadow ring drain", not "use vector entries". The logic is correct but the variable name is misleading.

Consider renaming or adding a comment clarifying that for split queues, `use_vec_entry` controls whether to use the vector `sw_ring_vec` drain or the scalar `tx_pending_pkts` drain, not whether the descriptor ring uses vector layout.

---

## INFO

### 1. Pre-existing: shadow ring drain logs to TX_LOG not DRV_LOG

**File:** `drivers/net/intel/idpf/idpf_common_rxtx.c`  
**Line:** `TX_LOG(ERR, "invalid completion tag %u.", tag);`

The error message for an out-of-range completion tag uses `TX_LOG` while other functions in this file use `DRV_LOG`. This is consistent with the existing split in the file (some functions use `TX_LOG`, others use `DRV_LOG`), so it's acceptable, but the inconsistency is worth noting for future cleanup.

---

## SUMMARY

The patch correctly addresses the root cause of split-queue Tx corruption by introducing a shadow ring indexed by completion tag, and the memory footprint optimization is sound. However, the completion tag extraction in `idpf_split_tx_free()` is broken (reads `q_head` instead of `txd->compl_tag`), which will cause the shadow ring to free the wrong mbufs and leak the correct ones. This is a correctness bug that breaks the entire fix. Additionally, release notes are missing for a user-visible bug fix and memory usage improvement.

Review-Result: ERROR