Re: [PATCH v1 00/13] net/sxe2: fix bugs

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 14 Aug 2026 20:21:51 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> Jie Liu (13):
>   net/sxe2: add Rx queue buffer split fill support
>   net/sxe2: update switchdev repr VSI ID display format
>   net/sxe2: add ACL engine event statistics support
>   net/sxe2: enhance device cap and res management
>   net/sxe2: improve representor device initialization
>   net/sxe2: refactor flow tunnel port handling
>   net/sxe2: validate IPsec key length against maximum limit
>   net/sxe2: enhance repre event handling and  MP code
>   net/sxe2: optimize vectorized Tx/Rx path
>   common/sxe2: allow munmap during kernel reset
>   net/sxe2: clean up duplicate function declarations
>   net/sxe2: clean up structure definitions
>   doc/sxe2: add acl-stat-type parameter documentation
> 
>  doc/guides/nics/sxe2.rst                   |  33 +--
>  drivers/common/sxe2/sxe2_common.c          |   5 +-
>  drivers/common/sxe2/sxe2_ioctl_chnl.c      |   8 +-
>  drivers/net/sxe2/sxe2_cmd_chnl.c           | 215 +++++++++++++++--
>  drivers/net/sxe2/sxe2_cmd_chnl.h           |  15 +-
>  drivers/net/sxe2/sxe2_drv_cmd.h            |  47 ++--
>  drivers/net/sxe2/sxe2_dump.c               |  10 +-
>  drivers/net/sxe2/sxe2_ethdev.c             | 203 +++++++++-------
>  drivers/net/sxe2/sxe2_ethdev.h             |  38 +--
>  drivers/net/sxe2/sxe2_ethdev_repr.c        |  13 +-
>  drivers/net/sxe2/sxe2_flow.c               | 259 +++++++++++++++++----
>  drivers/net/sxe2/sxe2_flow.h               |   6 +-
>  drivers/net/sxe2/sxe2_flow_define.h        |  13 +-
>  drivers/net/sxe2/sxe2_flow_parse_action.c  |  37 ++-
>  drivers/net/sxe2/sxe2_flow_parse_pattern.c | 113 ---------
>  drivers/net/sxe2/sxe2_flow_parse_pattern.h |   7 -
>  drivers/net/sxe2/sxe2_ipsec.c              |   5 +
>  drivers/net/sxe2/sxe2_irq.c                |  27 ++-
>  drivers/net/sxe2/sxe2_mac.c                |  10 +-
>  drivers/net/sxe2/sxe2_mp.c                 |  65 +++---
>  drivers/net/sxe2/sxe2_mp.h                 |   3 +-
>  drivers/net/sxe2/sxe2_queue.c              |   2 +
>  drivers/net/sxe2/sxe2_queue.h              |   7 +-
>  drivers/net/sxe2/sxe2_rx.c                 |   5 +-
>  drivers/net/sxe2/sxe2_security.c           |   1 +
>  drivers/net/sxe2/sxe2_stats.c              |  10 +-
>  drivers/net/sxe2/sxe2_switchdev.c          |  12 +-
>  drivers/net/sxe2/sxe2_tx.c                 |  42 +++-
>  drivers/net/sxe2/sxe2_tx.h                 |   4 +
>  drivers/net/sxe2/sxe2_txrx.c               |  19 +-
>  drivers/net/sxe2/sxe2_txrx_poll.h          |   2 -
>  drivers/net/sxe2/sxe2_txrx_vec.c           |  77 +++---
>  drivers/net/sxe2/sxe2_txrx_vec.h           |   1 +
>  drivers/net/sxe2/sxe2_txrx_vec_avx2.c      |  10 +-
>  drivers/net/sxe2/sxe2_txrx_vec_avx512.c    | 123 +---------
>  drivers/net/sxe2/sxe2_txrx_vec_common.h    |   5 +-
>  drivers/net/sxe2/sxe2_txrx_vec_neon.c      | 215 +++++++++++------
>  drivers/net/sxe2/sxe2_txrx_vec_sse.c       |  10 +-
>  drivers/net/sxe2/sxe2_vsi.c                |   8 +-
>  39 files changed, 993 insertions(+), 692 deletions(-)
> 

Claude Opus 5 AI review found lots of things to address.
It stopped after the first 5.

Overall: the series is titled as bug fixes but most patches mix feature
work, renames, log-message rewording, and unrelated fixes into single
commits. Several of the real bug fixes are buried in patches whose
subject describes something else, and none carry Fixes:/Cc: stable.
Please split these out.

Patch 3/13 (net/sxe2: add ACL engine event statistics support)

Error: stale count-manager pointer causes double free.

  In sxe2_flow_rte_list_free(), mgr is declared once outside the
  TAILQ_FOREACH_SAFE loop and never reset per iteration.
  sxe2_flow_query_mgr() writes *mgr_ptr only on the full success
  path; every error path leaves the caller's mgr untouched.
  sxe2_flow_free_mgr() is then still called (it is gated on the COUNT
  bit, not on ret), so on iteration N it operates on the mgr freed in
  iteration N-1: TAILQ_REMOVE() on an already-removed node plus a
  second rte_free().

  This is reachable whenever an expanded flow has more than one
  sxe2_flow entry with a COUNT action and the FW query command fails
  on a later entry.

  The new "user_id == 0 && mgr" guard fixes the first-iteration NULL
  case but not this one. Reset the pointer per iteration:

	TAILQ_FOREACH_SAFE(hw_flow, &flow->sxe2_flow_list, next, hw_flow_temp) {
		mgr = NULL;

  or declare mgr inside the loop body.

Error: sxe2_flow_free_mgr() frees without unlinking on unknown engine.

  If flow->engine_type is neither ACL nor FNAV, neither branch runs,
  cid_mgr_list stays NULL, no TAILQ_REMOVE happens -- and the function
  still falls through to rte_free(mgr). That leaves a freed node on
  whichever list it came from. Add an else branch that logs and skips
  the free, or return early.

Warning: sxe2_flow_query_mgr() returns 0 for an unrecognized engine
  type without setting *mgr_ptr. Callers treat 0 as "mgr is valid":
  sxe2_flow_query_count() does count->hits = mgr->hits with mgr still
  NULL. The COUNT action is currently rejected for non-FNAV/ACL
  engines in sxe2_flow_parse_action(), so this is not reachable today,
  but the contract is wrong. Return -ENOTSUP on that path.

Info: the trailing else in sxe2_flow_query_mgr()

	} else {
		PMD_LOG_ERR(DRV, "query flow engine neither FNAV nor ACL");
		ret = -ENOTSUP;
	}

  is dead -- the if/else-if/else at the top of the function already
  goto l_end for anything that is not ACL or FNAV.

Warning: unrelated fix in this patch.

	-#define SXE2_PCI_DEVICE_ID_VF_1 0x10b
	+#define SXE2_PCI_DEVICE_ID_VF_1 0x10b2

  A VF PCI device ID correction has nothing to do with ACL statistics.
  Separate patch, with Fixes: and Cc: stable@dpdk.org.

Warning: the rxq->fnav_enable assignment added to sxe2_queues_init()
  is also unrelated to this patch's subject.

Warning: the acl-stat-type devarg is added here but documented in
  patch 13/13. Code and documentation must land in the same commit.

Info: struct sxe2_flow_cid_mgr.stat_index is uint16_t, but
  sxe2_drv_flow_acl_get_stat_id() returns uint32_t and
  sxe2_drv_acl_query_stat_req.stat_id is __le32. The assignment
  mgr->stat_index = stat_index truncates. Pre-existing on the fnav
  side, but the new ACL path inherits it.

Patch 4/13 (net/sxe2: enhance device cap and res management)

Error: error-unwind regression in sxe2_dev_init().

  Before this patch:

	init_flow_err:
	init_rss_err:
		sxe2_security_uinit(dev);
	init_security_err:

  After:

	init_flow_err:
		sxe2_security_uinit(dev);
	init_rss_err:
	init_security_err:
		sxe2_intr_uninit(dev);

  sxe2_rss_disable() runs after sxe2_security_init() succeeded, so a
  failure there now jumps past sxe2_security_uinit() and leaks the
  security context. Move init_rss_err back above the security_uinit
  call.

Error: buffer-split ptype count includes the sentinel.

  sxe2_buffer_split_supported_hdr_ptypes_get() now sets
  *no_of_elements = RTE_DIM(ptypes), but the array still ends with a
  RTE_PTYPE_UNKNOWN terminator left over from the sentinel-based
  convention. rte_eth_buffer_split_get_supported_hdr_ptypes() copies
  all no_of_elements entries verbatim, so callers get a bogus
  RTE_PTYPE_UNKNOWN entry and an inflated count. Compare ice, whose
  array has no terminator. Drop the RTE_PTYPE_UNKNOWN element.

Warning: sxe2_dev_close() cleanup fixes are unlabelled.

  This patch removes a duplicate sxe2_switchdev_uninit() and a
  duplicate sxe2_dev_pci_map_uinit() from sxe2_dev_close(). Those are
  double-free fixes and belong in their own patch with Fixes: and
  Cc: stable@dpdk.org, not inside a "cap and res management" commit.

Info: dev_info->nb_rx_queues / nb_tx_queues are dead stores.
  rte_eth_dev_info_get() overwrites both from dev->data after the PMD
  op returns (lib/ethdev/rte_ethdev.c). Drop them.

Info: sxe2_dev_infos_get() returns -EINVAL directly on the new NULL
  vsi check while the rest of the file uses goto l_end. Minor
  inconsistency.

Info: after the new res_type bounds check in sxe2_dev_pci_res_seg_map(),
  the following "if (!addr_info || ...)" test is redundant --
  addr_info is &array[res_type] and can never be NULL.

Info: in sxe2_switchdev_repr_match(), port_idx is initialized to
  UINT16_MAX and then unconditionally overwritten by the for loop.

Patch 1/13 (net/sxe2: add Rx queue buffer split fill support)

Warning: the subject describes buffer split, but the patch also drops
  a debug log from __sxe2_drv_cmd_params_fill(), adds a
  sxe2_link_update() call to sxe2_drv_mac_link_status_get(), fixes an
  error path in sxe2_drv_udp_tunnel_get(), and rewords a dozen log
  messages. The udp_tunnel_get change is a real fix (the function
  previously copied response fields after a failed command); it wants
  its own patch with a Fixes: tag.

Info: sxe2_rxq_buf_split_fill() re-tests
  rxq->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT, which the caller
  has already tested, so the else branch clearing hdr_len /
  split_type_mask is unreachable. Either drop the check in the helper
  or drop it in the caller.

Patch 5/13 (net/sxe2: improve representor device initialization)

  The added sxe2_stats_init() call and its l_init_irq_ctxt_err label
  unwind correctly. No findings.