回复: [V9 00/17] add-hinic3-PMD-driver

["wangfeifei (J)" <wangfeifei40@huawei.com>]

> -----邮件原件-----
> 发件人: Stephen Hemminger <stephen@networkplumber.org>
> 发送时间: 2025年9月10日 6:43
> 收件人: Feifei Wang <wff_light@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_light@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;
>