[PATCH v2 0/9] ENETC driver related changes series

[Gagandeep Singh]

Hi,

> -----Original Message-----
> From: Stephen Hemminger <stephen at networkplumber.org>
> Sent: Monday, June 22, 2026 8:36 PM
> To: Gagandeep Singh <G.Singh at nxp.com>
> Cc: dev at dpdk.org; Hemant Agrawal <hemant.agrawal at nxp.com>
> Subject: Re: [PATCH v2 0/9] ENETC driver related changes series
> 
> On Mon, 22 Jun 2026 17:05:08 +0530
> Gagandeep Singh <g.singh at nxp.com> wrote:
> 
> > V2 changes:
> >   - Fixed an un-used variable compilation issue reported on fedora:43-gcc-
> minsize
> >   - Fixed various AI reported issues:
> > 	- Release notes updated for all new devargs
> > 	- enect4.ini features doc updated for scattered RX.
> > 	- removed Not required RTE_PTYPE_UNKNOWN.
> > 	- Fixed mid-frame mbuf leak in SG case.
> > 	- Enabled SG for enetc4 PF also.
> > 	- move to calloc from rte_zmalloc in parse_txq_prior().
> > 	- added vaidation checks on strdup, strtoul.
> > 	- added NC devargs to use cacheable ops conditionally.
> > 	- removed dead code like bd_base_p etc.
> > 	- Fixed rte_cpu_to_le_16() conversion on flags and combined
> > 	  all flags related patches in one patch.
> > 	- Fixed memory leak issue due to TXQ priority patch.
> >    - There were some false positives, I have ignored them:
> > 	Race condition on flags field:
> > 		clean_tx_ring only touches HW-completed BDs
> (next_to_clean→hwci),
> > 		never newly-submitted BDs; doorbell hasn't fired yet.
> > 	Missing dcbf in clean_tx_ring:
> > 		DPDK is single-threaded per queue; TX path always overwrites
> > 		flags completely before dcbf.
> > 	TX dcbf granularity with wrap:
> > 		Safe (AI admits it).
> > 	RX refill flush at wrap:
> > 		In-loop dcbf at i & mask == 0 already flushes aligned groups;
> > 		trailing flush only needed for partial groups.
> > 	RX reading before invalidate:
> > 		dccivac precedes the read for every group in the loop
> >
> > Gagandeep Singh (7):
> >   net/enetc: fix TX BD structure
> >   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
> >
> >  doc/guides/nics/features/enetc4.ini    |   1 +
> >  doc/guides/rel_notes/release_26_07.rst |  10 +
> >  drivers/net/enetc/base/enetc_hw.h      |  13 +-
> >  drivers/net/enetc/enetc.h              |  31 +-
> >  drivers/net/enetc/enetc4_ethdev.c      | 172 ++++++++--
> >  drivers/net/enetc/enetc4_vf.c          | 204 ++++++++++--
> >  drivers/net/enetc/enetc_ethdev.c       |  25 +-
> >  drivers/net/enetc/enetc_rxtx.c         | 430 ++++++++++++++++++++++---
> >  8 files changed, 768 insertions(+), 118 deletions(-)
> >
> 
> Better but still had some AI feedback if I asked it for more complete review.
> Agree that putting new devargs in doc is needed.
> 
> Error
> =====
> 
> [PATCH v2 7/9] net/enetc: add devargs to control VSI-PSI timeout and delay
> 
> drivers/net/enetc/enetc4_vf.c, enetc4_vf_dev_init()
> 
>   kvlist is leaked on the two invalid-value error paths. It is
>   allocated by rte_kvargs_parse() (line 1347) and only freed at
>   line 1385, but both
> 
>       return -1;   /* invalid VSI Timeout, line 1367 */
>       return -1;   /* invalid VSI Delay,   line 1380 */
> 
>   return before that free. A malformed enetc4_vsi_timeout= or
>   enetc4_vsi_delay= leaks the kvargs structure on every probe.
> 
>   Free before returning, e.g.:
> 
>       if (errno != 0 || hw->vsi_timeout == 0) {
>               ENETC_PMD_ERR("Invalid VSI Timeout value = %u",
>                               hw->vsi_timeout);
>               rte_kvargs_free(kvlist);
>               return -1;
>       }
> 
>   (same for the delay path), or restructure with a goto.
> 
> 
Fixed in v3.

> Warning
> =======
> 
> Series (patches 6-9)
> 
>   The new runtime devargs - enetc4_vsi_disable, enetc4_vsi_timeout,
>   enetc4_vsi_delay, enetc4_txq_prior, and nc - are registered via
>   RTE_PMD_REGISTER_PARAM_STRING and noted in the release notes, but
>   doc/guides/nics/enetc4.rst has no Runtime Configuration section
>   describing them. Convention is to document devargs in the NIC guide
>   so users can find the syntax (e.g. the nc=1 / 'a|b|c' priority list
>   formats are non-obvious).
> 
Documentation is done in v3

> Info
> ====
> 
> [PATCH v2 5/9] and [PATCH v2 9/9] - RX multi-segment reassembly
> 
>   In enetc_clean_rx_ring_nc() and enetc_clean_rx_ring_cacheable(),
>   on the frame-last BD:
> 
>       first_seg->pkt_len -= rx_ring->crc_len;
> 
>   reduces pkt_len but leaves the final segment's data_len unchanged,
>   so pkt_len != sum(data_len) when crc_len is non-zero. The old
>   single-segment path kept them equal (pkt_len = data_len = buf_len
>   - crc_len).
> 
>   This is currently unreachable: enetc4 does not advertise
>   RTE_ETH_RX_OFFLOAD_KEEP_CRC, so crc_len is always 0 and the
>   subtraction is a no-op. Flagging only so the asymmetry is on record
>   if KEEP_CRC is ever added - at that point the last segment's
>   data_len would need the same adjustment (and the CRC may straddle
>   the last two segments).
I have noted it down, will submit the changes as a separate patch when
We will support KEEP_CRC in enetc4.