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

Hi

> -----Original Message-----
> From: Stephen Hemminger <[email protected]>
> Sent: Monday, August 10, 2026 2:30 AM
> To: Gagandeep Singh <[email protected]>
> Cc: [email protected]; Hemant Agrawal <[email protected]>
> Subject: Re: [PATCH v4 00/14] net/enetc: add new features for ENETC4 on i.MX95
> 
> On Fri,  7 Aug 2026 16:56:09 +0530
> Gagandeep Singh <[email protected]> wrote:
> 
> > 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 offload support for ENETC4
> >   net/enetc: add TSO support for ENETC4 VF
> >   net/enetc: add RSC (hardware LRO) support for ENETC4
> >   net/enetc: extend link speed code field to 8-bit for PF-to-VF message
> >   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             |  77 +++
> >  doc/guides/nics/features/enetc4.ini    |   8 +
> >  doc/guides/rel_notes/release_26_11.rst |  19 +
> >  drivers/net/enetc/base/enetc4_hw.h     | 129 +++-
> >  drivers/net/enetc/base/enetc_hw.h      |   6 +
> >  drivers/net/enetc/enetc.h              | 135 ++++-
> >  drivers/net/enetc/enetc4_ethdev.c      | 402 +++++++++++--
> >  drivers/net/enetc/enetc4_vf.c          | 779 ++++++++++++++++++++++---
> >  drivers/net/enetc/enetc_rxtx.c         | 529 ++++++++++++++++-
> >  9 files changed, 1939 insertions(+), 145 deletions(-)
> >
> 
> Still lots of issues, the biggest one is that not each patch builds; not bisectable.
> 
> More extensive AI review.
> 
> ENETC4 v4 series review - 14 patches
> 
> No Reviewed-by.  The headline finding from the previous round is
> unchanged: 13 of the 14 commits fail to build.  Only the final commit is clean, so
> git bisect is broken across essentially the whole series.
> 
> Build (applied on c1a46b9, gcc 13.3, -Dwerror=true):
> 
>   [1] 8a0a793 01/14 add KEEP_CRC offload support for ENETC4
>   [1] 63cae7e 02/14 add TSO support for ENETC4 VF
>   [1] b28e1a8 03/14 add RSC (hardware LRO) support
>       ... all fail ...
>   [1] e334f6c 13/14 enable TX PAUSE via VF RX congestion mode
>   [0] b5e71d6 14/14 add WRR Tx scheduler devarg for VF rings
> 
> Three distinct causes, all introduced in patch 01.
> 
> 
> Patch 01
> 
> Error: prev_seg used but never declared (fails commits 01-02).
> 
>   enetc_rxtx.c:597:25: error: 'prev_seg' undeclared
>   enetc_rxtx.c:844:25: error: 'prev_seg' undeclared
> 
> Patch 01 assigns prev_seg and passes it to enetc_rx_crc_trim() in both
> enetc_clean_rx_ring_nc() and enetc_clean_rx_ring_cacheable(), but the
> declaration hunk only changes first_seg/cur_seg to = NULL.  The *prev_seg =
> NULL declaration does not arrive until patch 03.  Move it to patch 01.
> 
> Error: duplicate offload arrays (fails commits 01-13).
> 
>   enetc4_ethdev.c:110:17: error: redefinition of 'dev_rx_offloads_sup'
>   enetc4_ethdev.c:117:17: error: redefinition of 'dev_tx_offloads_sup'
> 
> Patch 01 adds a second copy of both arrays at the top of enetc4_ethdev.c; the
> pre-existing ones near line 97 are not deleted until patch 14.  The deletion hunk
> belongs in patch 01.
> 
> Error: duplicate rx_enable declaration (fails commits 01-13).
> 
>   enetc4_ethdev.c:552:18: error: redeclaration of 'rx_enable' with no
>   linkage
> 
> enetc4_rx_queue_setup() already declares uint32_t rx_enable upstream.
> Patch 01 adds a second one; the removal is in patch 14.
> 
> This is the signature of building only the squashed series.  Please run
> devtools/test-meson-builds.sh, or at minimum a -Dwerror=true build at each
> commit, before v5.
> 
> Error: PF loses Scattered Rx and Multi-segment Tx.
> 
> The old arrays deleted in patch 14 contained RTE_ETH_RX_OFFLOAD_SCATTER
> and RTE_ETH_TX_OFFLOAD_MULTI_SEGS.  The replacements added in patch 01
> do not.  Net result at the tip:
> 
>   upstream                      after the series
>   RX: IPV4|UDP|TCP|SCATTER      RX: IPV4|UDP|TCP|KEEP_CRC|TCP_LRO
>   TX: IPV4|UDP|TCP|MULTI_SEGS   TX: IPV4|UDP|TCP|TCP_TSO|UDP_TSO
> 
> Nothing in any commit message says this is intentional.  enetc4.ini still declares
> "Scattered Rx = Y", and the new RSC path builds multi-segment clusters, so the
> capability is still needed.  The VF arrays keep both - only the PF regresses.
> 
> 
> Patch 02
> 
> Error: mbuf leak in enetc_xmit_pkts_lso().
> 
> Two validation paths drop a TSO frame with "start++; continue;":
> 
>   - hdr_len >= pkt_len || hdr_len > data_len
>   - tso_segsz == 0 || data_unit > ENETC4_LSO_MAX_DATA_UNIT ||
>     hdr_len + tso_segsz > ENETC4_LSO_MAX_FRAME
> 
> Neither writes q_swbd[i].buffer_addr, and neither frees the mbuf, but both count
> the packet in the burst return value.  The application considers it transmitted;
> enetc_clean_tx_ring() never sees it.
> Free-and-continue is the right shape here - add rte_pktmbuf_free(seg) before
> each start++.
> 
> Error: 16-bit byte swap on a uint8_t field.
> 
> In the non-TSO path of the same function:
> 
>   txbd->flags |= rte_cpu_to_le_16(ENETC4_TXBD_FLAGS_F);
> 
> struct enetc_tx_bd.flags is uint8_t.  On big-endian this evaluates to
> 0x8000 and truncates to 0, so the frame-last flag is never set and Tx stalls.
> Upstream and the LSO path in this very function both use plain "txbd->flags |=
> ENETC4_TXBD_FLAGS_F;".  Drop the conversion.
> 
> Warning: ring doubling bypasses MAX_BD_COUNT.
> 
> Both enetc4_alloc_txbdr() (LSO, patch 02) and enetc4_alloc_rxbdr() (RSC, patch
> 03) double nb_desc, but the nb_desc > MAX_BD_COUNT check runs before the
> doubling.  With MAX_BD_COUNT at 64000 the effective ring reaches 128000
> descriptors, and ENETC_RTBLENR_LEN(n) is just
> ((n) & ~0x7) with no upper clamp.  Validate the post-doubling count, or halve the
> accepted nb_desc when LSO/RSC is on.
> 
> 
> Patch 04
> 
> Warning: link speed decode has no upper bound.
> 
>   link.link_speed = (reply_msg->status - ENETC_SPEED_5000) * 1000 + 5000;
> 
> status is uint8_t and the default arm now catches everything from 8 to 255, so a
> garbage or unrecognized code yields a fabricated speed (0xFF -> 253000 Mbps)
> rather than RTE_ETH_SPEED_NUM_UNKNOWN.  Note also that 0xF, previously
> ENETC_SPEED_NOT_SUPPORTED, now decodes as 13000 Mbps in non-legacy
> mode.  Bound the result, or reject codes that do not map to a defined
> RTE_ETH_SPEED_NUM_* value.
> 
> Warning: flipping the reply-status extraction from 4-bit to 8-bit by default is a
> wire-protocol compatibility break.  Users on a PF kernel older than 6.18.37 must
> add vf_link_legacy=1 or link reporting breaks silently.  That deserves an explicit
> line in the release notes, not just "extended the field to 8-bit".
> 
> Info: adds a second #include <rte_kvargs.h> immediately below the existing one.
> 
> 
> Series-wide
> 
> Warning: mailbox ops added to the no-VSI ops table.
> 
> enetc4_vf_ops_no_vsi_m is selected when the user passes enetc4_vsi_disable,
> that is, explicitly asks for no VSI-PSI messaging.
> The series adds fw_version_get, mac_addr_set, mac_addr_add, promiscuous_*,
> allmulticast_*, vlan_filter_set, vlan_offload_set, vlan_pvid_set and stats_reset to
> it.  Most of those reach
> enetc4_msg_vsi_send() directly or through a helper, so they will now attempt a
> transaction on the disabled mailbox and stall until vsi_timeout.  Previously ethdev
> returned -ENOTSUP immediately.  Only the genuinely register-local ops (get_reg,
> rxq_info_get, txq_info_get, and stats_reset if it stays direct-register) belong in that
> table.
> 
> Warning: new devargs undocumented.
> 
> vf_link_legacy (patch 04) and enetc4_txq_wrr (patch 14) are both in
> RTE_PMD_REGISTER_PARAM_STRING but absent from
> doc/guides/nics/enetc4.rst, which documents every pre-existing devarg.
> Same gap flagged in the previous round.
> 
> Info: patch 01 adds two stray blank lines in enetc4_ethdev.c (one before
> enetc4_rx_queue_release, later removed in patch 03); patch 02 adds three more
> around enetc4_alloc_txbdr/enetc4_free_bdr and in struct enetc_bdr.
> 
> Info: enetc_recv_pkts_rsc is assigned to dev->rx_pkt_burst from
> enetc4_rx_queue_setup(), which will silently override the nc=1 burst selection
> made in dev_init.  Worth a guard or at least a comment on precedence.
> 
> I stopped short of a full read of patches 05-14 since the build breakage has to be
> resolved first and several of these findings will shift hunks around.  I can go back
> through 05-14 in detail once there is a v5, or now if you would rather have the
> complete list before the author respins.

V5 sent. I missed to mention V5-changes in the cover letter.
The changes are:
- 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.

Regards,
Gagan