|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 v11

This review covers patches 1-15 of the ENETC4 PMD series. I've examined each patch for correctness bugs, style compliance, API requirements, and guideline adherence per AGENTS.md.

---

## Summary

**Correctness Issues**: 2 errors  
**Process/Style Issues**: 0 warnings  
**Info**: 2 notes

---

## Patch 1: net/enetc: add keep-CRC Rx offload for ENETC4

### Errors

**1. Memory leak on error path in `enetc_rx_crc_trim()` (line 551)**

The function calls `rte_pktmbuf_free_seg(last_seg)` when the FCS straddles two segments, but if `prev_seg` is NULL (which the caller does allow via `prev_seg = NULL` initialization), the carry-over bytes are not trimmed from any segment and the detached `last_seg` is freed without updating the chain pointers. The cluster is left in an inconsistent state: `first_seg->nb_segs` is decremented but `prev_seg->next` is not updated (it would dereference NULL).

**Location**: `drivers/net/enetc/enetc_rxtx.c:544`

```c
static inline void
enetc_rx_crc_trim(struct rte_mbuf *first_seg, struct rte_mbuf *prev_seg,
		  struct rte_mbuf *last_seg, uint16_t crc_len)
{
	first_seg->pkt_len -= crc_len;
	if (likely(last_seg->data_len > crc_len)) {
		last_seg->data_len -= crc_len;
	} else if (prev_seg != NULL) {
		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 `prev_seg == NULL` and `last_seg->data_len <= crc_len`, the else-if does not execute. The function returns without freeing `last_seg` and without trimming the FCS from anywhere. The caller's scattered cluster now has a dangling `last_seg` still attached via `cur_seg->next`, but with no valid data and an inconsistent `nb_segs` count.

**Suggested fix**: Add an explicit branch for the impossible case (first segment is smaller than crc_len and there is no previous segment) to drop the frame or handle it as an error. Alternatively, document that the caller guarantees `prev_seg != NULL` whenever `last_seg->data_len <= crc_len`.

```c
} else if (prev_seg != NULL) {
	first_seg->nb_segs--;
	prev_seg->data_len -= crc_len - last_seg->data_len;
	prev_seg->next = NULL;
	rte_pktmbuf_free_seg(last_seg);
} else {
	/* Should not happen: a single-segment frame smaller than crc_len
	 * would have been handled by the likely() branch. Log and drop. */
	rte_pktmbuf_free(first_seg);
	return; /* or signal error */
}
```

---

## Patch 2: net/enetc: add TSO support for ENETC4 VF

### Info

**1. LSO validation in `enetc4_tx_queue_setup()` checks KEEP_CRC on Rx side (line 413)**

The LSO validation rejects the configuration when `RTE_ETH_RX_OFFLOAD_KEEP_CRC` is set on the Rx side, stating "LSO (TSO) is incompatible with KEEP_CRC". This is correct per the commit message ("LSO needs HW FCS insertion, so it is incompatible with KEEP_CRC on Rx"), but the condition is checking an **Rx offload** to gate a **Tx feature**. This cross-layer dependency is unusual and may confuse users.

**Location**: `drivers/net/enetc/enetc4_ethdev.c:420`

```c
if (data->dev_conf.rxmode.offloads &
    RTE_ETH_RX_OFFLOAD_KEEP_CRC) {
	ENETC_PMD_ERR("LSO (TSO) is incompatible with KEEP_CRC");
	rte_free(tx_ring);
	return -EINVAL;
}
```

**Suggested improvement**: Document this constraint in `doc/guides/nics/enetc4.rst` so users understand why an Rx offload affects Tx queue setup. The code is correct but the cross-domain check is noteworthy.

---

## Patch 3: net/enetc: add RSC (hardware LRO) support for ENETC4

### Errors

**1. Potential buffer overflow in `enetc_refill_rx_ring_rsc()` (line 917)**

The function walks the ring two slots at a time (`i += 2`) and checks `if (unlikely(i == rx_ring->bd_count))` to detect wrap. However, if `bd_count` is odd (which the validation logic does not explicitly forbid), the comparison `i == bd_count` is never true when `i` increments by 2. The loop would skip past `bd_count`, access `rx_ring->q_swbd[bd_count]` (one past the end), and corrupt memory.

**Location**: `drivers/net/enetc/enetc_rxtx.c:949`

```c
for (j = 0; j < buff_cnt; j += 2) {
	/* ... allocate mbuf ... */
	rx_swbd += 2;
	rxbd += 2;
	i += 2;

	if (unlikely(i == rx_ring->bd_count)) {  // BUG: never true if bd_count is odd
		dcbf((void *)grp_start_rxbd);
		i = 0;
		rxbd = ENETC_RXBD(*rx_ring, i);
		rx_swbd = &rx_ring->q_swbd[i];
		grp_start_rxbd = rxbd;
	} else if ((i & ENETC_BD_PER_CL_MASK) == 0) {
		dcbf((void *)grp_start_rxbd);
		grp_start_rxbd = rxbd;
	}
}
```

The validation in `enetc4_alloc_rxbdr()` rejects `ring_desc > MAX_BD_COUNT` but does not enforce that `ring_desc` (which is `nb_desc * 2` when `rsc_enable`) is even. If a user passes an odd `nb_desc` with RSC enabled, `ring_desc` becomes even, but if the multiplication overflows or if the `nb_desc` is already even, no check prevents an odd `bd_count` from reaching the refill loop.

**Suggested fix**: Add an assertion or explicit check that `rx_ring->bd_count` is even before entering the 2-slot stride loop, or change the wrap condition to `if (unlikely(i >= rx_ring->bd_count))`.

```c
if (unlikely(i >= rx_ring->bd_count)) {  // handles both even and off-by-one
```

---

## Patch 4: net/enetc: extend PF-VF link speed field to 8 bits

No issues found. The patch correctly extends the speed code from 4-bit to 8-bit and introduces the `vf_link_legacy` devarg for backward compatibility. The formula-based speed decoding is a clean improvement over the enum approach.

---

## Patch 5: net/enetc: add VF supported features file

No issues found. The patch adds `doc/guides/nics/features/enetc4_vf.ini` and updates `MAINTAINERS`. This is a documentation-only change and is correct.

---

## Patch 6: net/enetc: support firmware version get for VF

No issues found. The implementation correctly reads the PCI revision ID for the major version and queries the PSI for the minor version via VSI-PSI messaging. The fallback to "unknown" when the PSI does not support the command is appropriate.

---

## Patch 7: net/enetc: support registers dump

No issues found. The patch implements `.get_reg` for both PF and VF, dumping station interface, port (PF only), and per-ring BD registers. The version encoding in `regs->version` is a reasonable use of the field.

---

## Patch 8: net/enetc: support ethtool ring parameters

No issues found. The patch makes `enetc4_rxq_info_get()` and `enetc4_txq_info_get()` non-static and registers them in both VF ops tables. The nb_desc calculation correctly accounts for RSC (2x slots) and LSO (2x slots) rings.

---

## Patch 9: net/enetc: refresh link speed on VF link-up interrupt

No issues found. The patch correctly queries the link speed on link-up inside `enetc4_process_psi_msg()` and introduces a shared `enetc4_decode_link_speed()` helper. The mutex is already held by the caller, so the nested `enetc4_vf_get_link_speed()` call is serialized.

---

## Patch 10: net/enetc: support stats reset for VF

No issues found. The patch implements a software snapshot/delta approach for stats reset, which is the correct solution when the HW counters are read-only. The use of `enetc4_rd64()` (new helper for atomic 64-bit reads with carry detection) is appropriate.

---

## Patch 11: net/enetc4: add per-queue Rx interrupt support for VF

### Info

**1. Per-queue Rx interrupt setup calls `rte_intr_vec_list_alloc()` and `rte_intr_efd_enable()` before `rte_intr_enable()` (line 1997)**

The patch allocates the interrupt vector list and enables eventfds **before** calling `rte_intr_enable()`, which is the correct order per the DPDK interrupt API. This ensures vfio-pci can wire each MSI-X vector to its eventfd when programming the MSI-X table. The code is correct but the order is critical and worth noting.

**Location**: `drivers/net/enetc/enetc4_vf.c:1997`

```c
ret = rte_intr_efd_enable(intr_handle, nb_rx + ENETC4_VF_RX_VEC_BASE);
if (ret) { /* ... */ }
ret = rte_intr_vec_list_alloc(intr_handle, "enetc4_vf_rx_intr", nb_rx);
if (ret) { /* ... */ }
/* ... */
ret = rte_intr_enable(intr_handle);  // Must come after efd_enable + vec_list_alloc
```

This is good practice and matches the DPDK interrupt framework requirements. No change needed.

---

## Patch 12: net/enetc4: add SI-based port VLAN insertion and removal

No issues found. The patch implements VLAN insertion/removal via PSIaVLANR and PSIaCFGR0 registers for the PF, and forwards the request to the kernel PF via VSI-PSI mailbox (class 0x24) for the VF. The reply parsing correctly checks for `class_id == ENETC_MSG_CLASS_ID_CMD_SUCCESS` (0x1) rather than echoing the command class (0x24).

---

## Patch 13: net/enetc4: update VF link status to bitmask encoding

No issues found. The patch changes the link status from a two-value enum to a bitmask, which is a cleaner design for future extensions. The backward-compatible wire value (DOWN=0x1) is preserved.

---

## Patch 14: net/enetc4: enable Tx PAUSE via VF Rx congestion mode

No issues found. The patch correctly implements the PF-to-VF TX PAUSE signaling via BIT(1) of the link status message and applies ENETC_RBMR_CM to all active RX rings under the `vsi_lock` mutex. The use of `rte_atomic_load_explicit()`/`rte_atomic_store_explicit()` with `rte_memory_order_relaxed` is appropriate here because the `vsi_lock` already provides the necessary ordering.

---

## Patch 15: net/enetc4: add WRR Tx scheduler devarg for VF rings

No issues found. The patch adds `enetc4_txq_wrr` devarg to configure per-ring WRR weights in TBaMR. The implementation correctly clamps weights to 1..8, pre-shifts them into TBMR[WRR] bits [6:4], and applies them in `tx_queue_setup()` and `tx_queue_start()`. The memory allocated for `hw->txq_wrr` is freed in `enetc4_dev_close()`.

---

## Cross-Cutting Observations

1. **Mutex usage (vsi_lock)**: Introduced in patch 9 and extended in patch 14. The mutex is initialized as `PTHREAD_PROCESS_SHARED` in `enetc4_vf_dev_init()`, which is correct for shared-memory scenarios (primary/secondary processes). However, the mutex is only used by the VF and only in the primary process (the secondary process early-returns before the mutex is allocated). This is acceptable but the `PTHREAD_PROCESS_SHARED` attribute is not strictly required here since the VF does not share the mutex across processes.

2. **Secondary process support**: Multiple patches (6, 9) add early returns for secondary processes that skip hardware initialization but set burst function pointers. This is correct per the DPDK multi-process model.

3. **Release notes**: All patches correctly update `doc/guides/rel_notes/release_26_11.rst` with the new features. No release notes are missing.

---

## Conclusion

The series is well-structured and mostly correct. The two errors identified above (CRC trim logic in patch 1 and potential buffer overflow in patch 3) should be addressed before merging. The rest of the patches are solid.