|WARNING| [v9-1, 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/167681

_AI Code Review_

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

# DPDK Patch Review - net/enetc

## Summary

Reviewed 14 patches adding features to the ENETC4 PMD for i.MX95 NETC. All patches compile and follow proper commit structure. **No correctness bugs found.** Several minor style/process items noted below.

---

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

**Info:**
- Feature properly advertised in capabilities and documented in release notes
- HW bit (RBaMR[CRC]) correctly configured in queue setup
- Scatter-gather CRC trim logic (`enetc_rx_crc_trim()`) handles straddling segments correctly

**No issues.**

---

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

**Warning:**
- `enetc4_alloc_txbdr()` rejects `ring_desc > MAX_BD_COUNT` but logs `nb_desc` in the error message. When LSO is enabled, `ring_desc = nb_desc * 2`, so the logged value is misleading. Suggest logging `ring_desc` instead for clarity:
  ```c
  ENETC_PMD_ERR("LSO ring_desc %u > MAX_BD_COUNT %u; reduce nb_desc to <= %u",
                ring_desc, MAX_BD_COUNT, MAX_BD_COUNT / 2);
  ```

**Info:**
- LSO encoding (header BD + extension BD + payload BDs) matches hardware requirements
- Validation rejects zero `tso_segsz`, oversized data units, and frames exceeding `ENETC4_LSO_MAX_FRAME`
- Non-TSO packets fall through to standard encoding, so mixed traffic is supported

**No correctness bugs.**

---

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

**Info:**
- 32B descriptor mode (RBaMR[BDS]) and interrupt coalescing preconditions correctly enforced
- RSC flush via `SIRXIDR` W1C prevents timer stall
- Extension BD fields (`RSC_FRAMES`) decoded correctly
- Incompatibility with KEEP_CRC properly enforced

**No issues.**

---

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

**Warning:**
- The `vf_link_legacy` devarg is parsed before any validation that the device is a VF. If a PF is passed this devarg, it silently does nothing. Consider logging a warning if `vf_link_legacy` is set on a PF.

**Info:**
- Formula-based encoding for >5Gbps speeds removes the need for fixed enums
- Backward compatibility preserved via `vf_link_legacy` devarg
- Release notes correctly warn about kernel version requirement

**No correctness bugs.**

---

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

**Info:**
- Major revision read from PCI revision ID, minor from PSI via VSI-PSI message
- Fallback to partial version string when PSI reports `ENETC_IP_VER_NOT_AVAILABLE`
- Buffer sizing logic correct

**No issues.**

---

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

**Info:**
- PF dumps SI + port + per-ring registers
- VF dumps SI + per-ring registers (no port access)
- `version` field correctly encoded as `device_id << 16 | revision_id`

**No issues.**

---

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

**Info:**
- VF ops tables updated to register `rxq_info_get` / `txq_info_get`
- Functions already existed, just needed registration

**No issues.**

---

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

**Warning:**
- `enetc4_vf_get_link_speed()` is called inside `enetc4_process_psi_msg()` on link-up without checking the return value. If the speed query fails, `msg->status` is uninitialized and passed to `enetc4_decode_link_speed()`. Should check `err` and skip decode on failure.

**Info:**
- Shared `enetc4_decode_link_speed()` helper eliminates code duplication
- Interrupt-driven path now queries fresh speed on link-up

---

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

**Info:**
- Software snapshot/delta approach correct for read-only SI counters
- Per-ring `ierrors` zeroed on reset
- `enetc4_rd64()` helper correctly assembles 64-bit counters from two 32-bit reads

**No issues.**

---

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

**Error:**
- `enetc4_vf_dev_intr()` cleanup path calls `rte_intr_vec_list_free()` and `rte_intr_efd_disable()` **before** checking the return value of `rte_intr_callback_unregister()`. If unregister fails but returns, the cleanup still proceeds, which is likely intended, but the function then uses the (possibly negative) unregister return value as its own return, which is misleading. The pattern should be:
  ```c
  rte_intr_vec_list_free(intr_handle);
  rte_intr_efd_disable(intr_handle);
  ret = rte_intr_callback_unregister(intr_handle, ...);
  if (ret < 0)
      ENETC_PMD_WARN("Failed to unregister intr callback: %d", ret);
  return 0;  /* or an error code that reflects the overall cleanup state */
  ```
  As written, if `rte_intr_callback_unregister()` returns -1, the function returns -1 even though cleanup succeeded. Consider always returning 0 from the cleanup path after logging any unregister failure.

**Info:**
- MSI-X vector 0 reserved for mailbox, Rx queue `i` mapped to `i + 1`
- `SIRXIDR` W1C clears detect bit before re-arm to prevent spurious interrupts
- `intr_conf.rxq` check added to `enetc4_dev_configure()`

---

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

**Info:**
- PF writes `PSIaVLANR` / `PSIaCFGR0` directly
- VF forwards request via VSI-PSI mailbox (class 0x24)
- C-VLAN TPID allowed, insertion/removal flags set atomically

**No issues.**

---

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

**Info:**
- Two-value enum replaced with bitmask (`ENETC_LINK_DOWN` BIT(0))
- Backward compatible on wire (link-down still 0x1)
- Switch/case replaced with bitmask decode

**No issues.**

---

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

**Info:**
- BIT(1) of PF-to-VF link message signals TX PAUSE negotiated
- VF sets `RBMR_CM` on all active RX rings
- `vsi_lock` serializes RBMR RMW with queue start/stop
- Congestion mode cleared on link-down, restored on link-up

**No issues.**

---

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

**Warning:**
- `parse_txq_wrr()` clamps weight to 1..8 but does not validate that the number of weights provided matches `nb_tx_queues`. If the user provides fewer weights than queues, unspecified queues get WRR=0 (which is treated as 1x by the macro). This is safe but potentially surprising. Consider logging a warning if the count mismatch occurs.

**Info:**
- `ENETC_TBMR_WRR(n)` macro correctly encodes weight 1..8 as 0..7 in bits [6:4]
- Devarg parsed and applied in `enetc4_tx_queue_setup()` and `enetc4_tx_queue_start()`
- Memory freed in `enetc4_dev_close()`

---

## Cross-Cutting Observations

**Process:**
- All patches include release note updates
- Feature flags added to `enetc4.ini` where applicable
- No ABI changes introduced
- Copyright years updated to 2024-2026 where code modified

**Coding Style:**
- All patches follow DPDK indentation and brace style
- No forbidden tokens introduced
- Boolean comparisons are explicit (e.g., `if (enable)` on `bool` type is acceptable)

---

## Final Recommendation

**Patches 01-07, 09, 11-14: LGTM** - no changes required.

**Patch 08: Warning** - check return value of `enetc4_vf_get_link_speed()` in `enetc4_process_psi_msg()` before using `msg->status`.

**Patch 10: Error** - cleanup path in `enetc4_vf_dev_intr()` should not return the unregister error code after cleanup succeeded. Suggest returning 0 after logging any unregister failure.

**Patch 02, 04, 14: Info** - minor clarity improvements suggested in error messages / devarg validation (non-blocking).

---

**Overall:** High-quality series with correct implementation of all features. The two items flagged as Error/Warning are edge-case error-handling issues that do not affect normal operation but should be addressed for robustness.