|WARNING| [v5, 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/167430

_AI Code Review_

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

# DPDK ENETC4 PMD Patch Series Review

Reviewing 14 patches for ENETC4 NXP driver enhancements against DPDK coding standards and contribution guidelines.

---

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

### Errors

**enetc_rxtx.c:542** - Missing NULL check on prev_seg parameter
```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) {  // Check is here...
		first_seg->nb_segs--;
		prev_seg->data_len -= crc_len - last_seg->data_len;  // ...but unsigned subtraction could underflow
```
The expression `crc_len - last_seg->data_len` can underflow if `last_seg->data_len > crc_len` was false but the values are close. When `last_seg->data_len == crc_len`, the subtraction yields zero which is correct, but when `last_seg->data_len == crc_len - 1`, it wraps to a huge unsigned value. The condition `last_seg->data_len > crc_len` in the first branch means the else-if is reached when `last_seg->data_len <= crc_len`, which includes the underflow case.

**Suggested fix:** Change the second condition to ensure the subtraction cannot underflow:
```c
} else if (prev_seg != NULL && last_seg->data_len < crc_len) {
	uint16_t carry = crc_len - last_seg->data_len;
	first_seg->nb_segs--;
	prev_seg->data_len -= carry;
	prev_seg->next = NULL;
	rte_pktmbuf_free_seg(last_seg);
}
```

**enetc_rxtx.c:614** - prev_seg may be used uninitialized in cacheable path
```c
struct rte_mbuf *first_seg = NULL, *cur_seg = NULL, *prev_seg = NULL;
// ...
if (bd_status & ENETC_RXBD_LSTATUS_F) {
	seg->next = NULL;
	if (rx_ring->crc_len)
		enetc_rx_crc_trim(first_seg, prev_seg, seg,  // prev_seg passed here
				  rx_ring->crc_len);
```
In the single-segment frame case, `first_seg` is set but `prev_seg` remains `NULL` because the `else` branch that sets `prev_seg = cur_seg` is never entered. Then `enetc_rx_crc_trim()` is called with `prev_seg == NULL`. While the function checks `prev_seg != NULL`, the logic assumes multi-segment frames. For single-segment frames where the CRC needs trimming but the entire payload fits in one mbuf, the trim should just reduce `last_seg->data_len` without referencing `prev_seg`.

**Suggested fix:** The function logic is actually safe (single-segment frames take the first `if` branch and never reference `prev_seg`), but the initialization to `NULL` and the check make it correct. No change needed; my analysis was incorrect on second review. The code is safe.

---

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

### Warnings

**enetc4_ethdev.c:318** - Redundant multiplication by 2 could overflow on 32-bit
```c
ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
```
The cast to `uint32_t` happens before the multiply, so `nb_desc * 2` is computed in `uint32_t` width. If `nb_desc` is close to `UINT32_MAX / 2`, this could overflow. However, the subsequent check `if (ring_desc > MAX_BD_COUNT)` catches this, and `MAX_BD_COUNT` is 32768, well below the overflow threshold. The code is safe but could be clearer by casting after the multiply or by using explicit width:
```c
ring_desc = txr->lso_enable ? ((uint32_t)nb_desc << 1) : (uint32_t)nb_desc;
```
This is a minor clarity issue, not a bug.

**enetc_rxtx.c:280** - LSO data_unit could be zero leading to silent drop
```c
data_unit = rte_pktmbuf_pkt_len(seg) - hdr_len;
// ...
if (unlikely(seg->tso_segsz == 0 ||
             data_unit > ENETC4_LSO_MAX_DATA_UNIT ||
```
The check catches `tso_segsz == 0` but not `data_unit == 0`. A frame where `pkt_len == hdr_len` (no payload) would pass this check and proceed to build an LSO descriptor with zero payload, which the hardware may reject or mishandle. Consider adding `data_unit == 0` to the validation:
```c
if (unlikely(seg->tso_segsz == 0 || data_unit == 0 ||
             data_unit > ENETC4_LSO_MAX_DATA_UNIT ||
```

---

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

### Errors

**enetc4_ethdev.c:651** - Incorrect assignment operator in conditional expression
```c
if (rsc_size = 0 || rsc_size > ENETC4_RSC_MAX_FRAME)
```
This line uses assignment `=` instead of comparison `==`. The condition `rsc_size = 0` always evaluates to false (zero), so the `||` right-hand side is always evaluated, and `rsc_size` is unconditionally set to zero before the comparison. This is a logic error: the intent is to test whether `rsc_size` is zero, not to assign zero to it.

**Suggested fix:**
```c
if (rsc_size == 0 || rsc_size > ENETC4_RSC_MAX_FRAME)
```

---

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

### Info

**enetc4_vf.c:312** - Duplicated extraction logic in two functions
The `status` extraction logic (legacy vs. current layout) appears in both `enetc4_msg_vsi_reply_msg()` and `enetc4_msg_get_psi_msg()` with identical code:
```c
if (hw->vf_link_legacy)
	status |= ((vsimsgsr >> 4) & 0xf);
else
	status |= (vsimsgsr & 0xff);
```
Consider factoring this into a static inline helper to reduce duplication and ensure consistency.

---

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

### Warnings

**enetc4_vf.c:882** - Potential resource leak on rte_pci_read_config failure
```c
ret = rte_pci_read_config(pci_dev, &ip_mj, sizeof(ip_mj),
		RTE_PCI_REVISION_ID);
if (ret != sizeof(ip_mj)) {
	ENETC_PMD_ERR("Failed to read PCI revision ID");
	return -EIO;  // msg and msg->vaddr already allocated in enetc4_vf_get_ip_minor_revision
}
```
If `rte_pci_read_config()` fails, the function returns `-EIO` directly. However, if `enetc4_vf_get_ip_minor_revision()` was called earlier and succeeded, its `msg` and `msg->vaddr` allocations are freed in its own `end:` label. But if `enetc4_vf_get_ip_minor_revision()` failed with `-ENOTSUP` (which is allowed), the calling function `enetc4_vf_fw_version_get()` has no local allocations. On closer inspection, `enetc4_vf_get_ip_minor_revision()` cleans up its own allocations on all paths, so there is no leak here. The code is correct.

---

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

No issues found. The `.get_reg` implementation correctly reports required buffer size when `data == NULL` and bounds-checks the provided buffer.

---

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

No issues found. The patch simply registers existing functions in VF ops tables.

---

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

### Errors

**enetc4_vf.c:481** - Resource leak on rte_zmalloc failure in interrupt path
```c
rte_free(msg);
msg = rte_zmalloc(NULL, sizeof(*msg), RTE_CACHE_LINE_SIZE);
if (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);
} else {
	ENETC_PMD_WARN("Failed to alloc msg for speed query");
}
```
The original `msg` is freed, then a new `msg` is allocated. If the allocation fails, `msg` is now `NULL`, and the function continues without freeing it (because it's `NULL`). However, the function ends with `rte_free(msg);` which is safe on `NULL`, so there is no leak. The code is correct, though the warning message could clarify that the speed will not be updated.

---

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

No issues found. The software delta approach is sound given the hardware limitation.

---

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

### Errors

**enetc4_vf.c:1617** - Missing rte_intr_vec_list_free on failure path
```c
ret = rte_intr_vec_list_alloc(intr_handle,
		"enetc4_vf_rx_intr", nb_rx);
if (ret) {
	ENETC_PMD_WARN("Failed to alloc intr vec list: %d",
		       ret);
	rte_intr_efd_disable(intr_handle);  // efd disabled
	hw->rxq_intr_en = 0;
} else {
	for (i = 0; i < nb_rx; i++)
		rte_intr_vec_list_index_set(intr_handle, i,
			i + ENETC4_VF_RX_VEC_BASE);
	hw->rxq_intr_en = 1;
}
```
If `rte_intr_vec_list_alloc()` fails, the function disables efd and sets `rxq_intr_en = 0`, then continues to `rte_intr_enable()`. This is correct: if vec_list allocation fails, Rx interrupts are disabled and the device continues without them. The error path is handled correctly.

**enetc4_vf.c:1651** - Missing rte_intr_vec_list_free in disable path
```c
disable:
	enetc_vf_enable_mr_int(enetc_hw, false);
	hw->rxq_intr_en = 0;
	ret = rte_intr_disable(intr_handle);
```
When interrupts are disabled (`enable == false`), the function should free the interrupt vector list if it was allocated. The current code clears `rxq_intr_en` but does not call `rte_intr_vec_list_free()` or `rte_intr_efd_disable()`, potentially leaking the vector list allocation.

**Suggested fix:**
```c
disable:
	enetc_vf_enable_mr_int(enetc_hw, false);
	if (hw->rxq_intr_en) {
		rte_intr_vec_list_free(intr_handle);
		rte_intr_efd_disable(intr_handle);
	}
	hw->rxq_intr_en = 0;
	ret = rte_intr_disable(intr_handle);
```

---

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

No issues found. The VF-to-PF mailbox command handling is consistent with existing patterns.

---

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

No issues found. The change from enum to bitmask is backward-compatible (wire value for link-down remains `0x1`).

---

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

No issues found. The congestion mode state is correctly persisted and applied on queue start.

---

## PATCH 14/14: net/enetc4: add WRR Tx scheduler devarg for VF rings

### Errors

**enetc4_ethdev.c:1074** - Double-processing of devargs in configure
```c
hw->nc_mode = 0;
enetc4_get_devargs(dev, ENETC4_TXQ_PRIORITIES);
enetc4_get_devargs(dev, ENETC4_TXQ_WRR);
enetc4_get_devargs(dev, ENETC4_NC_MEMORY);
```
These devargs are already parsed in `enetc4_vf_dev_init()` or during device probe. Re-parsing them in `dev_configure()` will allocate new buffers for `txq_prior` and `txq_wrr` without freeing the previous ones if configure is called multiple times. The `parse_txq_prior()` and `parse_txq_wrr()` functions do call `rte_free()` on the old pointer before allocating, so this is safe, but calling `enetc4_get_devargs()` on every configure is wasteful. Consider parsing devargs only once during init.

**enetc4_ethdev.c:75** - Error path in parse_txq_prior returns inconsistent value
```c
if (!input_str)
	return -1;

rte_free(hw->txq_prior);
hw->txq_prior = rte_zmalloc(NULL, hw->max_tx_queues * sizeof(uint32_t), 0);
if (!hw->txq_prior) {
	free(input_str);
	return -ENOMEM;
}
```
The first error return is `-1`, the second is `-ENOMEM`. The kvargs parse callback should return a consistent error code. Use `-ENOMEM` or another negative errno for both, or `-1` for both. Prefer `-ENOMEM` for the allocation failure:
```c
if (!input_str)
	return -ENOMEM;
```

---

## Summary

### Errors Found (Must Fix)
1. **Patch 03** - Assignment `=` instead of comparison `==` in RSC size check (line 651)
2. **Patch 10** - Missing `rte_intr_vec_list_free()` and `rte_intr_efd_disable()` on interrupt disable path
3. **Patch 14** - Inconsistent error return codes in `parse_txq_prior()` (`-1` vs `-ENOMEM`)

### Warnings (Should Fix)
1. **Patch 02** - LSO validation does not reject zero-payload frames (