From: Stephen Hemminger <stephen@networkplumber.org>
To: liujie5@linkdatatechnology.com
Cc: dev@dpdk.org
Subject: Re: [PATCH v2 00/13] driver/sxe2: fix bugs
Date: Sun, 16 Aug 2026 09:29:00 -0700 [thread overview]
Message-ID: <20260816092900.75e27213@phoenix.local> (raw)
In-Reply-To: <20260816025233.2727226-1-liujie5@linkdatatechnology.com>
On Sun, 16 Aug 2026 10:52:20 +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 repr 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 | 59 +++--
> 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_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 +-
> 37 files changed, 984 insertions(+), 682 deletions(-)
>
The AI review identified that several of these are bugfixes but
no Fixes: given. Not a problem since sxe2 was introduced after
last stable release 25.11; therefore there is no need to backport.
Here is full AI review (I apologize for AI verbosity)
=====================================================================
sxe2 v2 review summary 13 patches
=====================================================================
STATUS OF v1 COMMENTS
Only three patches changed between v1 and v2:
03/13 __le32/__le64 -> uint32_t/uint64_t in the two new ACL structs
08/13 qstats-removal hunks (sxe2_mp.c/.h, sxe2_stats.c) dropped
12/13 context churn only
Patches 01, 04, 05, 06, 07, 09, 10, 11, 13 are byte-identical to v1.
No v1 finding was addressed. The cover letter carries no changelog,
so there is no way to tell whether the comments were rejected or
missed.
Totals: 11 Errors, 14 Warnings across 9 of 13 patches.
Clean: 02/13, 05/13, 10/13, 11/13.
---------------------------------------------------------------------
SERIES-LEVEL
---------------------------------------------------------------------
Warning Nine of thirteen patches mix unrelated changes under a
subject describing only one of them. Patch 09 is ~1100
lines spanning six independent changes. One logical change
per patch.
Warning Cover letter has no "v2:" changelog section.
---------------------------------------------------------------------
ERRORS
---------------------------------------------------------------------
09/13 Tx vector buffer ring written at 8-byte stride, read at
16-byte stride. sxe2_tx_buffer is 16 bytes, sxe2_tx_buffer_vec
is 8, and they alias through a union. The patch converts the
fill and release paths to buffer_ring_vec but leaves
sxe2_tx_bufs_free_vec() on buffer_ring. That function is the
Tx completion path for all four vector bursts (sse, avx2,
avx512, neon), so buffer[i] resolves to vec slot 2*i: half the
transmitted mbufs leak, the other half are freed twice.
09/13 NULL checks removed from sxe2_tx_queue_mbufs_release_vec().
rte_pktmbuf_free_seg() dereferences m->refcnt with no NULL
test, and queue_reset zeroes the whole ring.
06/13 New PF_BOND block in sxe2_flow_src_split_proc() is immediately
overwritten -- the two pre-existing unconditional assignments
to flow_src_vsi[*][0] were not removed. Member 0 is clobbered
and the else branch is dead.
06/13 adapter->bond_member_cnt is declared but never assigned. On a
PF_BOND device flow_bond_num is 0, every subsequent loop runs
zero times, and flow creation always fails with -EINVAL.
04/13 Error-unwind regression in sxe2_dev_init(): init_rss_err: now
sits below sxe2_security_uinit(), so an sxe2_rss_disable()
failure leaks the security context. Pre-patch it sat above.
[v1]
04/13 sxe2_buffer_split_supported_hdr_ptypes_get() sets
*no_of_elements = RTE_DIM(ptypes) while the array still ends
in an RTE_PTYPE_UNKNOWN sentinel. Callers get a bogus entry
and an inflated count. Compare ice, whose array has no
terminator. [v1]
03/13 Stale count-manager pointer causes double free. mgr is
declared once outside the TAILQ_FOREACH_SAFE loop in
sxe2_flow_rte_list_free() and never reset; query_mgr writes
*mgr_ptr only on success, but free_mgr is called regardless.
Iteration N operates on the mgr freed in N-1. [v1]
03/13 sxe2_flow_free_mgr() falls through to rte_free(mgr) without
TAILQ_REMOVE when engine_type is neither ACL nor FNAV,
leaving a freed node on the list. [v1]
12/13 sizeof(struct sxe2_tm_res) drops 4 -> 2. The struct is an
ioctl payload passed with sizeof() as req_len/resp_len from
four sites in sxe2_cmd_chnl.c. This is a driver/firmware ABI
change, not a cleanup. (The other six structs in this patch
are size-neutral -- I checked each.)
07/13 The added key-length check is unreachable: an identical
"src_key > SXE2_IPSEC_MAX_KEY_LEN" test already exists at the
top of sxe2_security_valid_key() on upstream.
13/13 Deletes the entire drv-sw-stats documentation section as a
side effect of adding acl-stat-type. Nothing in this series
removes that devarg, so docs and code now disagree.
---------------------------------------------------------------------
WARNINGS
---------------------------------------------------------------------
08/13 Blocking firmware commands added to the interrupt handler.
sxe2_event_irq_common_handler() now loops over every VF
representor calling sxe2_drv_mac_link_status_get(), which
reaches pthread_mutex_lock() + blocking ioctl(). One LSC
interrupt costs 1+N serialized kernel round-trips on the EAL
interrupt thread.
08/13 vf_id is uint8_t; nb_repr_vf is uint16_t bounded only by
RTE_MAX_ETHPORTS. Loop never terminates if that exceeds 255.
08/13 Commit message still lists the qstats changes that were
dropped in v2.
09/13 sxe2_tx_queues_vec_prepare() now calls queue_reset(), which
zeroes the buffer ring without freeing what it holds. Confirm
this only runs before first start.
06/13 flow_bond_num is uint8_t, bond_member_cnt is uint16_t, and
neither is clamped to SXE2_MAX_BOND_MEMBER_CNT (4). Overruns
flow_src_vsi[][4] on the stack once the field is populated.
06/13 The RSS key_len/queue_num validation fix (adding the missing
goto l_end) is unrelated to tunnel port handling. Separate
patch.
07/13 dev->security_ctx = NULL is the only substantive change in
this patch and is unrelated to its subject. Separate patch
with a matching subject.
04/13 Duplicate sxe2_switchdev_uninit() and duplicate
sxe2_dev_pci_map_uinit() removed from sxe2_dev_close(). These
are double-free fixes unrelated to cap/res management.
Separate patch. [v1]
03/13 SXE2_PCI_DEVICE_ID_VF_1 0x10b -> 0x10b2 is an unrelated fix
in an ACL-statistics patch. [v1]
03/13 rxq->fnav_enable assignment in sxe2_queues_init() is also
unrelated to this patch's subject. [v1]
03/13 acl-stat-type devarg added here, documented in 13/13. Code
and docs must land together. [v1]
01/13 Subject describes buffer split, but the patch also drops a
debug log, adds a sxe2_link_update() call, 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 that wants
its own patch. [v1]
---------------------------------------------------------------------
INFO
---------------------------------------------------------------------
09/13 NEON DD-count rewrite (popcount -> rte_ctz64(stat)/16) is a
real fix -- the old popcount counted DD bits past a gap.
Deserves its own patch.
08/13 Patch 01 adds sxe2_link_update() inside
sxe2_drv_mac_link_status_get(); this patch removes the now-
redundant call from sxe2_link_update_init(). Two halves of
one change split seven patches apart.
03/13 Trailing else in sxe2_flow_query_mgr() is dead -- the
if/else-if/else at the top already handles it. [v1]
03/13 sxe2_flow_cid_mgr.stat_index is uint16_t but the FW returns
uint32_t; the assignment truncates. [v1]
04/13 dev_info->nb_rx_queues / nb_tx_queues are dead stores;
rte_eth_dev_info_get() overwrites both after the PMD op
returns. [v1]
04/13 Redundant NULL test on addr_info after the new bounds check;
port_idx initialized to UINT16_MAX then unconditionally
overwritten; sxe2_dev_infos_get() returns directly where the
file uses goto l_end. [v1]
01/13 sxe2_rxq_buf_split_fill() re-tests the BUFFER_SPLIT offload
the caller already tested, making its else branch dead. [v1]
prev parent reply other threads:[~2026-08-16 16:29 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-16 2:52 [PATCH v2 00/13] driver/sxe2: fix bugs liujie5
2026-08-16 2:52 ` [PATCH v2 01/13] net/sxe2: add Rx queue buffer split fill support liujie5
2026-08-16 2:52 ` [PATCH v2 02/13] net/sxe2: update switchdev repr VSI ID display format liujie5
2026-08-16 2:52 ` [PATCH v2 03/13] net/sxe2: add ACL engine event statistics support liujie5
2026-08-16 2:52 ` [PATCH v2 04/13] net/sxe2: enhance device cap and res management liujie5
2026-08-16 2:52 ` [PATCH v2 05/13] net/sxe2: improve representor device initialization liujie5
2026-08-16 2:52 ` [PATCH v2 06/13] net/sxe2: refactor flow tunnel port handling liujie5
2026-08-16 2:52 ` [PATCH v2 07/13] net/sxe2: validate IPsec key length against maximum limit liujie5
2026-08-16 2:52 ` [PATCH v2 08/13] net/sxe2: enhance repr event handling and MP code liujie5
2026-08-16 2:52 ` [PATCH v2 09/13] net/sxe2: optimize vectorized Tx/Rx path liujie5
2026-08-16 2:52 ` [PATCH v2 10/13] common/sxe2: allow munmap during kernel reset liujie5
2026-08-16 2:52 ` [PATCH v2 11/13] net/sxe2: clean up duplicate function declarations liujie5
2026-08-16 2:52 ` [PATCH v2 12/13] net/sxe2: clean up structure definitions liujie5
2026-08-16 2:52 ` [PATCH v2 13/13] doc/sxe2: add acl-stat-type parameter documentation liujie5
2026-08-16 16:29 ` Stephen Hemminger [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260816092900.75e27213@phoenix.local \
--to=stephen@networkplumber.org \
--cc=dev@dpdk.org \
--cc=liujie5@linkdatatechnology.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox