Re: [PATCH v9-1 00/14] net/enetc: add new features for ENETC4 on i.MX95

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 13 Aug 2026 17:43:44 +0530
Gagandeep Singh <g.singh@nxp.com> wrote:

> V9-Changes:
>  - Mid-body variable declaration moved to top of the function.
>  - ret=0 dead code is removed.
>  - Added check on return value of rte_intr_callback_unregister()
>  - The pre-existing reply_msg leak fixed.
>  - Added primary-process guard in enetc4_vf_dev_init
>  - Removed Flow control from enetc4.ini
>  - Fixed VF stats_reset baseline underflows on counter wrap
>  - Fixed VF vlan_pvid_set fails on every successful command
> 
> V8-changes:
>  - Fixed Warning RBMR RMW races between threads by adding lock
>  - Fixed `static const` arrays in `enetc.h`
>  - Proper pthread_mutexattr_t initialization added as suggested by AI
>  - VSIMSGSR stale re-read race is eliminated
> 
> V7-changes:
>  - added missing 14th patch.
> 
> V6-changes:
>  - fixed free() on memory allocated with rte_zmalloc().
>  - fixed eventfd and vector-list leak on interrupt teardown
>  - fixed Rx ring doubling still bypasses
>  - fixed VSI-PSI transaction issued from the interrupt handler
>  - fixed the free-and-reallocate of msg is unnecessary churn
>  - fixed unsynchronized access to tx_pause_active and RBMR
>  - fixed signed shift overflow in the register-dump version field
>  - fixed the fw_size == 0 early return reports the length of "0.0"
>  - unwanted blank lines removed.
>  - parse_txq_prior() returns updated.
> 
> V5-changes:
>  - Patch 1 Build fixes which includes:
>     'prev_seg' undeclared fixed.
>     redefinition of 'dev_rx_offloads_sup' fixed.
>     Error: duplicate rx_enable declaration fixed.
>    PF loses Scattered Rx and Multi-segment Tx
>  - mbuf leak in enetc_xmit_pkts_lso() fixed.
>  - fixed conversion on ENETC4_TXBD_FLAGS_F issue.
>  - fixed link speed decode has no upper bound.
>  - fixed mailbox ops added to the no-VSI ops table.
>  - new devargs documented.
> 
> V4-changes:
>  - fix doc build issue: WARNING: undefined label: pmd_build_and_test
> 
> v3-changes:
>  - fix doc build issue.
>  - fix compilation issue on fedore:43-gcc-minsize
> 
> V2-changes:
>  - compilation fixes.
> 
> V1-changes:
> This series adds new PMD features to the ENETC4 driver targeting the
> NXP i.MX95 NETC IP.
> 
> The series covers:
> 
>  - KEEP_CRC Rx offload: preserve the Ethernet FCS in the receive buffer.
>  - TSO: TCP Segmentation Offload for the VF Tx path.
>  - RSC/LRO: hardware Receive Segment Coalesce for PF and VF Rx paths.
>  - Link speed code: extend the PF-to-VF mailbox field from 4-bit to
>    8-bit to support speeds beyond 10G.
>  - Firmware version: report the NETC IP version via fw_version_get.
>  - Register dump: dump SI, port (PF) and BD ring registers.
>  - Ring parameters: implement rxq_info_get / txq_info_get for the VF.
>  - Link-up interrupt: refresh the cached link speed on each VF link-up
>    interrupt so that link_update returns the current speed immediately.
>  - Stats reset: software snapshot/delta approach for VF counter reset.
>  - Per-queue Rx interrupt: MSI-X per-queue Rx interrupts for the VF,
>    enabling interrupt-driven receive with l3fwd-power.
>  - SI VLAN: hardware port VLAN insertion/removal for PF and VF.
>  - VF link status bitmask: switch VF link status to bitmask encoding
>    to align with the PF and newer kernel driver conventions.
>  - TX PAUSE: VF sets Rx congestion mode when the PF signals TX PAUSE
>    negotiated on the wire; adds Flow control = Y to enetc4.ini.
>  - WRR Tx scheduler: per-ring WRR weights via enetc4_txq_wrr devarg.
> 
> Gagandeep Singh (14):
>   net/enetc: add keep-CRC Rx offload for ENETC4
>   net/enetc: add TSO support for ENETC4 VF
>   net/enetc: add RSC (hardware LRO) support for ENETC4
>   net/enetc: extend PF-VF link speed field to 8 bits
>   net/enetc: support firmware version get for VF
>   net/enetc: support registers dump
>   net/enetc: support ethtool ring parameters
>   net/enetc: refresh link speed on VF link-up interrupt
>   net/enetc: support stats reset for VF
>   net/enetc4: add per-queue Rx interrupt support for VF
>   net/enetc4: add SI-based port VLAN insertion and removal
>   net/enetc4: update VF link status to bitmask encoding
>   net/enetc4: enable Tx PAUSE via VF Rx congestion mode
>   net/enetc4: add WRR Tx scheduler devarg for VF rings
> 
>  doc/guides/nics/enetc4.rst             | 100 +++
>  doc/guides/nics/features/enetc4.ini    |   7 +
>  doc/guides/rel_notes/release_26_11.rst |  21 +
>  drivers/net/enetc/base/enetc4_hw.h     | 137 +++-
>  drivers/net/enetc/base/enetc_hw.h      |   6 +
>  drivers/net/enetc/enetc.h              | 132 +++-
>  drivers/net/enetc/enetc4_ethdev.c      | 459 +++++++++++--
>  drivers/net/enetc/enetc4_vf.c          | 870 +++++++++++++++++++++----
>  drivers/net/enetc/enetc_rxtx.c         | 530 ++++++++++++++-
>  9 files changed, 2084 insertions(+), 178 deletions(-)
> 

Still some AI review feedback items that need addressing.
I say addressing because AI can and does spot things that are not
a real problem.

ENETC4 v9 series review - 14 patches

No Reviewed-by.  The two v8 errors are fixed, but this revision
introduces a regression in patch 14 that silently disables the existing
enetc4_txq_prior devarg, and the patch 11 reply check looks inverted
rather than fixed.  Applied cleanly onto c1a46b9; source review only,
no builds.


Fixed from v8

enetc4_rd64() now reads the 64-bit SI counters as a high/low pair and
oerrors is masked to 32 bits before the subtraction, so the stats_reset
delta no longer underflows on counter wrap.  "Flow control = Y" is gone
from enetc4.ini.  The ring-doubling paths validate the doubled count
against MAX_BD_COUNT.  enetc4_vf_link_update() frees reply_msg on both
"Wrong reply message" paths.  Interrupt teardown frees the vector list
and disables the efds.  txq_prior moved to rte_zmalloc/rte_free
consistently, with the old allocation released before re-parsing.


Errors

Patch 14 - enetc4_txq_prior and enetc4_txq_wrr have no effect at queue
setup.

In enetc4_tx_queue_setup() the hunk changed the target variable but not
the register write:

  uint32_t tx_en = ENETC_TBMR_EN;

  if (priv->hw.txq_prior)
          tx_data |= priv->hw.txq_prior[tx_ring->index];
  if (priv->hw.txq_wrr)
          tx_data |= priv->hw.txq_wrr[tx_ring->index];
  /* enable ring */
  enetc4_txbdr_wr(&priv->hw.hw, tx_ring->index, ENETC_TBMR, tx_en);

tx_data is never read again, so both the priority and the WRR weight are
dropped and only ENETC_TBMR_EN reaches TBaMR.  This is a regression: v8
had "tx_en |= priv->hw.txq_prior[...]", which worked.  The scheduler
bits now only get programmed through enetc4_tx_queue_start(), which
non-deferred queues never go through.  Either OR into tx_en, or drop
tx_en and write tx_data | ENETC_TBMR_EN.

Patch 11 - the vlan_pvid_set reply check is inverted, not fixed.

  if (reply_msg->class_id != ENETC_CLASS_ID_SI_VLAN_ISO) {
          ENETC_PMD_ERR("SI VLAN isolation command failed: ...");
          err = -EINVAL;
  }

For the configuration command classes the driver already handles,
class_id equal to the command class is the failure indication and
ENETC_MSG_CLASS_ID_CMD_SUCCESS (0x1) is success.  Both MAC and VLAN
filter follow that shape:

  if (class_id == ENETC_CLASS_ID_MAC_FILTER) {
          switch (status) { ... default: err = -EINVAL; }
  }

that is, every status under 0x20 is an error.  If 0x20 were also
returned on success, set_mac_addr could never succeed.

SI VLAN isolation (0x24) is a set-type command in the same 0x2x family,
so success should arrive as 0x1 and 0x24 should carry a class-specific
error status.  As written the function still returns -EINVAL on every
successful command, the same symptom as v8 with a different comparand.
The shape to match is:

  if (reply_msg->class_id == ENETC_CLASS_ID_SI_VLAN_ISO) {
          /* class-specific failure: inspect reply_msg->status */
          err = -EINVAL;
  }

The classes where class_id == command class *is* the success reply are
the query classes - LINK_STATUS (0x80), LINK_SPEED (0x81), GET_IP_VER
(0xF0) - which return data in the status byte.  If the PSI really does
return 0x24 on success for this command, please say so in the commit
message, since it contradicts the convention every other setter in the
file uses.


Warnings

Patch 11 - "VLAN offload = Y" added to enetc4.ini for a pvid change.

doc/guides/nics/features.rst lists vlan_pvid_set under "Other dev ops
not represented by a Feature"; pvid has no matrix entry.  The VLAN
offload entry requires vlan_offload_set plus
RTE_ETH_RX_OFFLOAD_VLAN_STRIP / RTE_ETH_TX_OFFLOAD_VLAN_INSERT in
rx_offload_capa / tx_offload_capa.  The PF ops table has no
vlan_offload_set at all and neither offload flag is in
dev_rx_offloads_sup / dev_tx_offloads_sup.  Drop the line.

Patches 02 and 03 - the burst function is selected on enable but never
restored.

enetc4_rx_queue_setup() sets dev->rx_pkt_burst = &enetc_recv_pkts_rsc
when LRO is requested and enetc4_tx_queue_setup() sets
dev->tx_pkt_burst = &enetc_xmit_pkts_lso when TSO is requested; neither
has an else branch.  The defaults are assigned once at probe in
enetc4_dev_hw_init().  After a dev_stop plus rte_eth_dev_configure()
without those offloads the pointers keep their previous values, and
enetc_clean_rx_ring_rsc() then walks a 16B-descriptor ring as if it
were 32B (i += 2, bd_count not doubled, extension slot read from the
neighbouring descriptor).  The RSC assignment also happens before
enetc4_alloc_rxbdr(), so it survives a failed setup.


Info

Patch 09 - enetc4_rd64() reads high and low with no re-read of the low
word, so a carry between the two accesses yields a value 2^32 too
large.  The window is tiny but the error is not; the usual form is
read-high / read-low / read-high-again and retry if the high word
moved.  Related: the operands of | have unspecified evaluation order,
so which half is fetched first is up to the compiler.

Patch 10 - enetc4_vf_rx_queue_intr_enable() writes RBICR1 = 0 and
RBICR0 = ICEN | ICPT(1), overwriting the coalescing timer that patch 03
programs for RSC (ENETC4_RSC_DEF_ICTT).  An application enabling both
LRO and intr_conf.rxq gets per-frame flushes and no coalescing, with no
diagnostic.

Patch 14 - ENETC_TBMR_WRR_MASK and ENETC_TBMR_PRIO_MASK use GENMASK,
which is defined in ntmp.h, not in base/enetc_hw.h where the macros
live.  It works only because enetc.h pulls in both and the expansion is
deferred to the use site; enetc_rxtx.c includes base/enetc_hw.h
directly and would break if it ever used them.

Patch 14 - devargs are now parsed in both enetc4_dev_init()
(priorities, nc) and enetc4_dev_configure() (priorities, wrr, nc).  The
duplicate parse is harmless since parse_txq_prior() frees the previous
array, but hw->nc_mode is reset and re-parsed in configure after the
burst functions were already chosen in init, so an nc change there
cannot take effect.

Patch 01 - enetc_rx_crc_trim() decrements first_seg->pkt_len
unconditionally; if a single-segment cluster ever arrived with
data_len <= crc_len and prev_seg == NULL, pkt_len would wrap.  Not
reachable for valid Ethernet frames, but a data_len > crc_len guard on
the pkt_len adjustment would make it total.