|WARNING| [v11, 15/15] net/enetc4: add WRR Tx scheduler devarg for VF rings

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-19

# DPDK Patch Review: net/enetc series.patch

## Summary
This 15-patch series adds extensive new features to the ENETC4 PMD for NXP i.MX95:
- KEEP_CRC Rx offload
- TSO (Large Send Offload) for VF
- RSC (hardware LRO) for PF and VF
- Extended PF-VF link speed encoding (4-bit - 8-bit)
- Firmware version reporting for VF
- Register dump support
- Ring parameters (queue info)
- VF link speed refresh on link-up interrupt
- VF statistics reset
- Per-queue Rx MSI-X interrupts for VF
- SI-based port VLAN insertion/removal
- VF link status bitmask encoding
- VF Tx PAUSE via Rx congestion mode
- WRR Tx scheduler devarg for VF

---

## Correctness Issues (Errors)

### Patch 1: KEEP_CRC Rx offload
**1. enetc_rx_crc_trim() segment drop without freeing chain** (Error)  
In `enetc_rxtx.c` `enetc_rx_crc_trim()`, when the FCS straddles two segments and the trailing segment is dropped:
```c
first_seg->nb_segs--;
prev_seg->data_len -= crc_len - last_seg->data_len;
prev_seg->next = NULL;
rte_pktmbuf_free_seg(last_seg);
```
If `last_seg->next` was non-NULL (i.e., the cluster had more than two segments and the FCS straddles the second-to-last and last), the segments beyond `last_seg` are leaked because `last_seg->next` is not walked and freed. `rte_pktmbuf_free_seg()` only frees the single segment, not a chain.

**Fix:**  
Before `rte_pktmbuf_free_seg(last_seg)`, free the tail chain:
```c
struct rte_mbuf *tail = last_seg->next;
if (tail)
    rte_pktmbuf_free(tail);
rte_pktmbuf_free_seg(last_seg);
```
or use `rte_pktmbuf_free(last_seg)` if you want to free the entire chain starting from `last_seg`.

---

### Patch 2: TSO support for ENETC4 VF
**1. LSO ring sizing arithmetic may silently truncate on 16-bit nb_desc** (Error)  
In `enetc4_alloc_txbdr()`:
```c
ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
```
If `nb_desc` is a `uint16_t` (common in DPDK queue setup), the multiplication `nb_desc * 2` is performed at 16-bit width before the cast to `uint32_t`, so `nb_desc = 0x8001` would overflow to `0x0002` before widening. The cast should precede the multiply:
```c
ring_desc = txr->lso_enable ? ((uint32_t)nb_desc * 2) : (uint32_t)nb_desc;
```
The same pattern appears in patch 3 for RSC rings. Fix both.

**2. LSO zero-length segment traversal infinite loop** (Error)  
In `enetc_xmit_pkts_lso()`, the payload BD loop:
```c
while (dseg) {
    /* ... */
    if (dlen == 0) {
        dseg = dseg->next;
        continue;
    }
    /* write BD for this segment */
    /* ... */
    dseg = dseg->next;
}
```
If the mbuf chain contains consecutive zero-length segments (e.g., malformed packet from application), the loop advances `dseg` but never increments `i` (the ring index), so `bds_to_use` is never decremented for these segments. If there are more zero-length segments than remaining BDs, the loop never breaks on `bds_to_use < bds_needed` and writes past the ring end when `i` wraps.

**Fix:**  
Move the `i++; bds_to_use--;` outside the `if (dlen == 0)` conditional so every segment consumes a ring slot regardless of length, or add a zero-length segment limit check.

---

### Patch 3: RSC (hardware LRO) support
**1. RSC ring sizing same 16-bit multiply truncation as patch 2** (Error)  
In `enetc4_alloc_rxbdr()`:
```c
ring_desc = rxr->rsc_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
```
Same issue as patch 2: cast `nb_desc` before the multiply.

**2. RSC refill: odd nb_desc produces wrong ring size check** (Error)  
The RSC refill walks two slots at a time, but `enetc4_alloc_rxbdr()` sets `rxr->bd_count = ring_desc`. If the user passes an odd `nb_desc`, `ring_desc` is `odd * 2 = even`, but `enetc_refill_rx_ring_rsc()` checks `if (unlikely(i == rx_ring->bd_count))` after advancing `i += 2`. If `bd_count` is even but not a multiple of 4, `i` can exceed `bd_count` by 1 before the check triggers, accessing an out-of-bounds descriptor.

**Fix:**  
Either enforce `nb_desc` is even/multiple-of-2 in `enetc4_rx_queue_setup()`, or round `ring_desc` up to the next even value, or change the refill wrap check to `if (unlikely(i >= bd_count))`.

---

### Patch 9: Refresh link speed on VF link-up interrupt
**1. enetc4_vf_get_link_speed() may be called with vsi_lock already held** (Error)  
In `enetc4_process_psi_msg()` (interrupt handler), when link-up is detected:
```c
if (!enetc4_vf_get_link_speed(eth_dev, msg) && ...)
```
`enetc4_vf_get_link_speed()` internally calls `enetc4_msg_vsi_send()`, which does `pthread_mutex_lock(&hw->vsi_lock)`. But `enetc4_process_psi_msg()` is itself the mailbox interrupt handler (registered in `enetc4_vf_dev_intr()`), and the kernel may have already delivered the mailbox interrupt, which the DPDK interrupt infrastructure services by calling the handler. If the handler or any caller already holds `vsi_lock`, this is a **recursive lock attempt** on a non-recursive mutex - **deadlock**.

Review the call path: `enetc4_dev_interrupt_handler()` - `enetc4_process_psi_msg()` does not appear to lock `vsi_lock` before calling `enetc4_vf_get_link_speed()`, so this is likely **safe** *if* the mailbox interrupt is masked while the handler runs (which is typical). However, the comment in the patch says "this PMD is poll-mode and never services the interrupt" for the RSC path, but here the link-status interrupt *is* being serviced. Verify that the EAL interrupt framework masks the mailbox MSI-X vector during the handler, or document that the link-status interrupt and any `link_update()` calls are serialized by a higher layer.

**Recommendation:** Add a comment explaining why the nested `vsi_lock` acquisition is safe (mailbox interrupt masked during handler), or use a try-lock and defer the speed query if the lock is busy.

---

### Patch 10: VF stats reset
**1. enetc4_rd64() busy-loop may stall interrupt thread** (Info - Warning)  
The new `enetc4_rd64()` function busy-waits in a `do { ... } while (hi != hi_check);` loop until the high word stabilizes. On a slow or congested interconnect, this could spin for many microseconds. Because `enetc4_vf_stats_reset()` and `enetc4_vf_stats_get()` call `enetc4_rd64()` on the data plane, a stall here would block the stats thread, but the review guidelines do not flag busy-waits in non-fast-path code unless they are unbounded. This one has an implicit bound (carry-resolves after two reads), so it is acceptable but worth noting.

**No change required**, but document that `enetc4_rd64()` may spin if the counter increments rapidly.

---

### Patch 14: Tx PAUSE via VF Rx congestion mode
**1. RBMR read-modify-write race between enetc4_vf_set_congestion_mode() and rx_queue_start/stop** (Error - Fixed)  
The patch adds `vsi_lock` serialization around RBMR writes in `enetc4_rx_queue_setup()`, `enetc4_rx_queue_start()`, and `enetc4_rx_queue_stop()`, but only when `device_id == ENETC4_DEV_ID_VF`. The PF code paths skip the lock. This is correct *if* `enetc4_vf_set_congestion_mode()` is only called for VFs (which it is, as it is part of the VF-specific link-status interrupt handler). However, there is a **potential race** if two threads call `rx_queue_start()` or `rx_queue_stop()` concurrently on different queues: the lock serializes them with `set_congestion_mode()`, but does not serialize them with each other. Since each queue has its own RBMR register, this is safe *unless* the RBMR registers alias or share a backing latch. The patch assumes per-queue RBMR registers are independent, which is the common hardware design. No action needed if this assumption holds.

**Verify:** Confirm that RBMR registers for different queues do not alias or share write latches. If they do, widen the lock scope.

---

### Patch 15: WRR Tx scheduler devarg
**1. txq_prior and txq_wrr leaks on dev_configure() re-entry** (Error - Fixed in patch)  
The updated `parse_txq_prior()` and new `parse_txq_wrr()` both free the old array before allocating a new one:
```c
rte_free(hw->txq_prior);
hw->txq_prior = rte_zmalloc(...);
```
This is correct. The `enetc4_dev_close()` code in the final hunk frees both arrays:
```c
rte_free(hw->txq_prior);
hw->txq_prior = NULL;
rte_free(hw->txq_wrr);
hw->txq_wrr = NULL;
```
No leak. The code is correct.

---

## Style and Process Issues (Warnings)

### Patch 1: KEEP_CRC
**1. Unnecessary variable initialization** (Info)  
In `enetc_clean_rx_ring_nc()` and `enetc_clean_rx_ring_cacheable()`:
```c
struct rte_mbuf *first_seg = NULL, *cur_seg = NULL, *prev_seg = NULL;
```
The variables are always assigned before use (inside the `if (!first_seg)` branch), so the `= NULL` initializers are not needed. However, the compiler may not prove this, and zeroing three pointers is cheap. This is acceptable.

**No change required.**

---

### Patch 2: TSO
**1. LSO ring size check message hardcodes MAX_BD_COUNT** (Warning)  
In `enetc4_alloc_txbdr()`:
```c
ENETC_PMD_ERR("LSO ring_desc %u > MAX_BD_COUNT %u; reduce nb_desc to <= %u with LSO enabled",
              ring_desc, MAX_BD_COUNT, MAX_BD_COUNT / 2);
```
The message suggests `nb_desc <= MAX_BD_COUNT / 2`, which is correct for LSO. Clear and actionable. No issue.

---

### Patch 3: RSC
**1. RSC refill uses RTE_MIN() without rte_common.h** (Info)  
In `enetc_refill_rx_ring_rsc()`:
```c
int m_cnt = RTE_MIN(want, ENETC_RXBD_BUNDLE);
```
`RTE_MIN` is defined in `<rte_common.h>`, which is included by `enetc.h`. No issue.

---

### Patch 4: Extend PF-VF link speed field
**1. Link speed switch statement replaced with formula** (Info)  
The patch replaces the explicit `case ENETC_SPEED_10G: ...` branches with a formula-based calculation. This is a code-size win and makes adding new speeds trivial. The comment explains the rationale. No issue.

**2. vf_link_legacy devarg parsing duplicates boilerplate** (Info)  
The `parse_vf_link_legacy()` function is structurally identical to other devarg parsers (strtoul, range check, assignment). This is acceptable repetition for a simple parameter. No issue.

---

### Patch 9: VF link speed refresh
**1. enetc4_decode_link_speed() comment references "Correction:"** (Error in guidelines - fixed here)  
The code does not contain any "(Correction: ...)" phrases, so this guideline violation does not apply.

---

### Patch 10: VF stats reset
**1. enetc4_rd64() uses uint64_t shift without cast** (Info)  
```c
return (uint64_t)hi << 32 | lo;
```
The `hi << 32` is performed on a `uint64_t` (because of the cast), so no truncation. Correct.

---

### Patch 11: Per-queue Rx interrupt support
**1. MSI-X vector allocation error handling** (Info)  
In `enetc4_vf_dev_intr()`, if `rte_intr_efd_enable()` or `rte_intr_vec_list_alloc()` fails, the code logs a warning, sets `hw->rxq_intr_en = 0`, and continues without Rx interrupts. This is safe: the ops table's `rx_queue_intr_enable` will return `-ENOTSUP` when `rxq_intr_en` is 0. No issue.

---

### Patch 13: VF link status bitmask
**1. Missing rte_free() on error path in original code** (Error - fixed in patch)  
The patch adds `rte_free(reply_msg);` on two error-return paths in `enetc4_vf_link_update()`. This fixes a pre-existing leak. Good catch.

---

## Process and Documentation

**1. Release notes comply with guidelines** (OK)  
Each patch with user-visible changes updates `doc/guides/rel_notes/release_26_11.rst`. The notes are concise and accurate.

**2. Features matrix updated** (OK)  
Patches 1, 2, 3, 6, 7, 11 add entries to `doc/guides/nics/features/enetc4.ini` and `enetc4_vf.ini` as appropriate. The additions match the documented features.

**3. No new API marked __rte_experimental** (OK)  
The patches add VF-specific ops but do not introduce new public API functions. The existing ops table registration is sufficient.

**4. Devargs documented** (OK)  
Patches 4, 8, 11, 15 add devargs (`vf_link_legacy`, `enetc4_txq_wrr`) and document them in `doc/guides/nics/enetc4.rst` with usage examples. Clear and complete.

**5. Commit message formatting** (Not reviewed per guidelines)  
SPDX/copyright and commit message formatting are handled by `checkpatches.sh` and not flagged here.

---

## Final Recommendations

**Errors (must fix):**

1. **Patch 1:** Fix `enetc_rx_crc_trim()` segment chain leak when