|WARNING| [v2] 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/170802

_AI Code Review_

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

# DPDK Patch Review: net/intel: fix idpf Tx payload corruption in split queue

## Summary

This patch fixes a critical payload corruption bug in the idpf split-queue Tx path by introducing a shadow ring (`tx_pending_pkts[]`) indexed by `compl_tag` to track mbuf ownership until RS completion. The core issue was that `sw_ring[]` slots were being reused before RS completion, causing the RS-completion walker to free mbufs still in flight.

---

## Errors

### 1. Resource leak on `idpf_tx_queue_setup()` failure path

**Location:** `drivers/net/intel/idpf/idpf_rxtx.c:502-515`

**Issue:**  
If `idpf_qc_split_tx_complq_setup()` fails (goto `err_complq_setup`), the code frees `tx_pending_pkts` and `rs_last_id`, but does NOT free `sw_ring`, which was allocated earlier. The error path should match the allocation order.

**Fix:**  
```c
err_complq_setup:
	idpf_qc_split_tx_pending_free(txq);
err_pending_alloc:
	rte_free(txq->rs_last_id);
err_rs_last_id_alloc:
	rte_free(txq->sw_ring);
err_sw_ring_alloc:
	rte_memzone_free(mz);
	return ret;
```

The same pattern exists in `cpfl_tx_queue_setup()` at lines 645-651 and should be verified there as well.

---

### 2. Bounds check on `tag` uses wrong variable

**Location:** `drivers/net/intel/idpf/idpf_common_rxtx.c:878`

**Issue:**  
The code checks `if (unlikely(tag >= txq->tx_pending_size))` but `tag` is assigned from `q_head`, which is bounded by `txq->sw_nb_desc`, not `txq->tx_pending_size`. The patch sets `tx_pending_size = nb_tx_desc`, but if `sw_nb_desc != nb_tx_desc` (which can happen in split queue configurations), the bounds check is incorrect.

**Analysis:**  
>From the code flow:
- `tag = q_head;`  
- `q_head` comes from `txd->qw1.compl_tag` (line 867: `q_head = rte_le_to_cpu_16(txd->qw1.compl_tag);`)  
- The transmit path writes `txd->qw1.compl_tag = tag;` where `tag = txq->tx_next_compl_tag`  
- `tx_next_compl_tag` wraps at `tx_pending_size` (lines 1096-1097)

So the bounds check is correct IF the HW is not returning corrupted values. However, defensive programming would validate against the ring's actual bounds.

**Recommended fix:**  
```c
if (unlikely(tag >= txq->tx_pending_size)) {
	TX_LOG(ERR, "invalid completion tag %u (max %u).",
	       tag, txq->tx_pending_size - 1);
```
Change is minor (add max value to error message for diagnostics).

---

## Warnings

### 1. Missing error check on `idpf_qc_split_tx_complq_setup()`

**Location:** `drivers/net/intel/cpfl/cpfl_rxtx.c:633-645`

**Issue:**  
The return value of `idpf_qc_split_tx_complq_setup()` is not checked before the function returns success. If `idpf_qc_split_tx_complq_setup()` fails, the queue setup will appear to succeed but the completion queue will not be initialized, leading to crashes or hangs on first Tx.

**Fix:**  
```c
ret = idpf_qc_split_tx_complq_setup(cpfl_txq);
if (ret != 0)
	goto err_complq_setup;
```
(The `idpf_tx_queue_setup()` function at line 503-507 already does this correctly; `cpfl_tx_queue_setup()` should match.)

---

### 2. `tx_pending_pkts[]` allocation size mismatch documentation

**Location:** `drivers/net/intel/common/tx.h:264-267`

**Issue:**  
The comment says "Sized nb_tx_desc" but the actual allocation at `idpf_common_rxtx.c:253` uses `txq->nb_tx_desc`. For split queues, `nb_tx_desc` (descriptor ring size) and `sw_nb_desc` (sw_ring size) can differ. The comment should clarify which `nb_tx_desc` field is meant.

**Suggested fix:**  
```c
/* Split-Tx completion tracking: shadow ring indexed by a
 * rolling compl_tag decouples RS-completion mbuf lifetime
 * from sw_ring[] slot reuse. Sized to txq->nb_tx_desc to
 * bound pending RS completions within the 2*nb_tx_desc complq.
 */
```

---

### 3. Redundant NULL check in reset path

**Location:** `drivers/net/intel/idpf/idpf_common_rxtx.c:237-243`

**Issue:**  
The code checks `if (txq->tx_pending_pkts != NULL)` before iterating, but then unconditionally iterates over `tx_pending_size` entries. If `tx_pending_pkts` is NULL, `tx_pending_size` should be 0 (as set by `idpf_qc_split_tx_pending_free()`), but the code does not enforce this invariant.

**Recommendation:**  
Either:
1. Set `txq->tx_pending_size = 0;` in `idpf_qc_split_tx_pending_free()`, or  
2. Move the NULL check inside the loop (less efficient), or  
3. Add an early return if NULL (current pattern is acceptable but could be clearer).

Current code is safe but defensive; no change required if you verify `tx_pending_size` is always 0 when `tx_pending_pkts` is NULL.

---

### 4. Potential unbounded loop in Tx path

**Location:** `drivers/net/intel/idpf/idpf_common_rxtx.c:1024-1028`

**Issue:**  
When `txq->tx_pending_pkts[tag]` is occupied, the code calls `idpf_split_tx_free()` in a loop (`nb_to_clean = 2 * txq->tx_rs_thresh; while (nb_to_clean--) ...`). If HW stops returning completions (e.g., due to a MAC stall), this loop will run to completion but not free the slot, and the outer transmit loop will `break`, stopping Tx.

**Analysis:**  
This is acceptable behavior (stop Tx when completions stall), but:
- The magic number `2 * txq->tx_rs_thresh` is not explained. Why 2x?  
- If HW is broken, the loop consumes CPU without progress.

**Recommendation:**  
Add a comment explaining the `2 * txq->tx_rs_thresh` heuristic (e.g., "drain up to two RS batches worth of completions to make forward progress").

---

### 5. Error message missing newline

**Location:** `drivers/net/intel/idpf/idpf_common_rxtx.c:253`

**Issue:**  
The error message `"Failed to alloc idpf tx_pending shadow ring"` does not end with a newline. While `DRV_LOG` may add one, DPDK convention is to include `\n` explicitly in log strings.

**Suggested fix:**  
```c
DRV_LOG(ERR, "Failed to alloc idpf tx_pending shadow ring\n");
```
(Check if `DRV_LOG` macro already appends newline; if so, this is not needed.)

---

## Info

### 1. Consider adding a compile-time assert on tag size

**Location:** `drivers/net/intel/idpf/idpf_common_rxtx.c:253-260`

**Observation:**  
The `compl_tag` field in the descriptor is 16 bits (`txd->qw1.compl_tag`). The patch sizes `tx_pending_pkts` to `nb_tx_desc`, which can be up to 4096 (typical max for DPDK Tx queues). If `nb_tx_desc > 65536`, the tag wraps, but DPDK typically limits descriptor counts to 4K-8K, so this is safe.

**Suggestion:**  
Add a static or runtime assert:
```c
RTE_BUILD_BUG_ON(sizeof(txd->qw1.compl_tag) < sizeof(uint16_t));
```
or in setup:
```c
if (txq->nb_tx_desc > UINT16_MAX) {
	DRV_LOG(ERR, "tx_pending_size %u exceeds compl_tag width\n",
	        txq->nb_tx_desc);
	return -EINVAL;
}
```
(This is a defensive check; typical DPDK queue sizes are well below 64K.)

---

### 2. Consider using `rte_pktmbuf_free_bulk()` for cleanup

**Location:** `drivers/net/intel/idpf/idpf_common_rxtx.c:275-278`

**Observation:**  
The reset path frees mbufs individually in a loop. If mbufs are from the same pool, `rte_pktmbuf_free_bulk()` would be faster. However, since this is a slow-path (queue stop/reset), the current approach is acceptable.

---

### 3. Naming: `tx_pending_pkts` vs `tx_pending_mbufs`

**Observation:**  
The array is named `tx_pending_pkts` but it stores `struct rte_mbuf *` (which may be multi-segment). The name is slightly misleading -- it's "pending packet head mbufs" not "pending packets" (packets are freed via `rte_pktmbuf_free()` which walks `mbuf->next`).

**Suggestion:**  
Rename to `tx_pending_mbufs` or `tx_pending_heads` for clarity. (Low priority; current name is understandable in context.)

---

## Correctness Summary

The patch correctly fixes the root cause (decoupling mbuf lifetime from `sw_ring[]` slot reuse). The main correctness concerns are:

1. **Missing `sw_ring` free on error path** (Error #1) -- must be fixed.  
2. **Missing error check on `idpf_qc_split_tx_complq_setup()`** (Warning #1) -- should be fixed.  

The rest are code quality improvements or clarifications.

---

## Final Recommendation

**Approve with changes:**
- Fix Error #1 (add `sw_ring` free to error path in both `idpf_tx_queue_setup` and `cpfl_tx_queue_setup`).  
- Fix Warning #1 (add error check on `idpf_qc_split_tx_complq_setup()` in `cpfl_tx_queue_setup`).  
- Consider adding a comment explaining the `2 * txq->tx_rs_thresh` heuristic (Warning #4).  

The core fix is sound and addresses a critical data corruption bug.