Re: [PATCH 00/10] NXP ENETC driver related changes

[Stephen Hemminger <stephen@networkplumber.org>]

On Sat, 20 Jun 2026 00:14:17 +0530
Gagandeep Singh <g.singh@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.