Hi Stephen,
Thanks for the review. Replies below; the [PATCH nn/28] numbers refer to the v5 series. At 2026-11-09 21:14:11, "Stephen Hemminger" <[email protected]> wrote: >On Fri, 30 Oct 2026 22:47:04 +0800 >Jiacheng Ye <[email protected]> wrote: > >> This series fixes offload capability advertisement, cmdq/compact CQE >> configuration, and several defects found during review, and adds NIC >> type identification, FEC, CIR drop statistics, VF LAG/link state, COS >> mask/map, mbox recording, RSS hash configuration, compact CQE Rx/Tx >> paths, TSO segment expansion, Tx burst mode reporting, queue statistics, >> pseudo-header checksum, per-tile cmdq ops, SP560 NIC support, and device >> kvargs tunables. >> >> Thanks to Stephen Hemminger for the review. The review comments are addressed >> in this version, and the changes are listed below. >> >> v5: > >Lots of open items from AI review > >Subject: Re: [PATCH v5 00/28] net/hinic3: fixes and new features > >Applies to main (release notes conflict aside) and every commit builds >with -Dwerror=true. The v4 items are addressed: tx_pkt_prepare is >installed in 09, and the SP560 CEQ EN bit is explained. > >There is still no cover letter or changelog. Please add one; at 28 >patches it is hard to see what changed between versions. [Jiacheng] Thanks for the note. The 00/28 cover letter was sent with the series (Message-ID [email protected]) and includes the v5 changelog; it is about 10.8 KB because it also carries the v2-v5 history, so it may have been truncated on the way. I have repeated the v5 changelog below for convenience. Changes in v5: - Register hinic3_prep_pkts() in the pseudo-header checksum patch instead of the compact CQE Rx patch. - Mention only SP620 in that commit message. - Document the RQ CEQ EN bit and the INTR field for SW compact CQE. - Drop the per-packet ERR log for a zero tso_segsz. - Keep log and error messages on a single line. - Check the non-TSO packet length before the early SGE-count return. - Scale the coalescing parameters by their unit in the log. - Clear the stale Rx hardware CI write-back memory on queue reset. > >Errors >====== > >[PATCH 18/28] net/hinic3: update RSS hash type configuration > >nic_dev->rss_type is set, but never cleared: > > cqe_info->rss_type = (rxq->nic_dev->rss_type.val != 0) ? 1 : 0; > >hinic3_remove_rxtx_configure() sets rss_state to DISABLE and leaves >rss_type alone, and so does the init_rss_fail path. After stop and >reconfigure without RSS (mq_mode NONE, or a single queue), the compact >CQE path still sets RTE_MBUF_F_RX_RSS_HASH on every mbuf, with a >garbage hash. Clear rss_type.val wherever rss_state goes to DISABLE. >Better, cache an rss_en flag in struct hinic3_rxq at queue start, so >the fast path does not dereference nic_dev for every packet. [Jiacheng] Accepted. The fast path now uses rxq->rss_en, cached when the queue starts, cleared on queue stop and reset. rss_type.val is cleared on every path that disables RSS (remove_rxtx_configure and the init RSS failure path), so no stale hash leaks into the compact CQE path. > > >[PATCH 19/28] net/hinic3: refactor flow rule parsing > >The new IP-in-IP pattern accepts an outer IPv6 with an inner L3: > > case RTE_FLOW_ITEM_TYPE_IPV4: > case RTE_FLOW_ITEM_TYPE_IPV6: > /* Ip-in-ip: a second L3 with an optional L4 (or ANY). */ > hinic3_flow_take(item, next); > >The parser then sets outer_ip_type = IPV6 and ip_type = IPV4/IPV6. >hinic3_fdir_tcam_ipv6_vxlan_init() programs addresses only when >ip_type == HINIC3_FDIR_IP_TYPE_ANY, so the rule is accepted with no >address match and steers all IPv6 IP-in-IP traffic. classify_inner() >already rejects outer IPv6 plus inner L3 for this reason. Do the same >here: > > case RTE_FLOW_ITEM_TYPE_IPV4: > case RTE_FLOW_ITEM_TYPE_IPV6: > if (ip_type == RTE_FLOW_ITEM_TYPE_IPV6) > return HINIC3_FLOW_KIND_INVALID; > hinic3_flow_take(item, next); [Jiacheng] Accepted. hinic3_flow_classify_ip() returns HINIC3_FLOW_KIND_INVALID when a second L3 follows an outer IPv6, so an outer IPv6 plus inner L3 rule is rejected by the parser. > > >[PATCH 24/28] net/hinic3: expand TSO from 127 to 255 segments > >A Tx WQEBB is 16 bytes, one per SGE, so a 255-SGE TSO packet needs >wqebb_cnt = sge_cnt + 1 = 256. hinic3_get_sq_free_wqebbs() never >returns more than q_depth - 1, and setup accepts depths 128 and 256. >On those rings the packet never fits: the burst hits > > if (wqe_info.wqebb_cnt > free_wqebb_cnt) { > txq->txq_stats.tx_busy += (nb_pkts - nb_tx); > goto end; > >every time, and the queue stalls at that packet forever. Before this >patch only depth 128 was affected. In hinic3_get_tx_offload(), reject >(free and count) a packet whose WQEBB count exceeds q_depth - 1, or >raise the Tx minimum depth to 512 in both setup and tx_desc_lim. Also >set tx_desc_lim.nb_seg_max and nb_mtu_seg_max. [Jiacheng] Accepted. hinic3_get_tx_offload() rejects the packet when wqe_info->sge_cnt >= q_depth - 1, i.e. when the WQEBB count cannot fit in a ring that never reports more than q_depth - 1 free WQEBBs; the packet is freed and counted in offload_errors. tx_desc_lim.nb_seg_max and nb_mtu_seg_max are set to HINIC3_TSO_PKT_MAX_SGE. An extended WQE needs one WQEBB per SGE plus one for the task section, so the usable SGE count is q_depth - 2: 126 segments at depth 128, 254 at depth 256, and the hardware maximum of 255 is reachable only from depth 512 up. nb_seg_max stays 255 (the hardware limit) and the Tx minimum depth stays 128; a packet that does not fit is rejected and counted rather than stalling the queue. > > >Warnings >======== > >[PATCH 03/28] net/hinic3: fix TCAM index leak on ethertype flow set > >Wrong Fixes tag. efa3b9b36fc7 has no TCAM allocation in >hinic3_set_fdir_ethertype_filter(); that came with b2d4933b80ec, >which first shipped in 26.03. Not in 25.11, so drop Cc: stable. > > Fixes: b2d4933b80ec ("net/hinic3: support htn FDIR for new SPx series NIC") [Jiacheng] Accepted. > > >[PATCH 05/28] net/hinic3: fix vport enable null dereference > >Same problem. The nic_dev dereference in hinic3_set_vport_enable() was >added by 9204b6b7bb1a (26.03), not de490215f74a. Drop Cc: stable. > > Fixes: 9204b6b7bb1a ("net/hinic3: split new/old command queue operations") > >The new check cannot fire: hinic3_func_init() sets hwdev->dev_handle >right after allocating hwdev, before any vport command. > > nic_dev = hwdev->dev_handle; > if (!nic_dev) > return -EINVAL; > >The real fix is moving the dereference below the hwdev check. Drop >the nic_dev check and fix the commit message. [Jiacheng] Accepted. > > >[PATCH 09/28] net/hinic3: fix pseudo-header checksum calculation > >hinic3_prep_pkts() writes headers through rte_pktmbuf_mtod_offset() >without checking that they are in the first segment. A multi-segment >mbuf with split headers gets an out-of-bounds write. Do what >rte_net_intel_cksum_flags_prepare() does: > > if (unlikely(rte_pktmbuf_data_len(mbuf) < > inner_l3_offset + mbuf->l3_len + mbuf->l4_len)) { > rte_errno = ENOTSUP; > return i; > } > >The shared mbuf check is too broad: > > if (unlikely(rte_mbuf_refcnt_read(mbuf) > 1 || RTE_MBUF_CLONED(mbuf))) { > >HINIC3_TX_OFFLOAD_MASK includes VLAN and IP checksum, which write >nothing. A cloned mbuf asking only for VLAN insert now fails >tx_prepare on SP620, which breaks flooding and multicast. Do the check >only when ol_flags has HINIC3_PKT_TX_L4_MASK, HINIC3_PKT_TX_TCP_SEG or >HINIC3_PKT_TX_OUTER_UDP_CKSUM. > >The patch also starts advertising RTE_ETH_TX_OFFLOAD_OUTER_UDP_CKSUM. >That is a new capability in a stable-tagged fix, and the commit >message does not mention it. Move it to its own patch and add "Outer >L3 checksum" and "Outer L4 checksum" to hinic3.ini. [Jiacheng] Accepted for the contiguity check (it now mirrors rte_net_intel_cksum_flags_prepare()) and for narrowing the shared-mbuf check to the L4/TSO/outer-UDP flags. The OUTER_UDP_CKSUM advertisement was moved out of this fix into its own offload patch. For the ini rows: the feature matrix has no separate outer checksum rows, and the drivers that advertise OUTER_IPV4_CKSUM/OUTER_UDP_CKSUM (hns3, bnxt, enic) only mark "L3/L4 checksum offload". Should I add "Outer L3 checksum" / "Outer L4 checksum" to default.ini and list them in hinic3.ini? > > >[PATCH 10/28] net/hinic3: add SP230 Rx queue interrupt handling > >The comment and commit message say the SP230 vectors are masked: > > if (HINIC3_IS_SP230_NIC(nic_dev)) > return hinic3_sp230_mask_rxq_msix(dev); > >hinic3_sp230_mask_rxq_msix() only calls hinic3_init_rxq_msix_attr(), >which sets pending limit, coalesce timer and resend timer. Nothing >calls hinic3_set_msix_state(..., HINIC3_MSIX_DISABLE) or sets the >auto-mask state. Either mask the vectors, or rename the function and >fix the text to say coalescing only. [Jiacheng] Accepted. > > >[PATCH 13/28] net/hinic3: add FEC mode get/set and capability support > > if (num < HINIC3_FEC_CAPA_NUM_PER_SPEED) { > ... > return -EINVAL; > >rte_eth_fec_get_capability() requires a too-small array to return the >needed count, not an error. Return HINIC3_FEC_CAPA_NUM_PER_SPEED. > >When firmware reports HINIC3_PORT_FEC_NOT_SET, fec_get returns 0, >which is not a valid mode mask. Since set rejects AUTO, once a mode is >forced the user can't go back to the default. Map NOT_SET to and from >RTE_ETH_FEC_MODE_CAPA_MASK(AUTO), and accept AUTO in >hinic3_fec_param_valid(). > >The get/set helpers do not check out_size, unlike the rest of the mgmt >helpers in that file: > > if (fec_msg.head.status != 0 || err) { > >Use "if (err || !out_size || fec_msg.head.status)". [Jiacheng] Accepted for the needed-count return and the out_size checks in all FEC mgmt helpers. For AUTO I kept it read-only. A forced FEC mode can only be configured after auto-negotiation is disabled with the hinicadm5 tool; while auto-negotiation is enabled the firmware rejects the SET request (mgmt status 0x10), and the PMD cannot toggle the auto-negotiation state. So NOT_SET is mapped to AUTO in fec_get() and in the reported capabilities, AUTO is masked out of the advertised capabilities, and fec_set() rejects it. To configure rs/base/nofec/llrs, disable auto-negotiation with hinicadm5 first; to return to auto, re-enable it there. An application that saves the mode from fec_get() and restores it later will get -EINVAL for AUTO, which is intentional. > > >[PATCH 14/28] net/hinic3: add mbox message recording and debug dump > >eth_dev_priv_dump is also registered for secondary processes, for >example dpdk-proc-info --show-port-private, but the dump does > > pci_dev = hwdev->pci_dev; > >hwdev is in shared memory, but pci_dev points into the primary's heap, >so a secondary reads a bad pointer. Pass RTE_ETH_DEV_TO_PCI(dev) down >from hinic3_eth_dev_priv_dump(). [Jiacheng] Accepted. > > >[PATCH 15/28] net/hinic3: add COS mask and COS map support > >The patch drops the existing COS clamps, and the commit message does >not say so: > > - *cos_id = vf_dcb.state.default_cos % HINIC3_COS_NUM_MAX_HTN; > + *cos_id = vf_dcb.state.default_cos; > >The PF HTN limit of 4 is removed as well. cos_map is only applied on >SP230, so SP620 VFs can now get COS 4-7. Keep the clamp on paths that >don't use cos_map, or explain in the commit message why it is safe. [Jiacheng] The commit message will explain it: the four-COS clamp existed for the HTN tile, where the COS may be split between the NIC and RoCE; cos_map[] folds any COS outside the firmware cos_mask into default_cos, so the clamp is no longer needed on SP230. On SP620 and SP560 all eight COS belong to the NIC, so the clamp is dropped there as well. > > >[PATCH 16/28] net/hinic3: add VF link state and promiscuous support > >dev_set_link_down only disables the vport. The next PF link-up event >goes through hinic3_set_vport_state(), which re-enables the vport >whenever HINIC3_DEV_START is set: > > if (hinic3_get_bit(HINIC3_DEV_START, &nic_dev->dev_status)) > err = hinic3_set_vport_enable(hwdev, true); > >That sets vf_valid_status back to 1 and reports the link up again, so >an administrative link down does not stick. Keep a separate admin-down >flag, set only by set_link_up/down, and check it in >hinic3_set_vport_state() and in the link status calculation. [Jiacheng] Accepted. > > >[PATCH 19/28] net/hinic3: refactor flow rule parsing > >The commit message says IPv4 and VXLAN keys no longer set the >vlan_flag mask. The lines removed were duplicates. >hinic3_fdir_tcam_notunnel_init() and >hinic3_fdir_tcam_vxlan_geneve_init() still do > > tcam_key->key_mask.vlan_flag = 1; > >so those rules still match only untagged packets. Fix the commit >message, or the code if the change was intended. [Jiacheng] The vlan_flag field is only used by bifur, which this driver does not support, so the setters are dropped. > > >[PATCH 20/28] net/hinic3: add SP560 NIC type identification and > handling > >The new HTN ops change the SP230 queue context values. Before this >patch, SP230 used the common values: drop OFF 0, prefetch 1/6/256, no >PFH_TH. Now it gets: > > #define HINIC3_DEFAULT_DROP_THD_OFF 0xFFFF > #define SQ_PREFETCH_MAX 5 > #define SQ_PREFETCH_MIN 4 > #define SQ_PREFETCH_THRESHOLD 48 > ... > #define RQ_PFH_TH 7 > >The commit message only talks about SP560. Either keep the old SP230 >values, or put the change in a separate patch with the reason. [Jiacheng] This patch also switches the HTN tile (SP230) queue context from the generic values inherited from the shared code to the tile-recommended ones: drop threshold off 0xFFFF, SQ prefetch 4/5/48, RQ prefetch 2/4/32 and PFH_TH 7. The generic values were not tuned for the HTN tile. This will be described in the commit message. > > >[PATCH 23/28] net/hinic3: ensure queue depth and state consistency > >The depth cache is cleared only in dev_configure: > > nic_dev->rxq_depth = 0; > nic_dev->txq_depth = 0; > >ethdev allows queue setup again while stopped, without reconfigure. >testpmd "port config all rxd 2048" does this. Queue 0 then fails with >-EINVAL against the stale depth. Instead of the cache, compare against >q_depth of the other non-NULL dev->data->{rx,tx}_queues[]. > >dev_stop marks queues STOPPED in ethdev but leaves txq->status at >START. A deferred-start queue started at runtime keeps transmitting >after stop/start, and rte_eth_dev_tx_queue_stop() then returns early >without calling the driver. Call HINIC3_SET_TXQ_STOPPED() in the same >loop. [Jiacheng] The cache is now reset in dev_configure() and in dev_stop(), and dev_start() validates that every configured queue is present and shares queue 0's depth. A reconfiguration while stopped, including "port config all rxd 2048" without a new dev_configure(), therefore works; a partial reconfiguration is refused at dev_start() instead of running with rings that do not match the root context. HINIC3_SET_TXQ_STOPPED() is called in the dev_stop loop. > > >[PATCH 24/28], [PATCH 25/28] Tx path logging > >These are new per-packet logs in the Tx path, at WARNING and ERR: > > PMD_DRV_LOG(WARNING, "illegal pkt, payload offset (%u) > data len (%u).", > PMD_DRV_LOG(WARNING, "TSO payload over hardware limit."); > PMD_DRV_LOG(ERR, "IPinIP not support TSO"); > PMD_DRV_LOG(ERR, "VXLAN_GPE not support TSO"); > >A bad packet stream floods the log, and the drops are already counted >in offload_errors. Remove them, or use a rate-limited data path log. [Jiacheng] Accepted. > > >[PATCH 25/28] net/hinic3: split Tx burst function for compact CQE > support > >hinic3_mbuf_dma_map_sge() is shared by both bursts and does this for >every segment: > > if (txq->nic_dev->nic_type == NIC_SP620) > wqe_desc->queue_info = 0; > >In the compact burst, a single-WQEBB WQE keeps its task in >hdr->queue_info, and hinic3_set_tx_offload_compact_cqe() has already >written it. On an SP620 that reports NIC_F_TX_WQE_COMPACT_TASK, the >checksum and VLAN offload are wiped. The commit message says the burst >is chosen by feature bit, not NIC model, so this check is inconsistent. >Do the zeroing in hinic3_xmit_pkts() only, for SQ_WQE_COMPACT_TYPE, >once per packet. > >The compact burst declares wqe_info once per burst: > > struct hinic3_wqe_info wqe_info = { 0 }; > >hinic3_set_tx_offload_compact_cqe() sets vlan_valid, tso, mss and >payload_offset but never clears them. A VLAN or TSO packet leaks >those values into later packets in the same burst, for example a >plain checksum packet sent as TSO with a stale MSS. The old combined >burst had the same bug, and the normal path is fixed here, but the new >compact path copies it. Clear wqe_info.offload_info and >wqe_info.queue_info for each packet. [Jiacheng] Accepted. The zeroing moved to hinic3_xmit_pkts() and is done once per packet only for SQ_WQE_COMPACT_TYPE; hinic3_mbuf_dma_map_sge() no longer writes queue_info. The compact burst clears wqe_info.queue_info and wqe_info.offload_info for every packet in hinic3_get_tx_offload(), so VLAN/TSO state cannot leak into the next packet. > > >[PATCH 27/28] net/hinic3: add Tx burst mode get > > (nic_dev->config.rx_cqe_compact_en == HINIC3_RX_CQE_COMPACT_EN) ? > "compact CQE" : "scalar", > >Tx burst selection uses HINIC3_SUPPORT_TX_WQE_COMPACT_TASK(), not the >Rx compact CQE setting, so this can name the wrong function. Use > > dev->tx_pkt_burst == hinic3_xmit_pkts_compact_cqe [Jiacheng] Accepted. > >Info >==== > >[PATCH 09/28] Tunnel type is an enumerated field, not a bitmask: > > if ((ol_flags & HINIC3_PKT_TX_TUNNEL_IPIP) == HINIC3_PKT_TX_TUNNEL_IPIP) { > >This also matches GTP (0x7) and other values. Use >(ol_flags & HINIC3_PKT_TX_TUNNEL_MASK) == HINIC3_PKT_TX_TUNNEL_IPIP. >The VXLAN and GENEVE support tests have the same problem. [Jiacheng] Accepted. > >[PATCH 11/28] The tunnel TSO gating fixed here came from c1537bf6c033 >("net/hinic3: support Compact CQE in init"). Add a second Fixes tag. [Jiacheng] Accepted. > >[PATCH 12/28] rx_discard_phy is counted and named on every NIC type >but only filled in on SP620, so SP230/SP560 show a counter that is >always 0. Use the same HINIC3_IS_SP620_NIC() condition in calc_num, >get_names and get. [Jiacheng] Accepted. > >[PATCH 21/28] rx_cqe_coalesce_num and rx_cqe_timer_loop only take >effect with HW compact CQE. hinic3.rst should say so. [Jiacheng] Accepted. hinic3.rst now says the two RX CQE coalescing parameters only take effect with HW compact CQE and are ignored with SW compact and split CQE. > >[PATCH 22/28] HINIC3_RX_COMPACT_L3_L4_CSUM_ERROR goes to the default >case and keeps value 1, which is HINIC3_RX_CSUM_IP_CSUM_ERR. A bad >L4 checksum is then reported as RTE_MBUF_F_RX_IP_CKSUM_BAD. Also, >the ptype table produces RTE_PTYPE_INNER_L4_FRAG, _ICMP and _ESP, >which are missing from dev_supported_ptypes_get. [Jiacheng] Fixed: HINIC3_RX_COMPACT_L3_L4_CSUM_ERROR maps to HINIC3_RX_CSUM_LAYER_UNKNOWN_ERR instead of the default value. The inner FRAG/ICMP/ESP encodings are reserved firmware values that the current hardware does not produce; those cases are not emitted, so the table and dev_supported_ptypes_get() stay in sync. > >[PATCH 25/28] rte_prefetch0(tx_pkts[0]) runs before the loop, so with >nb_pkts == 0 it reads past the array. [Jiacheng] Accepted. > >Pre-existing, not introduced by this series: >- hinic3_dev_stats_get() does > dev->data->rx_mbuf_alloc_failed += rxq->rxq_stats.rx_nombuf; > on every call, so rx_nombuf grows each time stats are read. >- hinic3_start_rq()/hinic3_stop_rq() call hinic3_set_vport_enable(), > a blocking mgmt message, while holding the queue_list_lock spinlock. [Jiacheng] Both are pre-existing. I will send them as separate patches rather than mixing them into this series, together with the unaligned mbox-header read. Thanks again for the review. Regards, Jiacheng
