|WARNING| [v10, 14/14] 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/167859

_AI Code Review_

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

# DPDK Patch Review: ENETC4 Driver Enhancement Series

This review covers patches 01/14 through 14/14 of the ENETC4 PMD enhancement series.

---

## PATCH 01/14: net/enetc: add keep-CRC Rx offload for ENETC4

### Errors

**1. Error: Logic error in scatter CRC trim**
```c
/* drivers/net/enetc/enetc_rxtx.c:549 */
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);
	}
}
```

**Issue:** When `last_seg->data_len <= crc_len` but `prev_seg == NULL`, the function silently fails to trim the CRC. This occurs on single-segment frames where `data_len < crc_len` (corrupted/malformed packet). The function should either handle this case (drop the packet, trim what's available) or document that the caller must guarantee `last_seg->data_len > 0`.

**Suggested fix:** Add an else branch to handle the impossible/corrupt case, or at minimum add a comment documenting the caller's contract.

```c
} else if (prev_seg != NULL) {
	/* multi-segment: drop last seg, trim carry-over from prev */
	...
} else {
	/* Single-seg with data_len < crc_len: malformed.
	 * Trim what's available; caller must validate pkt_len >= crc_len.
	 */
	last_seg->data_len = 0;
}
```

**2. Error: MTU set writes RBMR without ring-enable**
```c
/* drivers/net/enetc/enetc4_ethdev.c:719 */
if (!rx_conf->rx_deferred_start) {
	rx_enable |= ENETC_RBMR_EN;
	enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC_RBMR,
		       rx_enable);
	dev->data->rx_queue_state[rx_ring->index] =
		       RTE_ETH_QUEUE_STATE_STARTED;
} else {
	enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC_RBMR,
		       rx_enable);  /* <- writing with EN clear? */
	dev->data->rx_queue_state[rx_ring->index] =
		       RTE_ETH_QUEUE_STATE_STOPPED;
}
```

**Issue:** The deferred-start path writes `rx_enable` (which has `ENETC4_RBMR_CRC` set/clear but EN=0) to RBMR. This programs the CRC bit on a stopped ring. While likely harmless (the ring isn't active), it's inconsistent: the comment says "enable ring" but the code explicitly does *not* enable when deferred. Either remove the write in the `else` branch (defer all RBMR setup until `rx_queue_start`), or clarify why CRC must be programmed even on a stopped ring.

**Suggested fix:** Only write RBMR when the ring is being started. For deferred rings, defer all RBMR configuration until `rx_queue_start()`.

```c
if (!rx_conf->rx_deferred_start) {
	rx_enable |= ENETC_RBMR_EN;
	enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC_RBMR, rx_enable);
	dev->data->rx_queue_state[rx_ring->index] = RTE_ETH_QUEUE_STATE_STARTED;
} else {
	/* Defer all RBMR setup (including CRC bit) until rx_queue_start */
	dev->data->rx_queue_state[rx_ring->index] = RTE_ETH_QUEUE_STATE_STOPPED;
}
```

---

## PATCH 02/14: net/enetc: add TSO support for ENETC4 VF

### Errors

**1. Error: LSO ring allocation integer overflow**
```c
/* drivers/net/enetc/enetc4_ethdev.c:322 */
ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
```

**Issue:** `nb_desc` is `uint16_t`. Multiplying by 2 happens in `int` (32-bit signed) before the cast to `uint32_t`. If `nb_desc >= 0x8000`, the multiplication overflows the signed int, producing a negative value that then gets zero-extended to `uint32_t`, yielding a wrong count. While the subsequent `> MAX_BD_COUNT` check catches most cases, this is the wrong pattern for a size calculation.

**Suggested fix:** Widen `nb_desc` before the multiply.
```c
ring_desc = txr->lso_enable ? ((uint32_t)nb_desc * 2) : (uint32_t)nb_desc;
```

---

## PATCH 03/14: net/enetc: add RSC (hardware LRO) support for ENETC4

### Errors

**1. Error: RSC ring allocation integer overflow**
```c
/* drivers/net/enetc/enetc4_ethdev.c:533 */
ring_desc = rxr->rsc_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
```

**Issue:** Same as the LSO ring allocation: `nb_desc` is `uint16_t`, multiply happens in `int`, overflow at `nb_desc >= 0x8000`.

**Suggested fix:** Widen before multiply.
```c
ring_desc = rxr->rsc_enable ? ((uint32_t)nb_desc * 2) : (uint32_t)nb_desc;
```

**2. Error: RSC RBMR write without ring-enable (deferred-start path)**
```c
/* drivers/net/enetc/enetc4_ethdev.c:724 */
if (!rx_conf->rx_deferred_start) {
	/* enable ring; apply congestion mode if TX PAUSE is already active. */
	rx_enable |= ENETC_RBMR_EN;
	...
} else {
	enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC_RBMR,
		       rx_enable);  /* writes RBMR with EN=0 */
	dev->data->rx_queue_state[rx_ring->index] =
		       RTE_ETH_QUEUE_STATE_STOPPED;
}
```

**Issue:** Same as PATCH 01: the deferred-start path writes RBMR without EN set. This programs BDS=1 (32B descriptors) and CRC=0 on a stopped ring. While likely harmless, it's inconsistent with the "defer all setup until start" model. Either remove the write or document why BDS must be set before RBaRSCR[EN].

**Suggested fix:** Defer all RBMR setup until `rx_queue_start()` for deferred rings.

---

## PATCH 04/14: net/enetc: extend PF-VF link speed field to 8 bits

### Warnings

**1. Warning: Complex speed decode duplicated in two functions**

The speed decode logic (switch on speed code, handle legacy vs current layout) is duplicated in `enetc4_decode_link_speed()` and was previously in `enetc4_vf_link_update()`. While the patch correctly refactors most of it into a shared helper, the same decode appears in both the interrupt handler (`enetc4_process_psi_msg`) and the poll path (`enetc4_vf_link_update`). This is now correct, but the repetition of the "formula-based decode" comment in both call sites is unnecessary.

**Suggested improvement:** The refactoring is good. No code change needed, but consider consolidating the long formula comment into `enetc4_decode_link_speed()`'s header so it's not repeated at every call site.

---

## PATCH 05/14: net/enetc: support firmware version get for VF

No issues found. The patch correctly handles the two-step read (PCI revision for major, PSI mailbox for minor), falls back gracefully when PSI reports `ENETC_IP_VER_NOT_AVAILABLE`, and implements the `fw_version_get` size-query protocol correctly.

---

## PATCH 06/14: net/enetc: support registers dump

No issues found. The patch correctly sizes the output buffer, reports register count and width, and handles both PF and VF register sets.

---

## PATCH 07/14: net/enetc: support ethtool ring parameters

No issues found. The patch simply registers the existing `enetc4_rxq_info_get` and `enetc4_txq_info_get` ops in the VF ops tables, making them non-static. This is a straightforward change with no functional risk.

---

## PATCH 08/14: net/enetc: refresh link speed on VF link-up interrupt

### Errors

**1. Error: Speed re-query on link-up without error handling**
```c
/* drivers/net/enetc/enetc4_vf.c:470 (in enetc4_process_psi_msg) */
memset(msg, 0, sizeof(*msg));
if (!enetc4_vf_get_link_speed(eth_dev, msg) &&
    msg->class_id == ENETC_CLASS_ID_LINK_SPEED)
	enetc4_decode_link_speed(msg->status,
				hw->vf_link_legacy,
				&link);
```

**Issue:** If `enetc4_vf_get_link_speed()` succeeds but the class_id is *not* `ENETC_CLASS_ID_LINK_SPEED` (e.g., a PSI firmware bug or mailbox corruption), the function silently skips the speed decode and leaves `link.link_speed` and `link.link_duplex` uninitialized. The subsequent `rte_eth_linkstatus_set()` then writes garbage into the cached link state.

**Suggested fix:** Initialize `link` to a safe default (e.g., `RTE_ETH_SPEED_NUM_UNKNOWN`, `RTE_ETH_LINK_FULL_DUPLEX`) before the speed query, or explicitly handle the error case.

```c
/* Initialize to safe defaults before speed query */
link.link_speed = RTE_ETH_SPEED_NUM_UNKNOWN;
link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
memset(msg, 0, sizeof(*msg));
if (!enetc4_vf_get_link_speed(eth_dev, msg)) {
	if (msg->class_id == ENETC_CLASS_ID_LINK_SPEED)
		enetc4_decode_link_speed(msg->status, hw->vf_link_legacy, &link);
	else
		ENETC_PMD_WARN("Link-up speed query returned wrong class_id 0x%x",
			       msg->class_id);
}
```

---

## PATCH 09/14: net/enetc: support stats reset for VF

No issues found. The software snapshot/delta approach is appropriate given the read-only HW counters on a VF.

---

## PATCH 10/14: net/enetc4: add per-queue Rx interrupt support for VF

### Errors

**1. Error: Unchecked return value from `rte_intr_vec_list_index_set()`**
```c
/* drivers/net/enetc/enetc4_vf.c:1975 */
for (i = 0; i < nb_rx; i++)
	rte_intr_vec_list_index_set(intr_handle, i,
		i + ENETC4_VF_RX_VEC_BASE);
```

**Issue:** `rte_intr_vec_list_index_set()` can return `-EINVAL` if the index is out of range. The loop does not check the return value, so a failure is silently ignored and the interrupt mapping is incomplete.

**Suggested fix:** Check the return value and fail setup on error.
```c
for (i = 0; i < nb_rx; i++) {
	ret = rte_intr_vec_list_index_set(intr_handle, i,
					   i + ENETC4_VF_RX_VEC_BASE);
	if (ret) {
		ENETC_PMD_ERR("Failed to set intr vec index %u: %d", i, ret);
		rte_intr_vec_list_free(intr_handle);
		rte_intr_efd_disable(intr_handle);
		hw->rxq_intr_en = 0;
		break;
	}
}
```

---

## PATCH 11/14: net/enetc4: add SI-based port VLAN insertion and removal

No issues found. The patch correctly programs the PSIaVLANR and PSIaCFGR0 registers on the PF, and forwards the request to the PSI via mailbox class 0x24 on the VF. The VF error handling correctly interprets the reply class_id (success = 0x1).

---

## PATCH 12/14: net/enetc4: update VF link status to bitmask encoding

No issues found. The refactoring from enum to bitmask is straightforward and maintains backward compatibility (ENETC_LINK_DOWN = BIT(0) = 0x1, same wire value).

---

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

### Errors

**1. Error: RBMR read-modify-write race between interrupt and queue start/stop**
```c
/* drivers/net/enetc/enetc4_vf.c:501 (enetc4_vf_set_congestion_mode) */
pthread_mutex_lock(&hw->vsi_lock);
rte_atomic_store_explicit(&hw->tx_pause_active, enable ? 1 : 0,
			  rte_memory_order_relaxed);
for (i = 0; i < nb_rx; i++) {
	rbmr = enetc4_rxbdr_rd(enetc_hw, i, ENETC_RBMR);
	if (enable)
		rbmr |= ENETC_RBMR_CM;
	else
		rbmr &= ~(uint32_t)ENETC_RBMR_CM;
	enetc4_rxbdr_wr(enetc_hw, i, ENETC_RBMR, rbmr);
}
pthread_mutex_unlock(&hw->vsi_lock);
```

**Issue:** The function correctly locks `vsi_lock` around the RBMR read-modify-write. However, `enetc4_rx_queue_start()` and `enetc4_rx_queue_stop()` in PATCH 03 were modified to also lock `vsi_lock` around their RBMR access *only when `device_id == ENETC4_DEV_ID_VF`*. This is correct *for PATCH 13*, but PATCH 03 added those locks *before* `enetc4_vf_set_congestion_mode()` existed. The PATCH 03 commit