|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 Code Review - net/enetc: series.patch

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

### Errors

None identified.

### Warnings

1. **Scattered Rx CRC trim logic should handle single-segment case**
   (enetc_rxtx.c, enetc_rx_crc_trim())
   
   When `last_seg->data_len < crc_len` and `prev_seg == NULL` (the packet is a single segment shorter than the CRC length), the code does nothing. This leaves `first_seg->pkt_len` unchanged (already decremented by `crc_len`), causing an underflow in the length field. Either reject such malformed packets or add a branch to handle them explicitly.

   ```c
   /* Current code silently does nothing when prev_seg is NULL
    * and last_seg->data_len < crc_len, leaving pkt_len underflowed. */
   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);
   }
   /* Missing: else { handle single-seg underflow or mark as error } */
   ```

2. **Initialization of first_seg and prev_seg to NULL may be redundant**
   (enetc_rxtx.c, enetc_clean_rx_ring_nc/cacheable)
   
   In both clean functions, `first_seg` and `cur_seg` are declared and later set to `NULL` explicitly, but the first use of these variables is always an assignment (`first_seg = seg`). The explicit initialization to `NULL` is defensive but not strictly necessary given the control flow. If you want to keep it for safety, it's acceptable, but it can be noted as extra caution.

### Info

1. **Release notes wording**
   The release note states "Follow the DPDK convention used by the legacy ENETC and ixgbe drivers". This is clear but could mention that the datapath subtraction of `crc_len` is now also applied to scattered Rx paths, not just single-segment.

---

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

### Errors

None identified.

### Warnings

1. **LSO frame validation checks should be grouped together**
   (enetc_rxtx.c, enetc_xmit_pkts_lso())
   
   The checks for invalid LSO frames (zero `tso_segsz`, oversized data unit, oversized frame) are scattered across multiple `if (unlikely(...))` blocks with individual `rte_pktmbuf_free()` and `start++; continue;` sequences. Grouping them into a single validation block would improve readability and reduce code duplication.

   Suggested refactor:

   ```c
   /* Validate LSO constraints before building descriptors */
   bool skip_frame = false;
   if (unlikely(hdr_len >= rte_pktmbuf_pkt_len(seg) ||
                hdr_len > rte_pktmbuf_data_len(seg) ||
                seg->tso_segsz == 0 ||
                data_unit > ENETC4_LSO_MAX_DATA_UNIT ||
                hdr_len + seg->tso_segsz > ENETC4_LSO_MAX_FRAME)) {
       skip_frame = true;
   }
   if (skip_frame) {
       rte_pktmbuf_free(seg);
       start++;
       continue;
   }
   ```

2. **Extension BD alignment comment could be clearer**
   (enetc_rxtx.c, enetc_xmit_pkts_lso())
   
   The comment "Extension BD occupies the next ring slot" does not mention that the ring is sized with `2 * nb_desc` specifically to accommodate this. Adding a note like "/* Ring was sized 2x to hold extension BDs */" would clarify the design.

3. **LSO compatibility check is late in configure flow**
   (enetc4_ethdev.c, enetc4_tx_queue_setup())
   
   The check for `RTE_ETH_RX_OFFLOAD_KEEP_CRC` when LSO is enabled happens after `enetc4_alloc_txbdr()` has already allocated ring memory. If the check fails, the ring is freed and the function returns `-EINVAL`, but it would be cleaner to validate offload compatibility before allocating any resources. Recommend moving the LSO compatibility checks to the top of the function.

### Info

1. **Documentation note**
   The RST doc states "LSO needs HW FCS insertion, so it is incompatible with KEEP_CRC on Rx". This is clear, but it could mention that the incompatibility is enforced at configure time (the driver returns `-EINVAL` if both are requested).

---

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

### Errors

None identified.

### Warnings

1. **RSC refill uses hardcoded ENETC_RXBD_BUNDLE stride**
   (enetc_rxtx.c, enetc_refill_rx_ring_rsc())
   
   The loop allocates one mbuf per 2-slot descriptor (`j += 2`) and bulk-allocates `m_cnt = RTE_MIN(want, ENETC_RXBD_BUNDLE)` mbufs at a time. The `ENETC_RXBD_BUNDLE` constant is defined as 8 in the non-RSC refill code. For RSC, `want` is `(buff_cnt - j) / 2`, so the bulk size is effectively `min((buff_cnt - j) / 2, 8)`. This is correct but subtle. A comment explaining why `ENETC_RXBD_BUNDLE` works for the 2-slot stride would improve clarity.

2. **RSC ring size doubling could overflow if nb_desc is MAX_BD_COUNT**
   (enetc4_ethdev.c, enetc4_alloc_rxbdr())
   
   When RSC is enabled, `ring_desc = (uint32_t)nb_desc * 2`. If `nb_desc` is `MAX_BD_COUNT / 2` or larger, `ring_desc` could exceed `MAX_BD_COUNT` and the validation message is printed. However, the error message says "reduce nb_desc to <= %u with LSO enabled" but this is for RSC (LRO), not LSO. The message should say "reduce nb_desc to <= %u with RSC (LRO) enabled".

   Also, the same doubling/validation logic appears in patch 02 for LSO Tx rings. Consider extracting a helper function to avoid duplication.

3. **ICPT default of 1 is minimal; consider documenting tuning guidance**
   (enetc4_ethdev.c, ENETC4_RSC_DEF_ICPT)
   
   The default packet threshold is 1 (flush on first packet). This means the HW timer is the only mechanism gating coalesce window. The comment states this is to satisfy RSC's ICEN precondition, but it would be helpful to document that users can tune ICPT via a future devarg if they want different behavior. (This is an informational note, not a code defect.)

### Info

1. **RSC clean invalidates cache lines with dccivac**
   (enetc_rxtx.c, enetc_clean_rx_ring_rsc())
   
   The comment explains why `dccivac` (clean+invalidate) is used instead of `DC IVAC` from EL0. This matches the pattern in the cacheable non-RSC path. Good documentation.

---

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

### Errors

None identified.

### Warnings

1. **Devarg parsing could validate vf_link_legacy value**
   (enetc4_vf.c, parse_vf_link_legacy())
   
   The function checks `val > 1` and returns `-EINVAL`, but the check happens after `strtoul()`. The docstring for `strtoul()` notes that `EINVAL` is returned if the base is invalid, not the value. The range check is correct, but the function returns `-EINVAL` without setting `errno`. This is acceptable since the devarg parser doesn't rely on `errno`, but it's worth noting that the error code could be more specific (e.g., `-ERANGE`).

2. **Speed decoding duplicates logic between enetc4_decode_link_speed() and the interrupt handler**
   (enetc4_vf.c)
   
   The new `enetc4_decode_link_speed()` helper is called from `enetc4_vf_link_update()` to decode the speed from the VSI reply. It's also called from `enetc4_process_psi_msg()` (in patch 08) after re-querying the speed. This is good refactoring, but the commit message for patch 04 doesn't mention that the helper will also be used in the interrupt path. This is minor and doesn't affect correctness.

### Info

1. **Backward compatibility devarg is documented**
   The RST doc clearly explains when to use `vf_link_legacy=1` (kernel PF before v6.18.37). Good user-facing documentation.

---

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

### Errors

None identified.

### Warnings

1. **PCI config read error could be more specific**
   (enetc4_vf.c, enetc4_vf_fw_version_get())
   
   When `rte_pci_read_config()` fails, the function returns `-EIO`, but the error message says "Failed to read PCI revision ID". The PCI config space read could fail for reasons other than I/O (e.g., insufficient permissions), so a more generic message like "Failed to read PCI config space" would be slightly more accurate. This is a minor documentation issue.

2. **Fallback to "unknown" for minor revision could be clearer**
   (enetc4_vf.c, enetc4_vf_fw_version_get())
   
   When `enetc4_vf_get_ip_minor_revision()` returns `-ENOTSUP`, the code formats the version as "%u.unknown". The function comment explains this is for PSIs that don't implement the IP_MN command, but it might be helpful to log a debug message when this fallback is taken so users know the minor version is unavailable.

### Info

1. **Version format matches kernel driver**
   The commit message states "This mirrors the behaviour of the kernel PF/VF drivers". Good consistency.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

1. **VF excludes port registers**
   The VF register dump correctly excludes port registers (which are not accessible to a VF). The function comment makes this clear.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

1. **Non-static functions are now registered in VF ops tables**
   The commit message explains that `enetc4_rxq_info_get()` and `enetc4_txq_info_get()` are made non-static so the VF can register them. This is a reasonable change.

---

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

### Errors

None identified.

### Warnings

1. **Speed query allocates a new reply_msg; existing one is freed**
   (enetc4_vf.c, enetc4_process_psi_msg())
   
   When the link-up interrupt arrives, the code frees the original `msg` (which held the link-status notification) and allocates a new `msg` for the speed query. If the allocation fails, the code prints a warning and continues with the stale speed. This is acceptable fallback behavior, but the warning message could mention that the speed may be incorrect. E.g., "Failed to alloc msg for speed query; speed may be stale".

2. **ENETC_LINK_UP removed from enum but no migration guide for out-of-tree code**
   (enetc.h, patch 12 removes ENETC_LINK_UP)
   
   Patch 04 introduced a helper (`enetc4_decode_link_speed()`) that uses the status code directly. Patch 08 calls this helper from the interrupt path. Patch 12 later removes the `ENETC_LINK_UP` enum value entirely, converting to a bitmask. Any out-of-tree code that used `ENETC_LINK_UP` will break. The release notes for patch 12 mention the bitmask change but don't call out that the enum is removed. Consider adding a note that `ENETC_LINK_UP` is removed and should be replaced with `!(status & ENETC_LINK_DOWN)`.

### Info

1. **Link speed refresh is a good fix**
   The commit message clearly explains the problem (stale speed after link-down/link-up) and the solution (re-query speed on LINK_UP interrupt). Good description.

---

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

### Errors

None identified.

### Warnings

None identified.

### Info

1. **Software delta approach is well-documented**
   The commit message and code comments clearly explain why software snapshots are needed (SI counters are read-only for VF). Good documentation.

---

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

### Errors

None identified.

### Warnings

1. **Rx interrupt setup happens before interrupt enable**
   (enetc4_vf.c, enetc4_vf_dev_intr())
   
   The code allocates efds and the interrupt vector list, then calls `rte_intr_enable()`. If `rte_intr_enable()` fails, the efds are not freed. The function returns early with an error, but the allocated resources are left in place. This is a resource leak on the failure path.

   Suggested fix:

   ```c
   ret = rte_intr_enable(intr_handle);
   if (ret) {
       ENETC_PMD_ERR("Failed to enable INTR %d", ret);
       /* Clean up on failure */
       rte_intr_vec_list_free(intr_handle);
       rte_intr_efd_disable(intr_handle);
       hw->rxq_intr_en = 0;
       return ret;
   }
   ```

2. **Rx interrupt disable does not free efds**
   (enetc4_vf.c, enetc4_vf_dev_intr() disable path)
   
   When interrupts are disabled (`enable == false`), the function clears `VSIIER_MRIE`, unregisters the callback, and disables the intr handle, but it does not call `rte_intr_vec_list_free()` or `rte_intr_efd_disable()`. This may leave the efds allocated even after interrupts are disabled. The disable path should mirror the enable path cleanup.

   Suggested addition to the disable path:

   ```c
   disable:
       enetc_vf_enable_mr_int(enetc_hw, false);
       hw->rxq_intr_en = 0;
       rte_intr_vec_list_free(intr_handle);  /* Add this */
       rte_intr_efd_disable(intr_handle);    /* Add this */
       ret = rte_intr_disable(intr_handle);
       if (ret)
           ENETC_PMD_WARN("Failed to disable INTR %d", ret);
   ```

### Info

1. **Documentation is thorough**
   The RST doc includes setup instructions, kernel requirements, and a worked example with `l3fwd