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

Hi,



NXP Confidential
> -----Original Message-----
> From: Stephen Hemminger <[email protected]>
> Sent: Saturday, August 15, 2026 8:47 PM
> To: Gagandeep Singh <[email protected]>
> Cc: [email protected]; Hemant Agrawal <[email protected]>
> Subject: Re: [PATCH v9-1 00/14] net/enetc: add new features for ENETC4 on
> i.MX95
>
> On Thu, 13 Aug 2026 17:43:44 +0530
> Gagandeep Singh <[email protected]> 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.

I checked the kernel PF reply. The PSI reports command completion in the reply class_id: a
successful command returns ENETC_MSG_CLASS_ID_CMD_SUCCESS (0x1),
while any other class_id (e.g. CMD_NOT_SUPPORT 0x3) indicates a
failure. The command class value (0x24) is only used in the outgoing VF-to-PSI header and is never echoed back in the reply,
So treating class_id != CMD_SUCCESS as failure.

>
> 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.

FALSE POSITIVE, no change. The claim that an nc change in dev_configure() "cannot take effect" does not hold:

nc_mode is derived from the device's devargs string, which is immutable after probe — re-parsing in configure always produces the exact value dev_init() computed. There is no possible "nc change" between the two calls.
The burst functions set in dev_init() (lines 1501-1503) are only a tentative default. The authoritative selection happens later in enetc4_tx_queue_setup() (lines 497-498) and enetc4_rx_queue_setup() (lines 728-730), which read hw->nc_mode and run after dev_configure(). So the configure-time value is fully honored at queue setup. The duplicate parse is redundant but harmless (as the comment itself concedes), and parse_txq_prior/wrr free the previous array each time, so no leak.

>
> 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.

Its False positive, - `crc_len` is always `RTE_ETHER_CRC_LEN = 4`
ENETC hardware enforces the Ethernet minimum frame size (64 bytes including FCS)
Any frame delivered to the driver has `data_len >= 64`, so `last_seg->data_len > 4` is always true


I have sent V10 series with the fixes.