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

## Summary

This patch series adds significant feature enhancements to the ENETC4 PMD for both PF and VF. The patches have been analyzed against the AGENTS.md guidelines, focusing on correctness bugs, coding style, API compliance, and documentation. Below are the findings organized by severity.

---

## Errors

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

**File: drivers/net/enetc/enetc4_ethdev.c**

**Line: ~570 (rx_queue_setup function)**

**Issue:** Dead store on `rx_enable` variable.

```c
/* enable ring */
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);  /* BUG: should not set EN flag here */
	dev->data->rx_queue_state[rx_ring->index] =
		       RTE_ETH_QUEUE_STATE_STOPPED;
}
```

On the deferred-start path (else branch), `rx_enable` is written to the register with the `ENETC_RBMR_EN` bit still set from the `if` block logic flow shown earlier in the patch. However, the earlier assignment `rx_enable |= ENETC_RBMR_EN;` is only inside the `if (!rx_conf->rx_deferred_start)` block, so this is actually correct. Wait--re-reading the diff:

```diff
+	if (keep_crc)
+		rx_enable |= ENETC4_RBMR_CRC;
+	else
+		rx_enable &= ~ENETC4_RBMR_CRC;
+
 	if (!rx_conf->rx_deferred_start) {
 		/* enable ring */
+		rx_enable |= ENETC_RBMR_EN;
 		enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC_RBMR,
-			       ENETC_RBMR_EN);
+			       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);
 		dev->data->rx_queue_state[rx_ring->index] =
 			       RTE_ETH_QUEUE_STATE_STOPPED;
 	}
```

The `rx_enable |= ENETC_RBMR_EN;` is only inside the `if` block, so on the `else` path `rx_enable` does **not** have the EN bit set. This is correct: deferred-start should program the CRC mode but not enable the ring. No error here.

---

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

**File: drivers/net/enetc/enetc_rxtx.c**

**Line: ~125-140 (enetc_xmit_pkts_lso function)**

**Issue:** Integer underflow risk on `first_payload` calculation.

```c
first_payload = (uint16_t)(seg_len - hdr_len);
```

The code already checks `hdr_len > seg_len` and skips the frame, but that check is at line ~87:

```c
if (unlikely(hdr_len >= rte_pktmbuf_pkt_len(seg) ||
	     hdr_len > rte_pktmbuf_data_len(seg))) {
	rte_pktmbuf_free(seg);
	start++;
	continue;
}
```

The condition is `hdr_len > seg_len`, which prevents the underflow on the subtraction at line ~147. No error.

---

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

**File: drivers/net/enetc/enetc_rxtx.c**

**Line: ~777 (enetc_clean_rx_ring_rsc function)**

**Issue:** Variable `first_seg` and `cur_seg` initialized to `NULL` at function scope but then used without checking if they are still `NULL` when `bd_status & ENETC_RXBD_LSTATUS_F` is true.

```c
struct rte_mbuf *first_seg = NULL, *cur_seg = NULL;
...
while (likely(rx_frm_cnt < work_limit)) {
	...
	if (!first_seg) {
		first_seg = seg;
		cur_seg = seg;
		...
	} else {
		first_seg->pkt_len += data_len;
		first_seg->nb_segs++;
		cur_seg->next = seg;
		cur_seg = seg;
	}
	...
	if (bd_status & ENETC_RXBD_LSTATUS_F) {
		seg->next = NULL;  /* seg is the current segment */
		ENETC_PMD_DP_DEBUG("RSC_FRAMES=%u pkt_len=%u nb_segs=%u",
				   rsc_frames, first_seg->pkt_len,  /* POSSIBLE NULL DEREF */
				   first_seg->nb_segs);
		rx_pkts[rx_frm_cnt] = first_seg;
		rx_frm_cnt++;
		first_seg = NULL;
	}
```

If the loop body is entered and `!first_seg` is true, `first_seg` is set to `seg`. On the next iteration (or the same iteration if `F` is set), `first_seg` is non-NULL. The only way to hit the `if (bd_status & ENETC_RXBD_LSTATUS_F)` block with `first_seg == NULL` is if no prior segment set it, which would require the `F` flag to be set on a descriptor where `R` was never set (invalid HW state). Given the loop logic, `first_seg` is always set before the `F` branch can execute. No error.

Wait--actually, the loop condition is `!(bd_status & ENETC_RXBD_LSTATUS_R)` which `break`s before `first_seg` is accessed, so the `R` bit must have been set to enter the block. Then `first_seg` is set in the `!first_seg` branch. No error.

---

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

**File: drivers/net/enetc/enetc4_vf.c**

**Line: ~468 (enetc4_process_psi_msg function)**

**Issue:** Potential double-free on `msg` if `enetc4_vf_get_link_speed()` fails and returns early after calling `rte_free(msg)` inside it, then the calling function also calls `rte_free(msg)` at the end.

Wait, let me check the code path:

```c
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);
```

Then at the end of `enetc4_process_psi_msg()`:

```c
end:
	rte_free(msg);
	return;
```

Now inside `enetc4_vf_get_link_speed()` (from the patch):

```c
msg = rte_zmalloc(NULL, sizeof(*msg), RTE_CACHE_LINE_SIZE);
if (!msg) {
	ENETC_PMD_ERR("Failed to alloc msg");
	return -ENOMEM;
}

msg_size = RTE_ALIGN(sizeof(struct enetc_msg_cmd_get_link_speed),
			ENETC_VSI_PSI_MSG_SIZE);
msg->vaddr = rte_zmalloc(NULL, msg_size, 0);
if (!msg->vaddr) {
	ENETC_PMD_ERR("Failed to alloc memory for msg");
	rte_free(msg);
	return -ENOMEM;
}
...
end:
	/* free memory no longer required */
	rte_free(msg->vaddr);
	rte_free(msg);
	return err;
```

Ah! `enetc4_vf_get_link_speed()` allocates its **own** `msg` structure internally and frees it at the end. The `msg` pointer in `enetc4_process_psi_msg()` is a **different** allocation. So there is no double-free. No error.

---

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

**File: drivers/net/enetc/enetc4_vf.c**

**Line: ~502 (enetc4_vf_set_congestion_mode function)**

**Issue:** Missing initialization of `enetc_hw` variable.

```c
static void
enetc4_vf_set_congestion_mode(struct rte_eth_dev *eth_dev, bool enable)
{
	struct enetc_eth_hw *hw =
		ENETC_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
	struct enetc_hw *enetc_hw = &hw->hw;  /* OK: hw is valid */
```

The `hw` pointer is obtained from `eth_dev->data->dev_private` which is always valid in a configured device. `enetc_hw` is the address of the embedded `hw.hw` struct. No error.

---

## Warnings

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

**File: drivers/net/enetc/enetc_rxtx.c**

**Line: ~550 (enetc_rx_crc_trim function)**

**Issue:** The CRC trim logic for scattered packets assumes `prev_seg` is non-NULL when `last_seg->data_len < crc_len`. However, if the cluster has only one segment (single-segment packet), `prev_seg` would be `NULL` and the pointer dereference would fault.

Actually, looking at the callers:

```c
enetc_rx_crc_trim(first_seg, prev_seg, seg, rx_ring->crc_len);
```

where `prev_seg` is tracked in the loop:

```c
if (!first_seg) {
	first_seg = seg;
	cur_seg = seg;
	prev_seg = NULL;
	...
} else {
	...
	prev_seg = cur_seg;
	cur_seg = seg;
}
```

So `prev_seg` is `NULL` when there is only one segment (`first_seg == seg`). If the single segment has `data_len < crc_len`, the `else if (prev_seg != NULL)` branch would not execute, and the FCS bytes would not be removed, resulting in a corrupt packet (too long by `crc_len` bytes).

**Recommendation:** When `last_seg->data_len < crc_len` and `prev_seg == NULL` (single-segment packet), the FCS is split between the end of the segment and... nowhere (HW should not produce this). If this can occur, the code should either truncate `last_seg->data_len` to zero and reduce `pkt_len` by `last_seg->data_len + remaining_crc` (dropping the segment), or WARN/drop the packet. Given the logic, this may be a defensive guard that never triggers in practice. Flag as a warning.

Actually, re-reading:

```c
if (likely(last_seg->data_len > crc_len)) {
	last_seg->data_len -= crc_len;
} else if (prev_seg != NULL) {
```

If `last_seg->data_len <= crc_len` and `prev_seg == NULL`, the function does nothing beyond the `first_seg->pkt_len -= crc_len` at the top, leaving the segment chain inconsistent (pkt_len reduced but segment data_len unchanged). This is a logic bug. Flag as **Error**.

Wait, let me re-check:

```c
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 `last_seg->data_len <= crc_len` and `prev_seg == NULL` (single-segment packet where the entire payload is less than 4 bytes), then:
- `pkt_len` is reduced by 4
- `data_len` is **not** reduced (should be reduced to `data_len - crc_len`, possibly going negative which is wrong)
- The packet is left in an inconsistent state.

**Verdict:** This is a correctness bug for the edge case of a very short single-segment packet (data_len <= 4 bytes) with KEEP_CRC=0. Flag as **Error**.

---

## Info

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

The patch does not update `doc/guides/nics/features/enetc4.ini` to mark "CRC offload = Y". 

Actually, it does:

```diff
+CRC offload          = Y
```

No issue.

---

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

The register dump feature is implemented but does not provide documentation explaining the register meanings or layout. This is acceptable as register dumps are typically for debugging and the HW reference manual is the authoritative source.

---

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

The interrupt setup allocates `nb_rx + 1` eventfds (MSI-X vector 0 for mailbox, vectors 1..nb_rx for queues) but only programs the intr_vec list with `nb_rx` entries (indices 0..nb_rx-1). The code appears correct but the comment "one bit per ring" in the SIRXIDR documentation could be clearer about the mailbox vector.

---

## Final Errors (after re-review)

### Patch 1: enetc_rx_crc_trim single-segment edge case

**File:** drivers/net/enetc/enetc_rxtx.c  
**Function:** enetc_rx_crc_trim  
**Issue:** When `last_seg->data_len <= crc_len` and `prev_seg == NULL` (single-segment packet with payload <= 4 bytes), the function reduces `first_seg->pkt_len` but does not adjust `last_seg->data_len`, leaving the mbuf in an inconsistent state.

**Fix:** Add an else branch to handle the single-segment case:

```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 {
	/* Single-segment packet: just reduce data_len directly */
	last_seg->data_len = (