|WARNING| [v2, 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/167345

_AI Code Review_

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

# DPDK Patch Review: series.patch

## Summary

This 14-patch series adds multiple features to the ENETC4 network driver for NXP i.MX95. The review focuses on correctness bugs, coding style compliance, and API requirements per AGENTS.md guidelines.

---

## Patch 01/14: Add KEEP_CRC offload support

### Errors

**enetc4_ethdev.c:592-596: Uninitialized variable used on all code paths**

```c
uint32_t rx_enable;
// ...
if (keep_crc)
    rx_enable |= ENETC4_RBMR_CRC;
else
    rx_enable &= ~ENETC4_RBMR_CRC;
```

`rx_enable` is modified with `|=` and `&=` before being initialized. If `rx_enable` holds garbage, this produces undefined behavior. Initialize to zero:

```c
uint32_t rx_enable = 0;
```

**enetc_rxtx.c:542-547: Unsafe pointer dereference when prev_seg is NULL**

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

This else-if branch is guarded, but the function signature shows `prev_seg` can be NULL. If `last_seg->data_len <= crc_len` and `prev_seg == NULL`, the trim is silently incomplete--the code continues without subtracting the full `crc_len`. This violates the function contract (caller expects `first_seg->pkt_len` reduced by `crc_len`). Either return an error or handle single-segment undersized frames explicitly.

### Warnings

**enetc4_ethdev.c:583: Missing blank line before conditional block**

Insert a blank line before `if (keep_crc)` to separate variable declarations from logic (DPDK style prefers visual separation).

---

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

### Errors

**enetc4_ethdev.c:337: Integer overflow in ring size calculation**

```c
ring_desc = txr->lso_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
```

Both operands are 32-bit unsigned. If `nb_desc` is large (e.g., approaching `UINT16_MAX`), the multiply can overflow before the assignment. Cast to `size_t` or check bounds:

```c
ring_desc = txr->lso_enable ? (size_t)nb_desc * 2 : nb_desc;
if (ring_desc > MAX_RING_DESC)
    return -EINVAL;
```

**enetc_rxtx.c:256-259: Potential use-after-free on segment chain**

```c
first_payload = (uint16_t)(seg_len - hdr_len);
dseg = seg;
is_first_seg = true;
while (dseg) {
    // ...
    dseg = dseg->next;
}
```

If the segment chain is malformed (e.g., circular reference), this loop never terminates. Bound iteration by `nb_segs` or a maximum descriptor count to prevent infinite loops on corrupt mbuf metadata.

**enetc_rxtx.c:275-283: Missing error path cleanup**

When the loop populates payload BDs, if allocation or DMA mapping fails partway through (not shown in this patch but a concern in general payload BD setup), any already-written BDs are leaked. Ensure a cleanup path exists to reset ring state on mid-operation failure.

### Warnings

**enetc4_ethdev.c:427: Overly defensive KEEP_CRC check text**

The error message "LSO (TSO) is incompatible with KEEP_CRC" is clear, but the condition is a warning-level policy rather than a hard HW limit. Consider logging as WARN instead of ERR since applications may be experimenting with offload combinations.

---

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

### Errors

**enetc4_ethdev.c:671: Same uninitialized variable pattern as Patch 01**

```c
uint32_t rx_enable;
// ...
if (rsc_enable) {
    rx_enable |= ENETC4_RBMR_BDS;
```

Initialize to zero before modification.

**enetc_rxtx.c:953-961: Missing bounds check on RSC descriptor traversal**

The RSC receive path walks descriptors with:

```c
while (likely(rx_frm_cnt < work_limit)) {
    // read BD at i
    i += 2;
    if (unlikely(i == bd_count)) {
        i = 0;
```

If HW produces a malformed descriptor sequence where the last-frame flag is never set, `rx_frm_cnt` never increments but `i` wraps indefinitely. Limit total BDs processed per call independent of frame count:

```c
int bd_processed = 0;
while (likely(rx_frm_cnt < work_limit && bd_processed < bd_count)) {
    // ...
    bd_processed++;
```

**enetc4_ethdev.c:702-712: ICTT written before ICEN cleared (RM violation)**

The comment states "Program ICTT FIRST (with ICEN = 0) then enable ICEN, as the RM requires." But the code writes `ENETC4_RBICR1` first (ICTT), then `ENETC4_RBICR0` with `ICEN` set in a single write. If ICEN was already set from a previous ring setup and not explicitly cleared, the RM sequence is violated. Clear ICEN in a separate write before setting ICTT:

```c
enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC4_RBICR0, 0);
enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC4_RBICR1, ENETC4_RSC_DEF_ICTT);
enetc4_rxbdr_wr(&adapter->hw.hw, rx_ring->index, ENETC4_RBICR0,
                ENETC4_RBICR0_ICEN | ENETC4_RBICR0_ICPT(ENETC4_RSC_DEF_ICPT));
```

### Warnings

**enetc_rxtx.c:920: Invalidate-before-read pattern relies on cache line alignment**

The `dccivac` call at the start assumes `next_to_clean` starts on a cache-line-aligned BD. If ring allocation does not guarantee this, the first invalidate may miss the initial descriptor. Verify `ENETC_BD_RING_ALIGN` is at least cache-line size.

---

## Patch 04/14: Extend link speed code field to 8-bit

### Errors

**enetc.h:228-230: Enum value out of bounds for 8-bit field**

```c
enum speed {
    ENETC_SPEED_5000 = 0x7,
    ENETC_SPEED_MAX = 0xff,
};
```

`ENETC_SPEED_MAX` is defined but never used in a switch case. If a future PF sends a speed code > 0xff (due to a bug), the formula `(status - ENETC_SPEED_5000) * 1000 + 5000` produces garbage. Clamp input:

```c
if (status > ENETC_SPEED_MAX) {
    link->link_speed = RTE_ETH_SPEED_NUM_UNKNOWN;
    return;
}
```

**enetc4_vf.c:311-318: Missing error check on strtoul**

```c
val = strtoul(value, &endptr, 0);
if (errno != 0 || *endptr != '\0' || val > 1) {
```

Errno is only valid if `strtoul` returns `ULONG_MAX`. Reset `errno` to zero before the call to avoid stale errors:

```c
errno = 0;
val = strtoul(value, &endptr, 0);
if (errno == ERANGE || *endptr != '\0' || val > 1) {
```

---

## Patch 05/14: Support firmware version get for VF

### Errors

**enetc4_vf.c:529: Resource leak on early return path**

```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_ip_ver), ...);
msg->vaddr = rte_zmalloc(NULL, msg_size, 0);
if (!msg->vaddr) {
    ENETC_PMD_ERR("Failed to alloc memory for msg");
    rte_free(msg);  // GOOD
    return -ENOMEM;
}
// ...
if (class_id != ENETC_CLASS_ID_GET_IP_VER) {
    ENETC_PMD_ERR("Wrong reply message 0x%x", class_id);
    err = -EIO;
    goto end;
}
```

The first `return -ENOMEM` on `msg->vaddr` alloc failure correctly frees `msg`. But if `class_id` check fails, `goto end` is used. Verify `end:` label frees both `msg->vaddr` and `msg`. Patch shows:

```c
end:
    rte_free(msg->vaddr);
    rte_free(msg);
    return err;
```

This is correct. No error.

**enetc4_vf.c:554-555: Buffer size calculation off-by-one**

```c
if (fw_size == 0)
    return snprintf(NULL, 0, "%u.%u", ip_mj, ip_mn) + 1;
```

The `+1` accounts for the null terminator, which is correct. However, later:

```c
ret = snprintf(fw_version, fw_size, "%u.%u", ip_mj, ip_mn);
if (ret < 0)
    return -EINVAL;
ret += 1;
if ((size_t)ret > fw_size)
    return ret;
```

If `snprintf` returns exactly `fw_size - 1` (buffer full, string truncated), adding `+1` produces `fw_size`, and the check `ret > fw_size` is false--no error is returned despite truncation. This is a corner-case logic error. The correct check is `>=`:

```c
if ((size_t)ret >= fw_size)
    return ret + 1;  // indicate required size
```

---

## Patch 06/14: Support registers dump

### Info

No correctness bugs identified. Implementation is straightforward register reads.

---

## Patch 07/14: Support ethtool ring parameters

### Info

No correctness bugs. Patch exports existing queue info functions to VF ops tables.

---

## Patch 08/14: Refresh link speed on VF link-up interrupt

### Errors

**enetc4_vf.c:441-448: Double-free on msg after speed query**

```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) && ...)
        enetc4_decode_link_speed(...);
} else {
    ENETC_PMD_WARN("Failed to alloc msg for speed query");
}
```

After the speed query, `msg` is not freed before the function continues. Later, at the end of `enetc4_process_psi_msg`, there is `rte_free(msg)`. If the speed query path reallocates `msg`, the original `msg` pointer is lost, causing a memory leak. The reallocation should free the old `msg` first, or use a separate variable:

```c
struct enetc_psi_reply_msg *speed_msg = rte_zmalloc(...);
// use speed_msg
rte_free(speed_msg);
```

---

## Patch 09/14: Support stats reset for VF

### Info

No correctness bugs. Software delta approach is appropriate given HW constraints.

---

## Patch 10/14: Add per-queue Rx interrupt support for VF

### Errors

**enetc4_vf.c:1930-1935: Resource leak on partial MSI-X setup failure**

```c
ret = rte_intr_efd_enable(intr_handle, nb_rx + ENETC4_VF_RX_VEC_BASE);
if (ret) {
    ENETC_PMD_WARN("Failed to enable per-queue Rx eventfds: %d", ret);
    ret = 0;
    hw->rxq_intr_en = 0;
} else {
    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);  // GOOD
        hw->rxq_intr_en = 0;
```

If `rte_intr_vec_list_alloc` fails, `rte_intr_efd_disable` is called, which is correct. However, if the outer `rte_intr_enable` later fails, the code jumps to the `disable:` label. That label does not call `rte_intr_efd_disable` or free the vec list. Add cleanup there:

```c
disable:
    if (hw->rxq_intr_en) {
        rte_intr_vec_list_free(intr_handle);
        rte_intr_efd_disable(intr_handle);
    }
    hw->rxq_intr_en = 0;
```

---

## Patch 11/14: Add SI-based port VLAN insertion and removal

### Errors

**enetc4_vf.c:1556-1561: DMA address error not propagated**

```c
msg->dma = rte_mem_virt2iova((const void *)msg->vaddr);
if (msg->dma == RTE_BAD_IOVA) {
    ENETC_PMD_ERR("Failed to get IOVA for SI VLAN isolation msg body");
    rte_free(msg->vaddr);
    rte_free(msg);
    rte_free(reply_msg);
    return -ENOMEM;
```

Error return is `-ENOMEM`, which is correct. But the function continues to use `msg->dma` in `enetc4_msg_vsi_send`. Verify that send function checks for `RTE_BAD_IOVA` or that this error path is never bypassed. Code shows early return, so no bug.

---

## Patch 12/14: Update VF link status to bitmask encoding

### Info

No correctness bugs. Encoding change is backward-compatible (link-down value unchanged).

---

## Patch 13/14: Enable TX PAUSE via VF RX congestion mode

### Errors

**enetc4_vf.c:481: Missing range check on nb_rx_queues**

```c
for (i = 0; i < nb_rx; i++) {
    rbmr = enetc4_rxbdr_rd(enetc_hw, i, ENETC_RBMR);
```

If `nb_rx` exceeds the HW ring count, this writes to invalid register offsets. The device configure step should have validated `nb_rx_queues <= max_rx_queues`, but verify that bound is enforced before calling `enetc4_vf_set_congestion_