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