|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

I'll review the DPDK patch series against the AGENTS.md guidelines, focusing on correctness bugs and coding standards.

## PATCH 01/14: KEEP_CRC Rx offload

**Errors:**

1. **Resource leak on error path** (Correctness bug)
   - In `enetc4_rx_queue_setup()`, if `enetc4_alloc_rxbdr()` fails, `rx_ring` is freed via `goto fail`, but if setup succeeds, the error path at the end (`fail:`) will free `rx_ring` even though it has been successfully added to `data->rx_queues[]`, leading to a use-after-free when the queue is later accessed.
   - Fix: The `fail:` label should only execute if the ring was not added to `rx_queues[]`. Add `data->rx_queues[rx_queue_id] = NULL;` before the `goto fail` or restructure error handling.

**Warnings:**

None.

**Info:**

1. The CRC trimming logic in `enetc_rx_crc_trim()` is correct for handling the scatter-gather boundary case, but the variable initialization `struct rte_mbuf *first_seg = NULL, *cur_seg = NULL, *prev_seg = NULL;` in the non-cacheable and cacheable Rx functions is redundant. The existing code already initialized these variables inline; the new initializers are unnecessary.

---

## PATCH 02/14: TSO support

**Errors:**

1. **Integer overflow in ring sizing** (Correctness bug)
   - In `enetc4_alloc_txbdr()`:
     ```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 if `nb_desc` is a `uint16_t` near the max, `nb_desc * 2` overflows in 16-bit before widening. The code should cast before multiply:
     ```c
     ring_desc = txr->lso_enable ? ((uint32_t)nb_desc * 2) : (uint32_t)nb_desc;
     ```

2. **Potential descriptor leak on LSO frames** (Correctness bug)
   - In `enetc_xmit_pkts_lso()`, when an LSO frame is skipped due to invalid headers or HW limits, the frame is freed with `rte_pktmbuf_free(seg)` but the descriptor indices (`i`) have already advanced (header BD + extension BD consumed). The ring state (`next_to_use`) is updated at the end, so those slots remain "allocated" but unused, potentially causing ring full conditions or stale descriptor reads on wrap-around.
   - Fix: Either roll back `i` and `bds_to_use` when skipping, or mark the skipped slots as unused by writing a zero-length BD.

**Warnings:**

1. The LSO burst does not validate that `seg->tso_segsz` is non-zero before checking HW limits, leading to a potential division by zero or malformed descriptor if an application mistakenly sets `tso_segsz = 0` and the skip logic fails to catch it early.

---

## PATCH 03/14: RSC (hardware LRO)

**Errors:**

1. **Ring size overflow on RSC allocation** (Correctness bug)
   - In `enetc4_alloc_rxbdr()`:
     ```c
     ring_desc = rxr->rsc_enable ? (uint32_t)nb_desc * 2 : (uint32_t)nb_desc;
     ```
   - Same issue as patch 02: the multiply can overflow before the cast. Cast before multiply.

2. **Incorrect RCIR write on refill** (Correctness bug)
   - In `enetc_refill_rx_ring_rsc()`:
     ```c
     enetc_wr_reg(rx_ring->rcir, i / 2);
     ```
   - The comment says "Internal indices track 16B slots, but the HW consumer-index register counts 32B descriptors, so program `i / 2`." However, `i` at this point is guaranteed to be even (it only advances by 2), so `i / 2` is correct. But there's a subtle issue: if `i` wraps to 0 during the fill, the write is `enetc_wr_reg(..., 0)`, which is correct. However, the logic that flushes the partial group when `j` is non-zero but `i` is not on a 4-slot boundary may flush invalid cache lines if `i` wrapped. The code should verify the flush range after wrap.

**Warnings:**

None.

---

## PATCH 04/14: PF-VF link speed field extension

**Errors:**

None. The bitmask extension is backward-compatible: the legacy 4-bit extraction `(msg >> 4) & 0xf` is correctly guarded by `vf_link_legacy`, and the new 8-bit layout `msg & 0xff` is used otherwise. The speed decoding logic in `enetc4_decode_link_speed()` correctly handles both layouts.

**Warnings:**

1. In `parse_vf_link_legacy()`, the error handling prints a warning but then returns `-EINVAL`, which is correct. However, the function silently ignores invalid devarg values by returning the error without setting `hw->vf_link_legacy`, meaning the driver continues with the default (0). This is acceptable but could confuse users who mistype the devarg.

---

## PATCH 05/14: Firmware version get

**Errors:**

1. **Buffer overflow in `snprintf` return check** (Correctness bug)
   - In `enetc4_vf_fw_version_get()`:
     ```c
     ret = snprintf(fw_version, fw_size, "%u.%u", ip_mj, ip_mn);
     if (ret < 0)
         return -EINVAL;
     ret += 1; /* add trailing '\0' */
     if ((size_t)ret > fw_size)
         return ret;
     ```
   - The check `if ((size_t)ret > fw_size)` is too late: `snprintf` has already truncated the string if `ret >= fw_size`. The caller needs to know the required size before calling, so the function should return the required size (including `\0`) when truncation occurs. The logic is correct, but the comment is misleading: it says "add trailing `\0`" but `snprintf` already includes `\0` in its count.

**Warnings:**

None.

---

## PATCH 06/14: Registers dump

**Errors:**

None. The register dump logic correctly iterates over the SI, port, and per-ring arrays, and the buffer size check prevents overflow.

**Warnings:**

None.

---

## PATCH 07/14: Ethtool ring parameters

**Errors:**

None. The patch makes the queue info ops non-static and registers them in the VF ops tables. No correctness issues.

**Warnings:**

None.

---

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

**Errors:**

1. **Memory leak on speed query failure** (Correctness bug)
   - In `enetc4_process_psi_msg()`, after the link-up case:
     ```c
     rte_free(msg);
     msg = rte_zmalloc(...);
     if (msg) {
         if (!enetc4_vf_get_link_speed(eth_dev, msg) &&
             msg->class_id == ENETC_CLASS_ID_LINK_SPEED)
             enetc4_decode_link_speed(...);
     } else {
         ENETC_PMD_WARN("Failed to alloc msg for speed query");
     }
     ```
   - The `msg` allocated inside the `if (msg)` block is never freed. It should be freed before `rte_eth_linkstatus_set()` or the function returns.

**Warnings:**

None.

---

## PATCH 09/14: Stats reset for VF

**Errors:**

None. The stats reset correctly snapshots the HW counters and resets the per-ring error accumulators. The delta approach is sound.

**Warnings:**

None.

---

## PATCH 10/14: Per-queue Rx interrupt support

**Errors:**

1. **Missing interrupt vector deallocation on error** (Correctness bug)
   - In `enetc4_vf_dev_intr()`, when enabling interrupts:
     ```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);
         hw->rxq_intr_en = 0;
     }
     ```
   - If `rte_intr_enable()` fails after `rte_intr_vec_list_alloc()`, the interrupt vector list is never freed. The `goto disable` path should free the vector list if it was allocated.

**Warnings:**

None.

---

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

**Errors:**

1. **Missing error check on `rte_mem_virt2iova()`** (Correctness bug)
   - In `enetc4_vf_vlan_pvid_set()`:
     ```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;
     }
     ```
   - This is correct. No issue.

**Warnings:**

None.

---

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

**Errors:**

None. The bitmask encoding is backward-compatible: `ENETC_LINK_DOWN = BIT(0)` is `0x1`, matching the old enum value. Link-up is encoded as `!(status & ENETC_LINK_DOWN)`.

**Warnings:**

None.

---

## PATCH 13/14: Enable Tx PAUSE via VF Rx congestion mode

**Errors:**

None. The congestion mode logic correctly applies `RBMR_CM` to all active rings when TX PAUSE is signaled, and clears it on link-down or when TX PAUSE is inactive.

**Warnings:**

None.

---

## PATCH 14/14: WRR Tx scheduler devarg

**Errors:**

1. **Memory leak in devarg parsing** (Correctness bug)
   - In `parse_txq_prior()` and `parse_txq_wrr()`:
     ```c
     rte_free(hw->txq_prior);
     hw->txq_prior = rte_zmalloc(...);
     ```
   - If the second `rte_zmalloc()` fails, the function returns `-ENOMEM` but `hw->txq_prior` is left as `NULL`. On the next call to `parse_txq_prior()`, the `rte_free(hw->txq_prior)` is safe (rte_free(NULL) is a no-op), but if the driver continues and tries to use `hw->txq_prior`, it will dereference NULL. The code should either restore the old pointer on allocation failure or ensure `hw->txq_prior` is only used when non-NULL.

2. **Use-after-free in `enetc4_dev_configure()`** (Correctness bug)
   - The code frees `hw->txq_prior` and `hw->txq_wrr` in `enetc4_dev_close()`, but `enetc4_dev_configure()` is called before `dev_start()` and can be called multiple times. The line:
     ```c
     enetc4_get_devargs(dev, ENETC4_TXQ_PRIORITIES);
     ```
   - calls `parse_txq_prior()`, which does `rte_free(hw->txq_prior)` then reallocates. If `dev_configure` is called twice without a `dev_close` in between, the second call will free the array allocated by the first call, then allocate a new one. This is correct. However, the line:
     ```c
     hw->nc_mode = 0;
     ```
   - is placed after the loop that resets the Tx rings, and before the devarg parsing. If `nc_mode` was set to 1 by a previous configure, this unconditionally clears it, which is wrong. The `nc_mode` should only be cleared if the devarg is not set.

**Warnings:**

None.

---

## Summary

**Critical Correctness Bugs (Errors):**

1. **Patch 01**: Resource leak / use-after-free in `enetc4_rx_queue_setup()` error path.
2. **Patch 02**: Integer overflow in LSO ring sizing (cast after multiply). Descriptor leak when LSO frames are skipped.
3. **Patch 03**: Integer overflow in RSC ring sizing (cast after multiply).
4. **Patch 08**: Memory leak in `enetc4_process_psi_msg()` speed query path.
5. **Patch 10**: Missing interrupt vector list deallocation on `rte_intr_enable()` failure.
6. **Patch 14**: Use-after-free potential if devarg parsing is called multiple times; `nc_mode` unconditionally cleared.

**Warnings:**

1. **Patch 02**: LSO burst does not validate `tso_segsz != 0` before checking HW limits.

**Info:**

1. **Patch 01**: Redundant variable initialization in Rx functions.