> -----邮件原件-----
> 发件人: Stephen Hemminger <step...@networkplumber.org>
> 发送时间: 2025年9月10日 6:43
> 收件人: Feifei Wang <wff_li...@vip.163.com>
> 抄送: dev@dpdk.org
> 主题: Re: [V9 00/17] add-hinic3-PMD-driver
>
> On Tue, 9 Sep 2025 15:31:28 +0800
> Feifei Wang <wff_li...@vip.163.com> wrote:
>
> > The hinic3 PMD (**librte_net_hinic3**) provides poll mode driver
> > support for 25Gbps/100Gbps/200Gbps Huawei SPx series Network Adapters.
> >
> > v9:
> > -resolve type conflict issue
> >
> > v8:
> > -resolve CI compile error
> > -modify mbox section
> >
> > v7:
> > -remove unused-functions
> >
> > v6:
> > -modify based on community comments
> > -remove mml files
> >
> > v5:
> > -fix build err
> >
> > v4:
> > -solve patch application conflict issue
> >
> > v3:
> > -fix checkpatchs errors issue
> >
> > v2:
> > -modify CI compile errors
> >
> > v1:
> > -modify community comments and compile issues -remove the compilation
> > flags in the meson build -remove (void) cur_link_machine_state -remove
> > '*** BLURE HERE ***' in cover letter
> >
> > .mailmap | 4 +-
> > MAINTAINERS | 6 +
> > doc/guides/nics/features/hinic3.ini | 60 +
> > doc/guides/nics/hinic3.rst | 47 +
> > doc/guides/nics/index.rst | 1 +
> > doc/guides/rel_notes/release_25_11.rst | 4 +
> > drivers/net/hinic3/base/hinic3_cmd.h | 156 +
> > drivers/net/hinic3/base/hinic3_cmdq.c | 972 +++++
> > drivers/net/hinic3/base/hinic3_cmdq.h | 230 ++
> > drivers/net/hinic3/base/hinic3_compat.h | 144 +
> > drivers/net/hinic3/base/hinic3_csr.h | 108 +
> > drivers/net/hinic3/base/hinic3_eqs.c | 710 ++++
> > drivers/net/hinic3/base/hinic3_eqs.h | 98 +
> > drivers/net/hinic3/base/hinic3_hw_cfg.c | 194 +
> > drivers/net/hinic3/base/hinic3_hw_cfg.h | 117 +
> > drivers/net/hinic3/base/hinic3_hw_comm.c | 449 +++
> > drivers/net/hinic3/base/hinic3_hw_comm.h | 365 ++
> > drivers/net/hinic3/base/hinic3_hwdev.c | 558 +++
> > drivers/net/hinic3/base/hinic3_hwdev.h | 183 +
> > drivers/net/hinic3/base/hinic3_hwif.c | 741 ++++
> > drivers/net/hinic3/base/hinic3_hwif.h | 144 +
> > drivers/net/hinic3/base/hinic3_mbox.c | 1225 +++++++
> > drivers/net/hinic3/base/hinic3_mbox.h | 181 +
> > drivers/net/hinic3/base/hinic3_mgmt.c | 355 ++
> > drivers/net/hinic3/base/hinic3_mgmt.h | 112 +
> > drivers/net/hinic3/base/hinic3_nic_cfg.c | 1795 ++++++++++
> > drivers/net/hinic3/base/hinic3_nic_cfg.h | 1530 ++++++++
> > drivers/net/hinic3/base/hinic3_nic_event.c | 407 +++
> > drivers/net/hinic3/base/hinic3_nic_event.h | 38 +
> > drivers/net/hinic3/base/hinic3_wq.c | 140 +
> > drivers/net/hinic3/base/hinic3_wq.h | 109 +
> > drivers/net/hinic3/base/meson.build | 50 +
> > drivers/net/hinic3/hinic3_ethdev.c | 3782
> ++++++++++++++++++++
> > drivers/net/hinic3/hinic3_ethdev.h | 164 +
> > drivers/net/hinic3/hinic3_fdir.c | 1379 +++++++
> > drivers/net/hinic3/hinic3_fdir.h | 398 ++
> > drivers/net/hinic3/hinic3_flow.c | 1501 ++++++++
> > drivers/net/hinic3/hinic3_flow.h | 196 +
> > drivers/net/hinic3/hinic3_nic_io.c | 806 +++++
> > drivers/net/hinic3/hinic3_nic_io.h | 171 +
> > drivers/net/hinic3/hinic3_rx.c | 1067 ++++++
> > drivers/net/hinic3/hinic3_rx.h | 353 ++
> > drivers/net/hinic3/hinic3_tx.c | 1024 ++++++
> > drivers/net/hinic3/hinic3_tx.h | 313 ++
> > drivers/net/hinic3/meson.build | 31 +
> > drivers/net/meson.build | 1 +
> > 46 files changed, 22418 insertions(+), 1 deletion(-) create mode
> > 100644 doc/guides/nics/features/hinic3.ini
> > create mode 100644 doc/guides/nics/hinic3.rst create mode 100644
> > drivers/net/hinic3/base/hinic3_cmd.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_cmdq.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_cmdq.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_compat.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_csr.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_eqs.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_eqs.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_hw_cfg.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_hw_cfg.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_hw_comm.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_hw_comm.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_hwdev.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_hwdev.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_hwif.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_hwif.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_mbox.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_mbox.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_mgmt.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_mgmt.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_nic_cfg.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_nic_cfg.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_nic_event.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_nic_event.h
> > create mode 100644 drivers/net/hinic3/base/hinic3_wq.c
> > create mode 100644 drivers/net/hinic3/base/hinic3_wq.h
> > create mode 100644 drivers/net/hinic3/base/meson.build
> > create mode 100644 drivers/net/hinic3/hinic3_ethdev.c
> > create mode 100644 drivers/net/hinic3/hinic3_ethdev.h
> > create mode 100644 drivers/net/hinic3/hinic3_fdir.c create mode
> > 100644 drivers/net/hinic3/hinic3_fdir.h create mode 100644
> > drivers/net/hinic3/hinic3_flow.c create mode 100644
> > drivers/net/hinic3/hinic3_flow.h create mode 100644
> > drivers/net/hinic3/hinic3_nic_io.c
> > create mode 100644 drivers/net/hinic3/hinic3_nic_io.h
> > create mode 100644 drivers/net/hinic3/hinic3_rx.c create mode 100644
> > drivers/net/hinic3/hinic3_rx.h create mode 100644
> > drivers/net/hinic3/hinic3_tx.c create mode 100644
> > drivers/net/hinic3/hinic3_tx.h create mode 100644
> > drivers/net/hinic3/meson.build
> >
>
[Feifei] Thanks for your timely reply and comments.
> Most of this driver is fine, a few more things.
>
>
> 1. Since DPDK applications are often statically linked, drivers should make
> sure
> that all global symbols have the same prefix. For this driver I see:
>
> $ nm ./build/drivers/librte_net_hinic3.a | grep ' [TDB] ' | grep -v hinic3
> 00000000000000e0 T cfg_mbx_vf_proc_msg
> 0000000000000040 T pf_handle_mgmt_comm_event
> 0000000000000030 T vf_handle_pf_comm_mbox
> 0000000000000310 T get_port_info
>
[Feifei] Accept.
> 2. Don't use rte_memcpy() for simple fixed size copies. For this driver
> replace
> all use of rte_memcpy with memcpy which has more checking
[Feifei] Accept.
>
> 3. Why is a cast needed here?
> rx_free_thresh = (uint16_t)((rx_conf->rx_free_thresh)
> ? rx_conf->rx_free_thresh
> : HINIC3_DEFAULT_RX_FREE_THRESH);
> Probably just need to add u here:
> :#define HINIC3_DEFAULT_RX_FREE_THRESH 32u
> Casts are the enemy of compiler checking.
>
[Feifei] Ok, Accept.
> 4. Prefer that messages not be broken across lines, it makes it harder
> to search source for where error message is. Don't worry if format line
> is
> a little long.
>
> Instead:
> if (err || out_param != 0) {
> PMD_DRV_LOG(ERR,
> "Set SQ ctxts failed, err: %d, out_param:
> %" PRIu64,
> err, out_param);
>
> err = -EFAULT;
> break;
> }
> Same extra line breaks elsewhere.
>
[Feifei] Accept.
> 5. Please fix these warnings, instead of stifling them.
>
> # The driver runs only on arch64 machine, remove 32bit warnings if not
> dpdk_conf.get('RTE_ARCH_64')
> extra_flags += [
> '-Wno-int-to-pointer-cast',
> '-Wno-pointer-to-int-cast',
> ]
> endif
>
[Feifei] Ok, we will delete these extra_flags, and fix new warning.
>
> 6. Having to add meson instructions for static library() seems like
> something
> that could be fixed elsewhere. Why is this in base/meson.build.
>
[Feifei] Sorry, if you means we can not add static library build instructions
in base meson file?
> 7. The code in hinic3_dev_rx_queue_start() is checking that rq_id <
> dev->data->nb_rx_queues.
> But this condition is already checked in ethdev layer via
> eth_dev_validate_rx_queue.
> Same useless check in hinc3_dev_rx_queue_stop() and
> hnic3_dev_tx_queue_stop().
> And in hinc3_dev_rx_queue_intr_enable/disable
>
[Feifei] Thanks for help us find this. We will check and delete this useless
check in our pmd.
> 8. Don't stub out rx_queue_count, rx_descriptor_status, and
> tx_descriptor_status
> If driver doesn't implement these, leave it NULL. Ethdev fills in a
> dummy
> operation for you.
>
[Feifei] Ok.
> 9. Don't log things exposed by API at INFO level.
> For example, probe or set_lro. Put at DEBUG level.
>
[Feifei]Accept.
> 10. Driver features says it has "CRC offload". The documentation is
> confusing
> but driver must support KEEP_CRC for this.
>
[Feifei]Ok, we will add keep crc offload feature in the future. So in our first
version,
We delete it.
> 11. Driver says it supports ptypes, but it does not (see ptypes_get).
>
[Feifei]Same as above, we add this in our next version, this version we delete
this.
> 12. Why does hinic3_xstats_cal_num return a signed value? Looks like it
> should
> be
> unsigned. Then can take off cast in xstats_get().
>
[Feifei] Accept.
> 13. When handling regular stats_get() best to count all queues even if queue
> is larger than RTE_ETHDEV_QUEUE_STATS_CNTRS. Just don't fill in
> per-queue value for those
> queues. Discards don't count as input packets. Also extra paren here.
>
[Feifei] Accept.
> 13, Why does reading stats clear rx_mbuf_alloc_failed? That should be
> cleared
> in reset.
>
> Something like this maybe:
> diff --git a/drivers/net/hinic3/hinic3_ethdev.c
> b/drivers/net/hinic3/hinic3_ethdev.c
> index b83e632d54..75d7ac3ca2 100644
> --- a/drivers/net/hinic3/hinic3_ethdev.c
> +++ b/drivers/net/hinic3/hinic3_ethdev.c
> @@ -2644,9 +2644,7 @@ hinic3_dev_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats) {
> struct hinic3_nic_dev *nic_dev =
> HINIC3_ETH_DEV_TO_PRIVATE_NIC_DEV(dev);
> struct hinic3_vport_stats vport_stats;
> - struct hinic3_rxq *rxq = NULL;
> - struct hinic3_txq *txq = NULL;
> - int i, err, q_num;
> + int err;
> uint64_t rx_discards_pmd = 0;
>
> err = hinic3_get_vport_stats(nic_dev->hwdev, &vport_stats); @@ -2656,25
> +2654,23 @@ hinic3_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> *stats)
> return err;
> }
>
> - dev->data->rx_mbuf_alloc_failed = 0;
> -
> /* Rx queue stats. */
> - q_num = (nic_dev->num_rqs < RTE_ETHDEV_QUEUE_STAT_CNTRS)
> - ? nic_dev->num_rqs
> - : RTE_ETHDEV_QUEUE_STAT_CNTRS;
> - for (i = 0; i < q_num; i++) {
> - rxq = nic_dev->rxqs[i];
> + for (unsigned i = 0; i < nic_dev->num_rqs; i++) {
> + struct hinic3_rxq *rxq = nic_dev->rxqs[i];
> +
> #ifdef HINIC3_XSTAT_MBUF_USE
> rxq->rxq_stats.rx_left_mbuf_bytes =
> rxq->rxq_stats.rx_alloc_mbuf_bytes -
> rxq->rxq_stats.rx_free_mbuf_bytes;
> #endif
> rxq->rxq_stats.errors = rxq->rxq_stats.csum_errors +
> - rxq->rxq_stats.other_errors;
> + rxq->rxq_stats.other_errors;
>
> - stats->q_ipackets[i] = rxq->rxq_stats.packets;
> - stats->q_ibytes[i] = rxq->rxq_stats.bytes;
> - stats->q_errors[i] = rxq->rxq_stats.errors;
> + if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> + stats->q_ipackets[i] = rxq->rxq_stats.packets;
> + stats->q_ibytes[i] = rxq->rxq_stats.bytes;
> + stats->q_errors[i] = rxq->rxq_stats.errors;
> + }
>
> stats->ierrors += rxq->rxq_stats.errors;
> rx_discards_pmd += rxq->rxq_stats.dropped; @@ -2682,15 +2678,14
> @@ hinic3_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> }
>
> /* Tx queue stats. */
> - q_num = (nic_dev->num_sqs < RTE_ETHDEV_QUEUE_STAT_CNTRS)
> - ? nic_dev->num_sqs
> - : RTE_ETHDEV_QUEUE_STAT_CNTRS;
> - for (i = 0; i < q_num; i++) {
> - txq = nic_dev->txqs[i];
> - stats->q_opackets[i] = txq->txq_stats.packets;
> - stats->q_obytes[i] = txq->txq_stats.bytes;
> - stats->oerrors += (txq->txq_stats.tx_busy +
> - txq->txq_stats.offload_errors);
> + for (unsigned int i = 0; i < nic_dev->num_sqs; i++) {
> + struct hinic3_txq *txq = nic_dev->txqs[i];
> +
> + stats->oerrors += txq->txq_stats.tx_busy +
> txq->txq_stats.offload_errors;
> + if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> + stats->q_opackets[i] = txq->txq_stats.packets;
> + stats->q_obytes[i] = txq->txq_stats.bytes;
> + }
> }
>
> /* Vport stats. */
> @@ -2698,22 +2693,21 @@ hinic3_dev_stats_get(struct rte_eth_dev *dev,
> struct rte_eth_stats *stats)
>
> stats->imissed = vport_stats.rx_discard_vport + rx_discards_pmd;
>
> - stats->ipackets =
> - (vport_stats.rx_unicast_pkts_vport +
> - vport_stats.rx_multicast_pkts_vport +
> - vport_stats.rx_broadcast_pkts_vport - rx_discards_pmd);
> + stats->ipackets = vport_stats.rx_unicast_pkts_vport +
> + vport_stats.rx_multicast_pkts_vport +
> + vport_stats.rx_broadcast_pkts_vport;
>
> - stats->opackets = (vport_stats.tx_unicast_pkts_vport +
> - vport_stats.tx_multicast_pkts_vport +
> - vport_stats.tx_broadcast_pkts_vport);
> + stats->opackets = vport_stats.tx_unicast_pkts_vport +
> + vport_stats.tx_multicast_pkts_vport +
> + vport_stats.tx_broadcast_pkts_vport;
>
> - stats->ibytes = (vport_stats.rx_unicast_bytes_vport +
> - vport_stats.rx_multicast_bytes_vport +
> - vport_stats.rx_broadcast_bytes_vport);
> + stats->ibytes = vport_stats.rx_unicast_bytes_vport +
> + vport_stats.rx_multicast_bytes_vport +
> + vport_stats.rx_broadcast_bytes_vport;
>
> - stats->obytes = (vport_stats.tx_unicast_bytes_vport +
> - vport_stats.tx_multicast_bytes_vport +
> - vport_stats.tx_broadcast_bytes_vport);
> + stats->obytes = vport_stats.tx_unicast_bytes_vport +
> + vport_stats.tx_multicast_bytes_vport +
> + vport_stats.tx_broadcast_bytes_vport;
> return 0;
> }
>
> @@ -2735,6 +2729,8 @@ hinic3_dev_stats_reset(struct rte_eth_dev *dev)
> int qid;
> int err;
>
> + dev->data->rx_mbuf_alloc_failed = 0;
> +
> err = hinic3_clear_vport_stats(nic_dev->hwdev);
> if (err)
> return err;
>
