Re: [PATCH v2 00/13] driver/sxe2: fix bugs
Stephen Hemminger <[email protected]>
| Newsgroups | org.dpdk.dev |
|---|---|
| Message-ID | <[email protected]> |
On Sun, 16 Aug 2026 10:52:20 +0800 [email protected] wrote: > From: Jie Liu <[email protected]> > > 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]