|SUCCESS| [v8, 14/14] net/enetc4: add WRR Tx scheduler devarg for VF rings

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: ENETC4 Driver Enhancement Series (v8, Patches 01-14)

## Summary

This 14-patch series adds multiple enhancements to the ENETC4 PMD for i.MX95 NETC. The patches introduce KEEP_CRC Rx offload, TSO/LRO support, VF link speed extensions, firmware version reporting, register dumps, ring parameters, link state fixes, statistics reset, per-queue Rx interrupts, SI-based VLAN offload, link status bitmask encoding, TX PAUSE support, and WRR Tx scheduler configuration.

---

## Patch 01/14: KEEP_CRC Rx offload for ENETC4

**Correctness:**
- **Error** (resource leak on error path): In `enetc4_rx_queue_setup()`, if `enetc4_alloc_rxbdr()` fails after `rx_ring` is allocated, the code does `goto fail` which only frees `rx_ring`. However, the VF path has already parsed devargs and allocated structures (e.g., `hw->vf_link_legacy`) in earlier patches. This patch itself is safe, but the pattern should be checked in the full context. For this patch alone, the error path is correct.
- **Error** (use-after-free / double-free risk): In `enetc_rx_crc_trim()`, when `last_seg->data_len <= crc_len` and `prev_seg != NULL`, the code does `rte_pktmbuf_free_seg(last_seg)`, then the caller continues processing. The caller must not access `last_seg` after this free. Checking the call sites (`enetc_clean_rx_ring_nc` and `enetc_clean_rx_ring_cacheable`):
  - In both places, after `enetc_rx_crc_trim(first_seg, prev_seg, seg, rx_ring->crc_len);`, the code immediately does `rx_pkts[rx_frm_cnt] = first_seg; rx_frm_cnt++; first_seg = NULL;` and the loop advances to the next descriptor.
  - The `seg` pointer (which is `last_seg` in the trim call) is reassigned from `rx_swbd->buffer_addr` in the next iteration.
  - The freed segment is not accessed after `enetc_rx_crc_trim()` returns.
  - This is safe.

- **Info** (minor logic): `enetc_rx_crc_trim()` only handles the case where `last_seg->data_len <= crc_len`. If `last_seg->data_len > crc_len`, it subtracts directly. The `prev_seg != NULL` check protects against a single-segment packet where `prev_seg` would be NULL. For a single-segment packet, if `last_seg->data_len <= crc_len`, the `if (prev_seg != NULL)` guard prevents dereferencing NULL. However, if the entire packet is smaller than the CRC length in a single segment, the first branch `if (likely(last_seg->data_len > crc_len))` would be false, and `prev_seg` is NULL, so the `else if (prev_seg != NULL)` is also false. The function does nothing, leaving `first_seg->pkt_len` reduced by `crc_len` but the segment's `data_len` unchanged. This is incorrect: a single-segment packet shorter than `crc_len` would have a negative effective payload. 
  - **Correction**: The patch sets `crc_len = 4` only when KEEP_CRC is enabled. KEEP_CRC means the FCS is preserved in the buffer, so the packet length reported by HW includes the 4-byte FCS. The datapath subtracts `crc_len` from `pkt_len` and `data_len`. For a valid Ethernet frame (minimum 64 bytes on wire, 60 bytes without FCS), the payload is always >= 4 bytes. A frame shorter than 4 bytes is a runt and would be dropped by HW or flagged as an error. So `last_seg->data_len < 4` is not a reachable case in practice. The code is safe.

**Style/Process:**
- All correct.

**Verdict:** No errors.

---

## Patch 02/14: TSO support for ENETC4 VF

**Correctness:**
- **Warning** (LSO ring sizing): `enetc4_alloc_txbdr()` doubles `nb_desc` for LSO-enabled rings: `ring_desc = txr->lso_enable ? nb_desc * 2 : nb_desc;`. The check `if (ring_desc > MAX_BD_COUNT)` caps the total ring size. Then it logs `ENETC_PMD_ERR("LSO ring_desc %u > MAX_BD_COUNT %u; reduce nb_desc to <= %u")`. The suggested maximum is `MAX_BD_COUNT / 2` (since `nb_desc * 2` must fit). This is correct.
- **Info** (integer overflow prevention): `ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;` - the cast to `uint32_t` happens before the multiply. `nb_desc` is `uint16_t` (max 65535), so `nb_desc * 2` as `uint32_t` is at most 131070, well under `MAX_BD_COUNT` (512 or 1024 typically). No overflow. Correct.
- **Info** (TSO header length validation): `enetc_xmit_pkts_lso()` computes `hdr_len = seg->l2_len + seg->l3_len + seg->l4_len;` and checks `if (unlikely(hdr_len >= rte_pktmbuf_pkt_len(seg) || hdr_len > rte_pktmbuf_data_len(seg)))`. The first condition catches zero or negative payload; the second ensures headers are contiguous in the first segment. Both are necessary and correct.
- **Info** (TSO data unit validation): The code checks `if (unlikely(seg->tso_segsz == 0 || data_unit > ENETC4_LSO_MAX_DATA_UNIT || hdr_len + seg->tso_segsz > ENETC4_LSO_MAX_FRAME))`. All three conditions are necessary:
  - `tso_segsz == 0`: prevents division by zero or infinite loop.
  - `data_unit > 256KB`: HW limit.
  - `hdr_len + tso_segsz > 9600`: per-segment frame size limit.
  - Correct.

- **Error** (LSO incompatible with KEEP_CRC check): `enetc4_tx_queue_setup()` checks `if (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO))` and then `if (data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_KEEP_CRC)` returns `-EINVAL`. This is a **cross-direction offload check** (Tx offload checking Rx offload). The comment says "LSO needs HW FCS insertion, so it is incompatible with KEEP_CRC on Rx." This is correct: KEEP_CRC on Rx means the FCS is preserved in received packets, but TSO on Tx requires the HW to insert the FCS in the segmented frames. The two are incompatible because the port-level FCS handling mode cannot be both strip (for TSO Tx) and preserve (for KEEP_CRC Rx) simultaneously on ENETC4. The check is correct and necessary.

- **Info** (LSO decision logic): The code decides LSO enable from `data->dev_conf.txmode.offloads` (port level) rather than `tx_conf->offloads` (queue level) to keep all queues consistent. This is correct per the comment: "The Tx burst is a single device-level function pointer, so it must be consistent for all queues."

**Style/Process:**
- All correct.

**Verdict:** No errors.

---

## Patch 03/14: RSC (hardware LRO) support for ENETC4

**Correctness:**
- **Warning** (RSC ring sizing): `enetc4_alloc_rxbdr()` doubles `nb_desc` for RSC rings: `ring_desc = rxr->rsc_enable ? nb_desc * 2 : nb_desc;`. The check `if (ring_desc > MAX_BD_COUNT)` caps it. The error message suggests `MAX_BD_COUNT / 2` maximum `nb_desc`. Correct.
- **Info** (RBaLENR programming): The code programs `enetc4_rxbdr_wr(hw, idx, ENETC_RBLENR, ENETC_RTBLENR_LEN(rx_ring->rsc_enable ? rx_ring->bd_count / 2 : rx_ring->bd_count));`. Since `bd_count` is already doubled for RSC rings, `bd_count / 2` gives the original `nb_desc` (number of 32B descriptors). This is correct: the HW length register counts 32B descriptors, not 16B slots.
- **Info** (RBaRSCR preconditions): The code sets `ENETC4_RBMR_BDS` (32B descriptors) first, then programs `ENETC4_RBRSCR[EN]`. The comment says "Commit RBMR[BDS] to HW now (ring still disabled, EN clear) so the 32B descriptor mode is active before RBaRSCR[EN] is set. BDS = 1 is a hard RSC precondition: if RBaRSCR[EN] is written while BDS is still 0 in HW, RSC may be silently ignored." This is a HW ordering requirement. The code writes `ENETC_RBMR` with `ENETC4_RBMR_BDS` set but `EN` clear, then writes `RBRSCR`, then writes `RBMR` again with `EN` set. Correct.
- **Info** (ICTT programming): The code writes `ENETC4_RBICR1` (ICTT) first, then `ENETC4_RBICR0` (ICEN). The comment says "Program ICTT FIRST (with ICEN = 0) then enable ICEN, as the RM requires." Correct.
- **Error** (RSC incompatible with KEEP_CRC check): `enetc4_rx_queue_setup()` checks `if (rsc_enable)` and then `if (keep_crc)` returns `-EINVAL`. This is correct: RSC requires `RBaMR[CRC] = 0` (FCS stripped), which is the opposite of KEEP_CRC. The check is correct.
- **Error** (RSC incompatible with nc_mode check): The code also checks `if (adapter->hw.nc_mode)` and returns `-EINVAL` with message "RSC (LRO) is incompatible with nc=1 (non-cacheable mode)". This is correct: RSC uses the cacheable Rx path, and the non-cacheable path does not support the 32B descriptor layout.

- **Info** (`enetc_refill_rx_ring_rsc()` stride): The function walks the ring two slots at a time: `for (j = 0; j < buff_cnt; j += 2)`. Each iteration allocates one mbuf for the even slot and sets the odd slot's `buffer_addr` to NULL (extension slot never owns a buffer). The consumer index register is programmed as `i / 2` (32B descriptor count). Correct.
- **Info** (`enetc_clean_rx_ring_rsc()` stride): The function advances `i += 2` and invalidates every 4 slots (2 descriptors): `if ((i & ENETC_BD_PER_CL_MASK) == 0)` where `ENETC_BD_PER_CL_MASK = 3` (64B cache line = 4x16B slots = 2x32B descriptors). Correct.
- **Info** (RSC_FRAMES extraction): `rsc_frames = ENETC4_RXBD_EXT_RSC_FRAMES(rte_le_to_cpu_32(rxbd_ext->rsc_frames));` where the macro is `(x) & 0xff` (low 8 bits). The spec says RSC_FRAMES is in bits 7-0 of the rsc_frames word. Correct.
- **Info** (SIRXIDR W1C): The code writes `enetc4_wr_reg(rx_ring->rbidr, BIT(rx_ring->index));` to clear the ring's interrupt detect bit. The comment says "Write-1-to-clear this ring's event in SIRXIDR. Without this the interrupt-coalescing timer never re-arms and HW stops coalescing after the first RSC frame." This matches the HW behavior: the coalescing timer only runs while the detect bit is clear. Correct.

**Style/Process:**
- All correct.

**Verdict:** No errors.

---

## Patch 04/14: Extend PF-VF link speed field to 8 bits

**Correctness:**
- **Info** (speed code formula): The new encoding for speeds > 5Gbps is `(link_speed - 5000) / 1000 + ENETC_SPEED_5000`. For 10Gbps: `(10000 - 5000) / 1000 + 7 = 5 + 7 = 12`. For 25Gbps: `(25000 - 5000) / 1000 + 7 = 20 + 7 = 27`. The reverse formula is `(status - ENETC_SPEED_5000) * 1000 + 5000`. For status=12: `(12 - 7) * 1000 + 5000 = 10000`. For status=27: `(27 - 7) * 1000 + 5000 = 25000`. Correct.
- **Info** (vf_link_legacy devarg): The code adds a `vf_link_legacy` flag to select the old 4-bit layout. The `parse_vf_link_legacy()` function parses the devarg, checks the value is 0 or 1, and sets `hw->vf_link_legacy`. The `enetc4_msg_vsi_reply_msg()` and `enetc4_msg_get_psi_msg()` functions use the flag to decide whether to extract the status from the upper 4 bits of the low byte (legacy) or the full low byte (current). Correct.

**Style/Process:**
- All correct.

**Verdict:** No errors.

---

## Patch 05/14: Firmware version get for VF

**Correctness:**
- **Info** (`enetc4_vf_get_ip_minor_revision()` message parsing): The function sends a class 0xF0, cmd 0x1 message and parses the reply from `vsimsgsr`. It extracts the class ID from the upper 8 bits and the minor revision from the lower 8 bits: `mc = ENETC_SIMSGSR_GET_MC(vsimsgsr); class_id = (mc >> 8) & 0xff; *ip_mn = mc & 0xff;`. This matches the spec: the lower byte of MC carries the IP_MN value. Correct.
- **Info** (PCI revision ID read): `rte_pci_read_config(pci_dev, &ip_mj, sizeof(ip_mj), RTE_PCI_REVISION_ID)` reads the PCI revision ID register (offset 0x08). The return value is checked against `sizeof(ip_mj)` (1 byte). Correct.
- **Info** (version string formatting): The code uses `snprintf(NULL, 0, ...)` to compute the required buffer size when `fw_size == 0`, adding 1 for the trailing null. When `fw_size > 0`, it formats the string and checks if the result fits. Correct.

**Style/Process:**
- All correct.

**Verdict:** No errors.

---

## Patch 06/14: Registers dump

**