Hi > -----Original Message----- > From: Stephen Hemminger <[email protected]> > Sent: Saturday, June 20, 2026 3:14 AM > To: Gagandeep Singh <[email protected]> > Cc: [email protected]; Hemant Agrawal <[email protected]> > Subject: Re: [PATCH 00/10] NXP ENETC driver related changes > > On Sat, 20 Jun 2026 00:14:17 +0530 > Gagandeep Singh <[email protected]> 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
