[PATCH 00/10] NXP ENETC driver related changes

[Gagandeep Singh]

Hi

> -----Original Message-----
> From: Stephen Hemminger <stephen at networkplumber.org>
> Sent: Saturday, June 20, 2026 3:14 AM
> To: Gagandeep Singh <G.Singh at nxp.com>
> Cc: dev at dpdk.org; Hemant Agrawal <hemant.agrawal at nxp.com>
> Subject: Re: [PATCH 00/10] NXP ENETC driver related changes
> 
> On Sat, 20 Jun 2026 00:14:17 +0530
> Gagandeep Singh <g.singh at nxp.com> wrote:
> 
> > ENETC driver related changes series
> >
> > Gagandeep Singh (8):
> >   net/enetc: fix TX BD structure
> >   net/enetc: fix TX BDs flag overwrite issue
> >   net/enetc: fix queue initialization
> >   net/enetc: support ESP packet type in packet parsing
> >   net/enetc: update random MAC generation code
> >   net/enetc: add option to disable VSI messaging
> >   net/enetc: add devargs to control VSI-PSI timeout and delay
> >   net/enetc4: add cacheable BD ring support with SW cache maintenance
> >
> > Vanshika Shukla (2):
> >   net/enetc: support scatter-gather
> >   net/enetc: set user configurable priority to TX rings
> >
> >  drivers/net/enetc/base/enetc_hw.h |  13 +-
> >  drivers/net/enetc/enetc.h         |  28 +-
> >  drivers/net/enetc/enetc4_ethdev.c | 123 +++++++--
> >  drivers/net/enetc/enetc4_vf.c     | 159 ++++++++++--
> >  drivers/net/enetc/enetc_ethdev.c  |  26 +-
> >  drivers/net/enetc/enetc_rxtx.c    | 411 ++++++++++++++++++++++++++----
> >  6 files changed, 649 insertions(+), 111 deletions(-)
> >
> 
> The AI review shows some thing that need to be addressed before merging.
> 
> [PATCH 04/10] net/enetc: support ESP packet type
> 
> Info: enetc_supported_ptypes_get() adds RTE_PTYPE_TUNNEL_ESP and a trailing
> RTE_PTYPE_UNKNOWN. *no_of_elements is RTE_DIM(ptypes), so the
> 0 entry is counted (not used as a sentinel). It is filtered out by the mask test in
> rte_eth_dev_get_supported_ptypes(), so harmless, but the
> RTE_PTYPE_UNKNOWN line is unnecessary and should be dropped.
> 
> 
> [PATCH 06/10] net/enetc: support scatter-gather
> 
> Warning: scatter Rx reassembly state (first_seg/cur_seg) is held in local variables
> and reset on every call. rx_frm_cnt only advances on the F bit, so work_limit
> won't cut a frame, but the "!(bd_status & LSTATUS_R)" break can exit mid-frame
> if HW has written the leading segments of a multi-segment frame but not yet the
> segment carrying F. On the next call first_seg is NULL again, next_to_clean has
> already advanced past the consumed leading segments, and those mbufs are
> leaked while the tail segments are mis-assembled as a new frame.
> Persist the partial chain across bursts in the ring (e.g.
> rx_ring->pkt_first_seg / pkt_last_seg) instead of locals. (Same pattern is
> reproduced in enetc_clean_rx_ring_cacheable in patch 10.)
> 
> Warning: enetc4 now advertises RTE_ETH_RX_OFFLOAD_SCATTER and
> RTE_ETH_TX_OFFLOAD_MULTI_SEGS (VF) but doc/guides/nics/features/
> enetc4.ini is not updated (Scattered Rx / Multi segment rows).
> 
> Info: the VF dev_info now advertises L3/L4 RX checksum offload, but
> enetc_dev_rx_parse() unconditionally sets RTE_MBUF_F_RX_IP_CKSUM_GOOD |
> RTE_MBUF_F_RX_L4_CKSUM_GOOD and never reports *_BAD. With the offload
> now advertised, an application relying on it will never see a bad-checksum
> indication.
> 
> Info: dccivac(data + (data_len - 1)) / dcbf(data + (seg_len - 1)) underflow to data-1
> when the segment length is 0 (uint16_t promotes to int). The preceding loop
> already covers the final cache line, so the extra op is redundant as well as unsafe
> for len==0.
> 
> 
> [PATCH 07/10] net/enetc: add option to disable VSI messaging
> 
> Warning: new devarg "enetc4_vsi_disable" is registered but not documented in
> doc/guides/nics/enetc.rst.
> 
> 
> [PATCH 08/10] net/enetc: add devargs to control VSI-PSI timeout/delay
> 
> Warning: new devargs "enetc4_vsi_timeout" / "enetc4_vsi_delay" are not
> documented in doc/guides/nics/enetc.rst.
> 
> 
> [PATCH 09/10] net/enetc: set user configurable priority to TX rings
> 
> Error: hw->txq_prior is allocated in parse_txq_prior() with
> rte_zmalloc() but never freed. It leaks on dev_close / re-probe. Free it in the
> close/uninit path (and note it is re-allocated every time the handler runs, so a
> repeated key would leak the prior allocation too).
> 
> Warning: txq_prior is control-path, CPU-only data; rte_zmalloc() consumes
> hugepage memory unnecessarily. Use calloc()/malloc().
> 
> Warning: the parsed value is OR'd straight into TBMR:
> 	tx_en |= priv->hw.txq_prior[tx_ring->index];
> with no mask. ENETC_TBMR_EN is BIT(31) and there is no TBMR priority mask
> defined. A user value with high bits set can corrupt unrelated TBMR control bits.
> Mask the input to the valid TBMR priority field.
> 
> Info: strdup(value) return is not checked; on failure strtok(input_str, "|") is called
> with a NULL first argument, which resumes from strtok's stale internal state
> rather than erroring.
> 
> Warning: new devarg "enetc4_txq_prior" not documented in
> doc/guides/nics/enetc.rst.
> 
> 
> [PATCH 10/10] net/enetc4: add cacheable BD ring support with SW cache
> 
> Warning: enetc4_dev_hw_init() switches rx_pkt_burst/tx_pkt_burst to the cache-
> maintenance variants unconditionally for every enetc4 device (PF and VF). The
> commit message scopes this to non-cache-coherent parts (i.MX95), but the code
> applies it everywhere, adding dcbf/dccivac cost on cache-coherent platforms that
> previously used the _nc fast path. Gate it on a devarg or coherency/platform
> check.
> 
> Warning: the RX payload invalidation uses dccivac (dc civac =
> clean+invalidate). The comment justifies clean-then-invalidate for the
> BD ring (refill dcbf leaves BD lines clean), but payload buffers are not cleaned
> before being handed to HW. If a payload cache line is dirty (stale CPU data from a
> prior use of the mbuf), the clean phase writes it back over the HW-DMA'd data in
> DDR before invalidating -> silent RX corruption on a non-coherent part. Please
> confirm payload lines can never be dirty here, or use invalidate-only.
> 
> Info: struct enetc_bdr gains "uint64_t bd_base_p" but it is never referenced
> anywhere. Remove the dead field.
> 
> Info: the 64-bit BD fast copy
> 	__uint128_t *dst128 = (__uint128_t *)&rxbd_temp;
> 	*dst128 = *(const __uint128_t *)rxbd;
> takes the address of an 8-byte-aligned stack union (rxbd_temp) as __uint128_t*.
> That is an under-aligned 128-bit access (UB); aarch64 tolerates it via ldp/stp but
> it is fragile. Force 16-byte alignment on rxbd_temp or copy as two u64.
> 
> 
> General (series-wide)
> 
> Warning: no release notes. The series adds user-visible features (scatter-gather,
> cacheable BD ring support, four new devargs) with no entry in
> doc/guides/rel_notes/. New driver capabilities and devargs need release-note
> coverage.

I have sent the V2 series which includes most of the fixes. I have mentioned all the fixes in the cover-letter.

Thanks,
Gagan