Hi Stephen,
Thanks for the review. Points will be addressed in v4: the series will be
reordered so that every commit builds, the SP230 masking will be split out
of the interrupt setup, the copy path will start every segment copy at its
own source offset, the IPv6 extension-header walk will be dropped, and the
copy pool will be capped. One warning I still did not take, the compact
CQE Rx interrupt one: my v3 reply was based on a misreading of a firmware
comment; after re-reading the code and testing it, I believe it is
supported, and I explain why below.
At 2026-10-20 00:52:41, "Stephen Hemminger" <[email protected]> wrote:
>On Mon, 19 Oct 2026 23:19:02 +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.
>>
>> v3:
>> - Move the ethertype-flow TCAM index leak fix out of the flow-refactor
>> patch into its own patch with a Fixes tag and a stable Cc tag.
>> - Set burst function pointers and tx_pkt_prepare for both primary and
>> secondary processes, and select the Tx path on its own feature bit.
>> - Zero the RQ/SQ context buffers and honour dp_intr_en for the STN CEQ
>> attribute in compact CQE mode.
>> - Return the number of Tx packets consumed instead of the doorbell
>> count, and guard the Tx prefetch against overreading the array.
>> - Validate device kvargs and reject values above the hardware field
>> width; document the real defaults and units.
>> - Reject mismatched queue depths and reset saved depths in
>> dev_configure(), and size the TSO copy pool per port.
>> - Report FEC capabilities per supported speed regardless of link state.
>> - Route the private dump through fprintf() on the provided FILE*.
>> - Derive the pseudo-header L4 protocol from the offload flags and fail
>> shared or indirect mbufs; add Fixes and stable Cc tags.
>> - Drop stable Cc tags where the Fixes target is not in an LTS release
>> and remove dead code and unused definitions.
>
>Still lots of AI feedback, biggest issue is the bisect break.
>You should run semi-automated incremental builds on any large
>patch series like this. CI does that.
>
>
>Subject: Re: [PATCH 00/27] net/hinic3: fixes and new features
>
>This is v3 of the series; please mark it as such in the subject, and
>put a changelog in the cover letter so reviewers can see what changed.
>
[Jiacheng] Accepted. It is a regression on my side: the per-commit build
has been part of the series work since v1, but I never wrote it down as a
standing rule, so when the assistant's working context was compacted for v3
that step was dropped, and the version tag went with it. From v4 on both
will be written down and checked before sending: applied and built one
commit at a time on current main, and the version in every subject.
>Applies to main (26.11-rc1). Tip builds with -Dwerror=true. Most of
>the v2 comments are addressed: Tx burst return value, prefetch, kvargs
>error and bounds, secondary process burst setup, RQ/SQ context memset,
>queue depth checks, per-port copy pool, priv_dump to FILE, TCAM leak
>split out, Fixes/Cc tags.
>
>
>Errors
>======
>
>Series: bisect broken at 20 through 26.
>
>[PATCH 20/27] sets tx_pkt_prepare = hinic3_prep_pkts, which is not
>defined until 27/27, and uses hinic3_xmit_pkts_compact_cqe, which
>arrives in 22/27.
>[PATCH 21/27] uses nic_dev->txq_depth, which is added in 23/27.
>
>Every commit must build. Move 27/27 ahead of 20 (it is a Fixes: patch
>anyway), select only hinic3_xmit_pkts in 20 and add the compact
>choice in 22, and add txq_depth in 21 or move the pool sizing to 23.
>
[Jiacheng] Accepted. Reordered along the lines you described. The Fixes:
patch (pseudo-header checksum) will be 08/27, right after the NIC type
identification patch it depends on. hinic3_prep_pkts() is defined before
the Tx burst split, which installs only hinic3_xmit_pkts; the compact
choice follows in the compact CQE patch. txq_depth will land with the
queue depth patch, ahead of the copy pool sizing that reads it, so no
patch uses a field that arrives later.
>[PATCH 08/27] net/hinic3: add SP230 Rx queue interrupt handling
>
>On SP230 hinic3_init_rxq_intr() now runs the full Rx interrupt setup
>even when intr_conf.rxq is 0. That path requires
>rte_intr_cap_multiple(), so on igb_uio or uio_pci_generic dev_start
>fails with -ENOTSUP for an application that never asked for
>interrupts. It also creates eventfds and sets rxq->dp_intr_en = 1,
>so the RQ context is programmed with the CEQ interrupt enabled.
>
>Split the SP230 masking out of the interrupt setup:
>
> if (!dev->data->dev_conf.intr_conf.rxq) {
> if (HINIC3_IS_SP230_NIC(nic_dev))
> return hinic3_sp230_mask_rxq_msix(dev);
> return 0;
> }
>
>where the helper only programs coalescing/auto-mask on vectors
>i + HINIC3_RX_VEC_START, with no efd, no intr_vec and no dp_intr_en.
>
[Jiacheng] Accepted. Fixed exactly as suggested, so dev_start keeps
working on igb_uio/uio_pci_generic when Rx interrupts are disabled.
>[PATCH 21/27] net/hinic3: expand TSO from 127 to 255 segments
>
>hinic3_copy_tx_mbuf_multi() always copies from the start of the source
>segment:
>
> rte_memcpy((uint8_t *)cur_mbuf->buf_addr + copy_offset,
> (uint8_t *)mbuf->buf_addr + mbuf->data_off,
> copy_size);
>
>When a source segment spans two copy mbufs, the second copy repeats
>the first bytes of that segment, and the packet goes out corrupted.
>This was also in v2; I missed it there.
>
> uint32_t src_off = mbuf->data_len - remaining_len;
>
> rte_memcpy((uint8_t *)cur_mbuf->buf_addr + copy_offset,
> rte_pktmbuf_mtod_offset(mbuf, uint8_t *, src_off),
> copy_size);
>
[Jiacheng] Accepted. Fixed alike:
uint32_t src_off = mbuf->data_len - remaining_len;
rte_memcpy((uint8_t *)cur_mbuf->buf_addr + copy_offset,
rte_pktmbuf_mtod_offset(mbuf, uint8_t *, src_off),
copy_size);
The same path also built the copy chain without maintaining nb_segs and
pkt_len; both will be fixed in the same patch.
>Warnings
>========
>
>[PATCH 18/27] net/hinic3: add SP560 NIC type identification and
> handling
>
>The STN op still forces EN=1 (interrupt disabled) with SW compact CQE,
>and the commit marks Rx interrupt as "P". An application that sets
>intr_conf.rxq still gets success from configure and start, and then
>never gets an interrupt. Reject it:
>
> if (dev->data->dev_conf.intr_conf.rxq &&
> nic_dev->config.rx_cqe_compact_en &&
> HINIC3_SUPPORT_RX_SW_COMPACT_CQE(nic_dev))
> return -ENOTSUP;
>
[Jiacheng] My earlier reply on this point was wrong: I argued from a
firmware comment and misread what it applies to, so I re-read the code and
tested the combination instead.
On SP560 the compact CQE path does raise Rx interrupts. In compact WQE
mode the CEQ enable bit has to stay set for the CEQ attribute to take
effect at all, so EN=1 does not mean "interrupt enabled". Whether an
interrupt is actually raised is controlled separately by the CEQ
interrupt number: with Rx interrupts disabled the driver programs an
invalid vector (0x1FF) and the hardware stays silent. ceq_intr is
therefore an independent switch, and the two settings do not conflict.
SP620 has no compact path; there the interrupt is controlled by the
intr_disable bit of the RQ context directly.
So instead of returning -ENOTSUP the driver programs the combination
correctly, and hinic3.ini keeps "Rx interrupt = Y".
>[PATCH 21/27] net/hinic3: expand TSO from 127 to 255 segments
>
>The copy pool is now num_sqs * txq_depth 4 KB mbufs, created at every
>dev_start. With 16 queues of 4096 descriptors that is about 280 MB of
>hugepage memory per port, for a slow path that only fires on packets
>with too many segments. Cap it, for example:
>
> depth = nic_dev->num_sqs * RTE_MIN(nic_dev->txq_depth, 256u);
>
[Jiacheng] Accepted. The cap will be in place as you wrote it. The
HINIC3_COPY_MEMPOOL_DEPTH floor is kept, so shallow queue configurations
are unaffected, and failing to allocate a copy mbuf is already a
per-packet drop that bumps offload_errors rather than a hard failure.
>[PATCH 27/27] net/hinic3: add pseudo-header checksum calculation
>
>For IPv6 the L4 offset still comes from walking extension headers,
>while the descriptor (hinic3_get_tx_offload) uses mbuf->l3_len. When
>the two disagree, software writes the checksum in one place and the
>hardware computes it at another. mbuf->l3_len is the application's
>contract and includes extension headers; use it, and drop
>hinic3_get_ipv6_len_proto().
>
[Jiacheng] Accepted. hinic3_get_ipv6_len_proto() will be gone; the L4
offset will come from mbuf->l3_len, the same value hinic3_get_tx_offload()
uses for the descriptor.
>Info
>====
>
>[PATCH 27/27] "l4_proto = (uint32_t)proto << 24" is only correct on
>little endian. The old rte_cpu_to_be_16(proto) was fine, since the
>ones' complement sum ignores word position. Use
>rte_cpu_to_be_32(proto).
>
[Jiacheng] Accepted. rte_cpu_to_be_32() will be used.
>[PATCH 27/27] Has Cc: stable, but it depends on NIC type
>identification from 07/27. Put it right after 07, so the stable
>backport is the pair.
>
[Jiacheng] Accepted. In v4 the patch is 08/27, right after NIC type
identification from 07/27.
Thanks again for the review.
Regards,
Jiacheng
