Hi Stephen,
Thanks for the review. All points will be addressed in v2: the real bug fixes have been split out and moved ahead with Fixes tags, the doc gaps are filled, and the tunables that made the burst functions spin are dropped. At 2026-10-11 02:30:00, "Stephen Hemminger" <[email protected]> wrote: >On Sat, 10 Oct 2026 22:44:51 +0800 >Jiacheng Ye <[email protected]> wrote: > >> This series fixes offload capability advertisement and cmdq/compact CQE >> configuration, and adds FEC, CIR drop statistics, VF LAG/link state, COS >> mask/map, mbox recording and telemetry, RSS hash configuration, compact CQE >> Rx/Tx paths, TSO segment expansion, pseudo-header checksum, per-tile cmdq >> ops, SP560 NIC support, and device kvargs tunables. >> >> Signed-off-by: Jiacheng Ye <[email protected]> > > >Large patch series, the partial and not complete list of problems identified >by AI review are: > >Review: [PATCH v1 00/17] net/hinic3 updates (Jiacheng Ye) > >Applied to main (d129414) with git am --3way, all 17 apply cleanly. >Per-commit build with -Dwerror=true passes for every patch. > >Series summary >-------------- > >This mixes bug fixes, new hardware (SP560), new features, and a >rewrite of both data paths. Several real fixes are buried inside >feature patches and need to move to the front with Fixes tags: > > - duplicate TAILQ_REMOVE in hinic3_flow_destroy() (09) > - VXLAN VNI spec written into key_mask (09) > - NULL deref of hwdev->dev_handle in set_vport_enable (06) > - secondary process never got dev_ops (14) > - HINIC3_DEV_ID_VF_SP230 changed 0x3750 -> 0x022a (10) [Jiacheng] Accepted. These five fixes will be split into standalone patches and moved to the front, each with a Fixes tag. > >No documentation beyond release notes. hinic3.rst needs SP560 and >the new devargs; features/hinic3.ini needs FEC, Burst mode info and >the new rte_flow items (geneve, vxlan_gpe). [Jiacheng] Accepted. hinic3.rst now lists SP560 and documents the new devargs, and hinic3.ini adds FEC, Burst mode info, geneve and vxlan_gpe. > >Patch 09 uses nic_type (HINIC3_IS_SP230_NIC) but nic_type is only >set in patch 10. Put the NIC type detection first. [Jiacheng] Accepted. NIC type detection is moved ahead of the flow refactor. > >Several new tunables (rx_empty_loop, tx_free_loop) make the burst >functions spin. rx_burst/tx_burst must return immediately; spinning >on empty CQ or full SQ is the application's choice, not the PMD's. [Jiacheng] Accepted. Both tunables and the spinning behavior are removed; the Tx path reclaims once and returns the short count. > >Errors >------ > >[01/17] net/hinic3: fix offload and cmdq/compact CQE config > >QINQ_STRIP and QINQ_INSERT are advertised but not implemented. >Nothing in hinic3_tx.c reads vlan_tci_outer and nothing in Rx sets >RTE_MBUF_F_RX_QINQ_STRIPPED. Adding HINIC3_PKT_TX_QINQ_PKT to >HINIC3_TX_OFFLOAD_MASK only routes the packet down the offload path; >the outer tag is never inserted. Drop both capabilities. > >VXLAN_TNL_TSO is now advertised unconditionally, but >hinic3_tx_offload_pkt_prepare() returns -EINVAL for VXLAN packets >when !HINIC3_SUPPORT_VXLAN_OFFLOAD(). Gate it like the others: > > if (HINIC3_SUPPORT_VXLAN_OFFLOAD(nic_dev)) > info->tx_offload_capa |= RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO; > >in hinic3_dev_tnl_tso_support(). [Jiacheng] Accepted. Both QINQ capabilities are dropped. VXLAN_TNL_TSO is now gated by HINIC3_SUPPORT_VXLAN_OFFLOAD(). > >[02/17] net/hinic3: add CIR drop statistics and VF LAG support > >hinic3_cmd_vf_lag() returns uint8_t but computes > bitmap[func_id / 32] & (1ULL << (func_id % 32)) >Bits 8..31 are truncated to 0, and the caller compares "== 1", so >only func_id % 32 == 0 ever matches. Return a bool: > > return !!(vf_lag_info.vf_lag_bitmap.vf_bit_map[func_id / 32] & > RTE_BIT32(func_id % 32)); > >and test the result as a boolean in hinic3_tx_queue_setup(). [Jiacheng] Accepted. > >get_port_cir_drop() never sets xstats[i].id. > >hinic3_xstats_calc_num() now adds HINIC3_PHYPORT_XSTATS_NUM for VF, >but hinic3_dev_xstats_get() and _get_names() still return before the >phy port stats for VF. Count and returned entries disagree. Remove >HINIC3_PHYPORT_XSTATS_NUM from the VF branch. [Jiacheng] Accepted. > >[09/17] net/hinic3: refactor flow rule parsing > >On SP620, HINIC3_FLOW_KIND_IPINIP is parsed by >hinic3_flow_parse_fdir_filter(). That parser has no tunnel state: >the inner IPV4/IPV6 item overwrites the outer key and tunnel_type >stays NORMAL. The installed rule matches the inner addresses against >the outer header of non-tunnel traffic. Reject IPIP on SP620 until >it is parsed by the tunnel parser. [Jiacheng] Accepted. On SP620, an IPIP rule would degrade into a plain IP match, so rejecting it (returning NULL) is more correct. > >hinic3_flow_parse_action() now uses the last non-VOID action and >ignores the rest. MARK/QUEUE, COUNT/QUEUE or DROP/QUEUE are accepted >and silently become QUEUE. Require exactly one action: > > for (act = actions; act->type != END; act++) { > if (act->type == VOID) > continue; > if (found) return rte_flow_error_set(..., "Only one action"); > found = act; > } > [Jiacheng] Accepted. Exactly one non-VOID action is now required. >[10/17] net/hinic3: add SP560 NIC type identification and handling > >hinic3_func_init() reads rq_wqe_type from >/sys/module/hinic{3,5}/parameters and fails probe with -EINVAL if >it is not there. With the device bound to vfio-pci the kernel driver >is normally not loaded, so probe fails. A PMD must not take device >configuration from another driver's module parameter. Use firmware >feature bits (RX_HW/SW_COMPACT_CQE are already there) and the >rx_cqe_compact_en devarg from patch 11; drop hinic3_read_module_param. > >hinic3_cmdq_get_ops() caches the result in a function static. The >first probed device decides the ops for every device in the process; >SP230 (HTN) plus SP620 in one system gets the wrong command format. >Remove the cache: > > return cmdq_ops[nic_dev->nic_type]; > [Jiacheng] Accepted. >[11/17] net/hinic3: add device kvargs parameters support > >hinic3_nic_common_config_get() runs after hinic3_func_init(). On a >parse error hinic3_dev_init() returns without undoing func_init, so >hwdev, interrupts and queues leak. Parse devargs first, before any >allocation. [Jiacheng] Accepted. > >[12/17] net/hinic3: add compact CQE Rx path and ptype table > >hinic3_init_rx_ptype_table() is called from hinic3_func_init(), >before hinic3_dev_init() parses devargs and calls >hinic3_nic_feature_init(). config.rx_cqe_compact_en is still 0, so >the normal-CQE table is always built and >hinic3_recv_pkts_compact_cqe() indexes it with compact ptype values. >Build the table after hinic3_nic_feature_init(). > >A table allocation failure is only logged; both Rx paths then >dereference a NULL ptype_tbl. Fail probe instead. > [Jiacheng] Accepted. >[13/17] net/hinic3: expand TSO from 127 to 255 segments > >wqe_info->last_cpy_mbuf_usable is set only in the overflow branch of >hinic3_is_tso_sge_valid() and never cleared. wqe_info is declared >once per burst, so every later packet that goes through the copy path >has its last copy mbuf truncated. Clear it with sge_cnt and >cpy_mbuf_cnt in hinic3_get_tx_offload(): > > wqe_info->last_cpy_mbuf_usable = 0; > >When the payload after the first 28 segments exceeds 227 * 4K, the >copy path cuts the last copy mbuf to header + N * MSS and reports the >packet as sent. The tail of the TCP payload is lost without any >error. Treat this as invalid (return false) and count it. [Jiacheng] Accepted. The oversized payload will be rejected and counted. > >[16/17] net/hinic3: add per-tile prefetch/drop/CEQ cmdq ops > >STN prepare_rq_ctxt_ceq_and_prefetch() sets pi_paddr_hi/lo to >rq_ci_paddr for compact RQs, then hinic3_rq_prepare_ctxt() >unconditionally overwrites both with pi_dma_addr after the call. >Either the assignment is needed and compact RQ context is wrong, or >it is dead code. Skip the pi_dma_addr assignment for >HINIC3_COMPACT_RQ_WQE, or call the op last. [Jiacheng] Accepted. > >[17/17] net/hinic3: add mbox count telemetry support > >hinic3_telemetry_info() walks every valid ethdev port and casts its >dev_private to struct hinic3_nic_dev. Any other PMD in the process >gets its private data dereferenced as hinic3. Check the driver >before touching dev_private. > >struct mbox_cnt_info is (32 + 16) * 4112, about 197KB, on the stack >of the telemetry thread. Do not build an array at all; add each >port to the dict as it is visited. > >Better: drop the private telemetry endpoint and report the mbox >counters as xstats, or implement eth_dev_ops.eth_dev_priv_dump >(rte_eth_dev_priv_dump) which exists for this; the mbox history dump >from patch 04 belongs there too. [Jiacheng] Accepted. Implemented eth_dev_priv_dump() and moved the mbox/AEQ history dump there. > >Warnings >-------- > >[03/17] "Fec param is valid, failed to set fec param." should say >invalid. nic_dev->fec_mode is written and never read. >hwdev->speed is only updated on link up, so fec_get_capability >reports a stale speed after link down. > [Jiacheng] Accepted. >[04/17] "mbox_header = *((uint64_t *)data)" is an unaligned, >aliasing read of a uint8_t array. Use memcpy(). > [Jiacheng] Accepted. >[05/17] cos_mask comes from firmware (uint8_t) and indexes >cos_map[HINIC3_COS_NUM_MAX]. Bound it: > txq->cos = nic_dev->cos_map[txq->cos & (HINIC3_COS_NUM_MAX - 1) > & nic_dev->cos_mask]; >The removal of the HTN cos limit and of "% HINIC3_COS_NUM_MAX_HTN" >for VF is not explained in the commit message. [Jiacheng] Accepted. The cos_mask index is now bounded with (HINIC3_COS_NUM_MAX - 1), and the commit message explains the removed HTN limit and VF modulo. > >[06/17] link_status and vf_valid_status are now bitfields in one >byte. link_status is written by the LSC interrupt thread and >vf_valid_status by the control thread; the read-modify-write loses >updates. Use two plain uint8_t. > >hinic3_dev_set_link_up() on VF returns -EAGAIN when the PF carrier >is down. set_link_up is the admin state; carrier down is reported >through link_update, not as an error. [Jiacheng] Accepted. > >[09/17] Patch 09 depends on nic_type from patch 10; at this commit >SP230 gets the STN TCAM limit (2048). Reorder. > [Jiacheng] Accepted. >GENEVE and VXLAN_GPE specs are cast to rte_flow_item_vxlan. The VNI >offset happens to match, but masks on the other fields (geneve >protocol and option length, gpe protocol) are silently ignored. >Use the proper item types and reject unsupported mask fields. > [Jiacheng] Accepted. GENEVE and VXLAN_GPE use their proper item types and unsupported mask fields are rejected. >hinic3_flow_query() returns 0 without filling hits/bytes and does >not check the action is COUNT; it only logs the rule at INFO. Remove >.query until COUNT is supported. [Jiacheng] .query is removed for now and will be added back once COUNT is supported. > >[10/17] The SP230 VF device ID change (0x3750 -> 0x022a) drops >support for the old ID. Separate fix patch with an explanation. > [Jiacheng] Accepted. The VF device ID change is now a standalone fix patch with an explanation. >[11/17] No RTE_PMD_REGISTER_PARAM_STRING() for the new devargs and >no documentation. strtol() result is not checked for trailing >garbage and negative values go into unsigned fields. [Jiacheng] Accepted. RTE_PMD_REGISTER_PARAM_STRING() and docs are added, and strtol() now rejects trailing garbage and negative values. > >Defaults change without mention: Rx CQE coalesce 63 -> 7, timer >15 -> 8, Tx CI pending limit 3 -> 2, coalescing time 16 -> 2. [Jiacheng] Documented. The default parameters are adjusted to reduce latency. > >In a secondary process hinic3_dev_init() re-parses devargs into the >shared nic_dev->config, overwriting the primary's settings. Skip it >when not primary. [Jiacheng] Accepted. Devargs are parsed only in the primary process so secondary processes do not overwrite the primary's settings. > >At this commit rx_cqe_compact_en defaults to 1 with no capability >check (added in patch 12), so vport enable requests compact CQE on >hardware without support. [Jiacheng] Accepted. rx_cqe_compact_en is now only enabled when the hardware advertises the compact CQE capability. > >[12/17] rx_empty_loop makes hinic3_recv_pkts() spin re-reading the >same CQE. Drop the devarg. [Jiacheng] Accepted. > >Non-ASCII characters in C comments ("VXLAN 、NVGRE ..."). Use ASCII. > [Jiacheng] Replaced with ASCII. >[14/17] Many unrelated changes: secondary process dev_ops, SP230 VF >software stats, CIR drop SP620 gating, cos_map SP230 gating, >DEFAULT_DRV_FEATURE bit 27 (RX_SW_COMPACT_CQE), queue state reset in >dev_stop, txq depth check, tx_burst_mode_get, new xstats. Split. [Jiacheng] Accepted. > >tx_free_loop (default 1000) makes both xmit functions spin on a full >SQ. Reclaim once and return the short count. [Jiacheng] Accepted. Removed; the Tx path reclaims once and returns. > >Both xmit functions break out of the burst when >hinic3_get_tx_offload() fails. A bad packet then blocks the queue >forever since the application retries it. Free it, count it, and >continue. [Jiacheng] Accepted. The offending packet is now freed, counted, and the burst continues instead of breaking out. > >[15/17] Pseudo-header checksums are computed inside tx_burst by >writing into packet headers. That is what .tx_pkt_prepare is for >(rte_net_intel_cksum_prepare() plus the IPv6 extension walk). As >written it modifies data of shared/indirect mbufs and assumes all >headers are in the first segment. [Jiacheng] Accepted. Pseudo-header checksums moved to tx_pkt_prepare, and shared/indirect mbufs skip the offload to avoid modifying shared data. > >Outer IPv4 UDP checksum is only written if the application left it >nonzero. RTE_MBUF_F_TX_OUTER_UDP_CKSUM means compute it. [Jiacheng] Accepted. The outer UDP checksum is now always computed. > >Info >---- > >[03/17] HINIC3_FEC_MODE_* equal RTE_ETH_FEC_MODE_CAPA_MASK() values; >use the ethdev macros. Pass NULL, not 0, for the unused pointer. >Typo "covert". [Jiacheng] Accepted. > >[04/17] Storing the constant PCI address in every mbox record is >redundant. Dumping all AEQ entries at ERR on each timeout floods >the log. [Jiacheng] Accepted. The redundant fields are removed and the full AEQ dump only runs from eth_dev_priv_dump. > >[08/17] rss_type is saved even when hinic3_set_rss_type() fails. >Compact CQE sets the RSS hash flag on every packet whenever any RSS >type is configured. > [Jiacheng] Accepted. >[09/17] vlan_flag is no longer set in the TCAM key, so rules now >match tagged traffic as well. Mention it in the commit message. > [Jiacheng] Accepted. >[12/17] On SP230 the Rx interrupt vectors are set up even when >intr_conf.rxq is 0; not explained. dev_supported_ptypes_get misses >L2_ETHER, L4_FRAG, L4_ICMP that the table produces. > [Jiacheng] On SP230 the device asserts the Rx MSI-X vector during aggregation, so the vectors are set up with coalescing/auto-mask to have those assertions masked instead of preempting a core. The missing L2_ETHER, L4_FRAG and L4_ICMP ptypes are now returned. >[13/17] Comment says "SP230 is 9600, SP560/620 is 4K"; the code >uses 65536 for SP230 and 9600 otherwise. [Jiacheng] Accepted. > >[14/17] tx_burst_mode_get stores txmode.offloads in uint16_t and does >not say which Tx function (compact or normal) is active, which is >the useful part. rte_eth_dev_attach_secondary() in func_init is >redundant; probe already attached. "rte_prefetch0(++tx_pkts)" >prefetches the array slot, not the mbuf. [Jiacheng] Accepted. > >Leftovers: > is_sp230_pci_dev() declared, never defined > HINIC3_MAX_QUEUE_DEPTH defined in both hinic3_ethdev.h and .c > HINIC3_QUEUE_STAT_CNTRS, HINIC3_RSS_QUEUE_BUF, IPV4_VERSION, > IPV6_VERSION unused > struct hinic3_tx_offload_info unused > txq->tx_free_loop set, never read > service_cap.port_cos_valid_bitmap unused > hinic3_ethertype_filter.flags never set [Jiacheng] Accepted. All of these leftovers are cleaned up. Thanks again for the review. Regards, Jiacheng
