|WARNING| [v6, 13/13] net/enetc4: enable Tx PAUSE via VF Rx congestion mode

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/167456

_AI Code Review_

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

# DPDK Patch Review: series.patch

## Summary

Reviewing 13 patches from the NXP ENETC4 PMD series for i.MX95. This review focuses on correctness bugs, C coding style compliance, API requirements, and guideline adherence per AGENTS.md.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch correctly implements KEEP_CRC offload. The FCS trim logic (`enetc_rx_crc_trim`) handles the scatter-gather boundary case where the FCS straddles two segments. The driver follows the DPDK convention of subtracting `crc_len` in the datapath. RBaMR[CRC] hardware control bit is properly documented and used.

---

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

### Errors

1. **Resource leak on error path** (line 115 in enetc4_ethdev.c):
   ```c
   if (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
                      RTE_ETH_TX_OFFLOAD_UDP_TSO)) {
       if (data->dev_conf.rxmode.offloads &
           RTE_ETH_RX_OFFLOAD_KEEP_CRC) {
           ENETC_PMD_ERR("LSO (TSO) is incompatible with KEEP_CRC");
           rte_free(tx_ring);  // GOOD: tx_ring freed
           return -EINVAL;
       }
       tx_ring->lso_enable = 1;
       dev->tx_pkt_burst = &enetc_xmit_pkts_lso;
   }
   
   err = enetc4_alloc_txbdr(tx_ring, nb_desc);
   if (err)
       goto fail;  // GOOD: goes to fail label
   ```
   The error path at the KEEP_CRC check correctly frees `tx_ring` before returning. The subsequent allocation failure goes to `fail:` which also frees `tx_ring`. No leak here.

   **Correction:** Not an error. The code is correct.

2. **Unused variable initialization** (enetc_rxtx.c line 238):
   ```c
   txbd_ext = (struct enetc_tx_bd_ext *)
              ENETC_TXBD(*tx_ring, i);
   memset(txbd_ext, 0, sizeof(*txbd_ext));
   ```
   The `memset` is intentional to zero the extension BD before populating the LSO fields. Not dead initialization.

   **Correction:** Not an error.

### Warnings

None.

### Info

The LSO implementation allocates a 2x ring when `lso_enable` is set to accommodate the extra extension BDs. The code correctly validates header length, data unit size, and segment size against hardware limits before attempting segmentation. Frames violating constraints are freed and skipped.

---

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

### Errors

None.

### Warnings

None.

### Info

RSC (Receive Segment Coalesce) is correctly implemented using 32B descriptors (RBaMR[BDS]=1). The driver properly:
- Doubles the ring allocation to account for 32B descriptors consuming two 16B slots.
- Enables interrupt coalescing (required precondition for RSC).
- Sets RBaRSCR[EN] only after BDS=1 is committed to hardware (as required by the reference manual).
- Reports coalesced frames with `RTE_MBUF_F_RX_LRO` when `RSC_FRAMES > 1`.
- Clears the SIRXIDR detect bit to re-arm the RSC flush timer on each poll.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch extends the link speed code from 4 bits to 8 bits in the PF-to-VF message by removing the unused cookie field. The formula-based approach for speeds > 5Gbps (`(link_speed - 5000) / 1000 + ENETC_SPEED_5000`) is well-documented. The `vf_link_legacy` devarg provides backward compatibility with older kernel PFs.

---

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

### Errors

None.

### Warnings

None.

### Info

The firmware version (`enetc4_vf_fw_version_get`) correctly reads the IP major revision from the PCI revision ID register and fetches the minor revision from the PSI via VSI-PSI messaging (class 0xF0, cmd 0x1). The version is formatted as `<major>.<minor>`. When the minor revision is unavailable, the function falls back to `<major>.unknown`. Buffer sizing is handled correctly per the `.fw_version_get` op contract.

---

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

### Errors

None.

### Warnings

None.

### Info

The `.get_reg` implementation correctly reports the register count and width when `regs->data == NULL`, then populates the buffer on the second call. The PF dumps station interface, port, and per-ring BD registers. The VF omits port registers (not accessible by a VF). The version field is populated with `device_id << 16 | revision_id`.

---

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

### Errors

None.

### Warnings

None.

### Info

The patch makes `enetc4_rxq_info_get` and `enetc4_txq_info_get` non-static and registers them in both VF ops tables. This enables ethtool ring parameter queries on the VF. No functional changes to the queue info retrieval logic.

---

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

### Errors

None.

### Warnings

1. **pthread_mutex_init without process-shared attribute** (enetc4_vf.c line 1760):
   ```c
   pthread_mutex_init(&hw->vsi_lock, NULL);
   ```
   The `vsi_lock` mutex is used to serialize VSI-PSI mailbox transactions. The mailbox registers are in the VF's own BAR (not shared with other processes), so this mutex is protecting per-device state within a single process. Process-shared attributes are not required here.

   **Correction:** Not an issue. The mutex is process-private and does not need `PTHREAD_PROCESS_SHARED`.

### Info

The patch introduces `enetc4_decode_link_speed()` to factor out the speed decoding logic (handling both current and legacy 4-bit layouts). On a link-up interrupt, the driver re-queries the speed from the PF before calling `rte_eth_linkstatus_set()`, ensuring the cached speed reflects the freshly negotiated value. The `vsi_lock` mutex is added to serialize all VSI-PSI mailbox transactions (preventing concurrent access from the interrupt thread and control thread).

---

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

### Errors

None.

### Warnings

None.

### Info

Since VF SI-level counters are read-only and cannot be zeroed directly (and soft-reset is unreliable due to a known HW erratum), the driver implements stats reset using a software snapshot/delta approach. On reset, the current HW counter values are captured as a baseline in `vf_stats_saved`. On stats_get, the driver reports `current_hw_value - saved_baseline`. Per-ring software error counters (`ierrors`) are zeroed directly on reset.

---

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

### Errors

None.

### Warnings

1. **Non-cacheable mode does not support Rx interrupts** (enetc4_vf.c):
   ```c
   if (hw->nc_mode)
       return -ENOTSUP;
   ```
   The code correctly returns `-ENOTSUP` when `nc=1` (non-cacheable mode) is set, as documented. This is not an error; it is the intended behavior.

   **Correction:** Not an issue.

### Info

MSI-X per-queue Rx interrupts are implemented for the cacheable Rx path only. Vector 0 is reserved for the PSI-to-VSI mailbox. Rx queue `i` is mapped to vector `i + 1`. The driver allocates eventfds before calling `rte_intr_enable()` so vfio-pci can wire each MSI-X vector to its eventfd. Per-ring coalescing is enabled with `ICPT=1` (first packet fires interrupt immediately). The SIRXIDR W1C detect bit is cleared before re-arming RBIER to prevent spurious interrupts after traffic stops.

---

## Patch 11/13: net/enetc4: add SI-based port VLAN insertion and removal

### Errors

None.

### Warnings

None.

### Info

SI-based port VLAN (pvid) is implemented via the per-SI VLAN isolation mechanism. On a PF, the driver writes directly to PSIaVLANR(0) and PSIaCFGR0(0). On a privileged VF, the request is forwarded to the kernel PF via VSI-PSI mailbox (class 0x24, cmd 0x1). The PF handler programs the same registers on behalf of the VF's station interface. The class 0x24 reply is added to the pass-through list in `enetc4_msg_vsi_send()`.

---

## Patch 12/13: net/enetc4: update VF link status to bitmask encoding

### Errors

None.

### Warnings

None.

### Info

The link status code is changed from a two-value enum (`UP=0x0`, `DOWN=0x1`) to a bitmask. `ENETC_LINK_DOWN` is now `BIT(0)`. Link up is encoded as the DOWN bit being clear. This keeps the wire value for link-down identical (0x1) and ensures backward compatibility with older kernel PFs. The `switch/case` statements in `enetc4_process_psi_msg()` and `enetc4_vf_link_update()` are replaced with bitmask decode (`if (status & ENETC_LINK_DOWN)`).

---

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

### Errors

None.

### Warnings

None.

### Info

When the PF negotiates TX PAUSE on the port, it signals this to the VF via BIT(1) of the PF-to-VF link status mailbox message. The VF PMD responds by setting `RBMR_CM` (BIT(4)) on all active RX rings so the MAC emits PAUSE frames on ingress pressure. The `tx_pause_active` flag is an `RTE_ATOMIC(uint8_t)` written on the interrupt thread and read on the control thread. The driver uses `rte_atomic_load_explicit(..., rte_memory_order_acquire)` and `rte_atomic_store_explicit(..., rte_memory_order_release)` for synchronization. The saved state is applied in `rx_queue_setup()` and `rx_queue_start()` so rings started later inherit the correct congestion mode setting.

---

## Overall Assessment

**Correctness:** No correctness bugs identified. All error paths correctly release allocated resources. Atomic operations use appropriate memory ordering. Interrupt handling and link status updates are correctly synchronized.

**Style:** Code follows DPDK conventions. No style violations found.

**API & Documentation:** All new features are documented in `doc/guides/nics/enetc4.rst` and `doc/guides/rel_notes/release_26_11.rst`. Feature matrix (`doc/guides/nics/features/enetc4.ini`) is updated. New ops are registered in the correct tables.

**Process:** No release notes or API changes require additional process steps beyond what is already done.

---

## Recommendation

**All patches are approved.** The series is well-structured, correctly implements the hardware features, and follows DPDK guidelines. No changes required.