Re: [PATCH v3 00/51] net/sxe2: add Stars SXE2 net driver with fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 26 Aug 2026 16:53:32 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> This series adds the Stars SXE2 (Wuxi Stars Micro System) Ethernet
> driver, including the common driver (drivers/common/sxe2), the net
> driver (drivers/net/sxe2), and its documentation.
> 
> The driver supports:
> - Legacy and switchdev modes with VF representors
> - Scalar and vectorized (SSE/AVX2/AVX512/NEON) Rx/Tx paths
> - FNAV and ACL flow engines with per-flow statistics
> - RSS, QinQ, IPsec, Traffic Management and multi-process support
> 
> In addition to the driver functionality, the series fixes a number of
> correctness issues found during review, including a security context
> leak in the device init error path, a NULL dereference in the
> vectorized Tx mbuf release path, an out-of-bounds representor ID
> access, and a representor LSC loop counter type mismatch. The command
> channel log messages and ioctl channel handling are also aligned with
> the reference implementation.
> 
> This is v3 of the series, incorporating review feedback. All patches
> build independently and no ABI changes are introduced.

Although I appreciate having each part broken into a patch for review;
this large a series gets a little hard to review.

You are posting large series where one patch is fixing a bug which was
introduced by an earlier patch. That is a good way to do development but
is not best practice for final submission. Please consolidate and reorder
patches so that each patch builds, and after each patch there are no
new bugs.

The more verbose (sorry) AI review highlights this.

Findings below come from applying the series and reading the
resulting source.  Numbers are the v3 patch numbers.


Patches that fix earlier patches in the same series
---------------------------------------------------

  09/51  fixed by  49/51  uint8_t vf_id vs uint16_t nb_repr_vf
  15/51  fixed by  48/51  no_of_elements counts the terminator
  22/51  fixed by  48/51  init_rss_err label placement
  23/51  fixed by  48/51  PF match: all ports vs any port
  27/51  fixed by  47/51  stat_index truncation, list handling
  31/51  fixed by  50/51  NULL check in vec Tx mbuf release
  40/51  fixed by  51/51  sxe2_rxq_buf_split_fill rewritten

Please squash each fix into the patch that introduced the bug.
48/51 mixes three unrelated fixes, so split it first.


Errors
------

24/51  probe all requested PF ports

  sxe2_eth_pmd_probe_pf() has "uint16_t owner_id __rte_unused".
  The new loop calls it N times with nothing varying.  The
  function calls rte_eth_dev_pci_allocate(pci_dev, ...), which
  derives the port name from the PCI device, so iteration 2
  fails with -ENOMEM.  The error path then calls
  sxe2_eth_pmd_remove(cdev), destroying the port that did probe.
  Multi-port devargs go from working to failing.  Use owner_id,
  or drop the patch.

33/51  fix RSS action attribute validation

  sxe2_flow_check_rss_action_attr() starts with
  "int32_t ret = ENOTSUP" (no minus), so the three new goto
  l_end paths return a positive errno.  The default: case above
  returns -EINVAL.  Use -ENOTSUP.

44/51  align command structs with historical kernel layout

  Removing __rte_packed_begin moves fields in wire structures:

    sxe2_tm_add_mid_msg    info offset 3 -> 4
    sxe2_tm_add_queue_msg  info offset 5 -> 8
    sxe2_tm_res            size 4 -> 2

  The commit says members are kept as-is, but offsets change.
  If these are the offsets firmware expects, put the numbers in
  the commit message.  Otherwise this breaks TM commands.

46/51  remove drv-sw-stats parameter documentation

  Removing the drv-sw-stats devarg text is right, the devarg is
  gone.  Removing the "Receive Software Statistics" section is
  not: sxe2_stats.c still registers rx_sw_unicast_packets,
  rx_sw_broadcast_packets, rx_sw_multicast_packets,
  rx_sw_drop_packets and rx_sw_drop_bytes, and
  sxe2_rx_sw_stats_update() still runs.  Keep that section, or
  remove the xstats too.


Warnings
--------

Tags

  20 patches have Cc: stable with no Fixes: tag (05/51, and
  33/51 through 51/51).  sxe2 shipped in 26.07, so stable is
  correct, but each needs a Fixes: line.

  Seven Fixes: hashes are 10 characters; DPDK requires 12.  All
  resolve to real commits:

    635084db5d -> 635084db5d57    ac60f302cb -> ac60f302cbef
    66a10eb60e -> 66a10eb60ebe    d8e4e075ac -> d8e4e075acb4
    83866f8d76 -> 83866f8d7638    8cebf80f1e -> 8cebf80f1eda
    a2b79a9351 -> a2b79a93515e

  17/51, 30/51 and 39/51 are refactors carrying Fixes: and
  Cc: stable.  They are not bug fixes; drop the tags.

  35/51 has no functional change (else if -> nested else { if }).
  Please drop the patch.

Commit message does not match the diff

  31/51 describes a buffer ring union and a rename.  It also
  removes #ifndef SXE2_TEST from the AVX512 file, deletes the
  AVX512 bulk free path, rewrites the NEON Rx path (ptype
  extraction, staterr, DD counting, shuffle masks) and adds
  sxe2_tx_desc_fill_4_neon_simple().  It also drops the per-load
  rte_atomic_thread_fence() calls in the NEON Rx loop.  The
  result matches the ixgbe/i40e NEON idiom and looks correct,
  but a fast path memory ordering change must be described.
  This is four or more patches.

  27/51 also sets rxq->fnav_enable in sxe2_queue.c, which is
  unrelated to ACL statistics.

  32/51 bundles four changes: tx_pkt_prepare_dummy, the NEON
  simple/offload split, two debug logs, and the rx_free_thresh
  default.

Other

  27/51 is a new feature (acl-stat-type devarg) inside a fix
  series and has no release notes.  No patch in the series
  touches doc/guides/rel_notes/.

  15/51 sets RTE_ETH_RX_OFFLOAD_RSS_HASH twice: unconditionally,
  then again under SXE2_DEV_CAPS_OFFLOAD_RSS.  One is dead.  The
  same patch moves the PTP block below the IPsec block with no
  effect.

  14/51 sets dev_info->nb_rx_queues and nb_tx_queues, but
  rte_eth_dev_info_get() overwrites both right after the PMD
  callback (lib/ethdev/rte_ethdev.c:4146).  Keep the
  max_mac_addrs part, drop the queue counts.


Info
----

  08/51  "vf" is passed as a %s argument; put it in the format
         string.  Keying the name on cdev->dev->name means two
         PF ports on one PCI function would collide.
  23/51  port_idx = UINT16_MAX is overwritten by the for loop.
  25/51  rename left "sxe2_fnav_cid_mgr" in rte_zmalloc() and
         "sxe2vf_fnav_cid_mgr" in a log message.
  27/51  the ACL devarg is range checked against
         SXE2_FNAV_STAT_ENA_*, and reuses the fnav req/resp
         structs.
  31/51  continuation lines in sxe2_rx_desc_ptype_fill_neon()
         use tabs for alignment.  DPDK uses tabs to indent and
         spaces to align.
  32/51  0x%016x on a uint32_t is misleading.
  36/51  sxe2_drv_mac_link_status_get() now reaches into
         rte_eth_devices[] and calls sxe2_link_update() from the
         command channel.  It is safe, but it inverts layering.

  sxe2.ini lists only x86-32 and x86-64, although
  sxe2_txrx_vec_neon.c is built on arm.  Pre-existing, but this
  series reworks NEON heavily.

  Several commit messages justify a change as "matching the V3
  implementation".  Reviewers cannot see V3.  Please state what
  the code should do and why.