回复: [V2 0/7] hinic3 change for support new SPx NIC

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

> -----邮件原件-----
> 发件人: Stephen Hemminger <stephen@networkplumber.org>
> 发送时间: 2026年3月16日 23:46
> 收件人: Feifei Wang <wff_light@vip.163.com>
> 抄送: dev@dpdk.org; chenyi (CY) <chenyi221@huawei.com>
> 主题: Re: [V2 0/7] hinic3 change for support new SPx NIC
> 
> On Mon, 16 Mar 2026 21:43:22 +0800
> Feifei Wang <wff_light@vip.163.com> wrote:
> 
> > From: chenyi221 <chenyi221@huawei.com>
> >
> > Change hinic3 driver to support Huawei new SPx series NIC.
> >
> > v2:
> > --fix build issues
> >
> > Feifei Wang (7):
> >   net/hinic3: add support for new SPx series NIC
> >   net/hinic3: add enhance cmdq support for new SPx series NIC
> >   net/hinic3: use different callback func to split new/old cmdq
> >     operations
> >   net/hinic3: add fun init ops to support Compact CQE
> >   net/hinic3: add rx ops to support Compact CQE
> >   net/hinic3: add tx ops to support Compact CQE
> >   net/hinic3: use different callback func to support htn fdir
> >
> >  drivers/net/hinic3/base/hinic3_cmd.h          |  80 ++-
> >  drivers/net/hinic3/base/hinic3_cmdq.c         | 370 ++++------
> >  drivers/net/hinic3/base/hinic3_cmdq.h         | 112 ++-
> >  drivers/net/hinic3/base/hinic3_cmdq_enhance.c | 111 +++
> > drivers/net/hinic3/base/hinic3_cmdq_enhance.h | 125 ++++
> >  drivers/net/hinic3/base/hinic3_csr.h          |  18 +-
> >  drivers/net/hinic3/base/hinic3_hw_comm.c      |  15 +-
> >  drivers/net/hinic3/base/hinic3_hw_comm.h      |  31 +-
> >  drivers/net/hinic3/base/hinic3_hwdev.c        |  13 +-
> >  drivers/net/hinic3/base/hinic3_hwdev.h        |  18 +
> >  drivers/net/hinic3/base/hinic3_hwif.c         |  10 +-
> >  drivers/net/hinic3/base/hinic3_mgmt.c         |   5 +-
> >  drivers/net/hinic3/base/hinic3_mgmt.h         |   2 +
> >  drivers/net/hinic3/base/hinic3_nic_cfg.c      | 182 ++---
> >  drivers/net/hinic3/base/hinic3_nic_cfg.h      |  98 ++-
> >  drivers/net/hinic3/base/meson.build           |   1 +
> >  drivers/net/hinic3/hinic3_ethdev.c            | 280 ++++++--
> >  drivers/net/hinic3/hinic3_ethdev.h            | 120 ++--
> >  drivers/net/hinic3/hinic3_fdir.c              | 657 +++++++++++++-----
> >  drivers/net/hinic3/hinic3_fdir.h              | 361 ++++++++--
> >  drivers/net/hinic3/hinic3_nic_io.c            | 525 ++++++--------
> >  drivers/net/hinic3/hinic3_nic_io.h            | 163 ++++-
> >  drivers/net/hinic3/hinic3_rx.c                | 265 +++++--
> >  drivers/net/hinic3/hinic3_rx.h                | 182 ++++-
> >  drivers/net/hinic3/hinic3_tx.c                | 458 ++++++------
> >  drivers/net/hinic3/hinic3_tx.h                | 154 +++-
> >  .../net/hinic3/htn_adapt/hinic3_htn_cmdq.c    | 167 +++++
> >  .../net/hinic3/htn_adapt/hinic3_htn_cmdq.h    |  63 ++
> >  drivers/net/hinic3/htn_adapt/meson.build      |   7 +
> >  drivers/net/hinic3/meson.build                |   8 +-
> >  .../net/hinic3/stn_adapt/hinic3_stn_cmdq.c    | 151 ++++
> >  .../net/hinic3/stn_adapt/hinic3_stn_cmdq.h    |  46 ++
> >  drivers/net/hinic3/stn_adapt/meson.build      |   7 +
> >  33 files changed, 3362 insertions(+), 1443 deletions(-)  create mode
> > 100644 drivers/net/hinic3/base/hinic3_cmdq_enhance.c
> >  create mode 100644 drivers/net/hinic3/base/hinic3_cmdq_enhance.h
> >  create mode 100644 drivers/net/hinic3/htn_adapt/hinic3_htn_cmdq.c
> >  create mode 100644 drivers/net/hinic3/htn_adapt/hinic3_htn_cmdq.h
> >  create mode 100644 drivers/net/hinic3/htn_adapt/meson.build
> >  create mode 100644 drivers/net/hinic3/stn_adapt/hinic3_stn_cmdq.c
> >  create mode 100644 drivers/net/hinic3/stn_adapt/hinic3_stn_cmdq.h
> >  create mode 100644 drivers/net/hinic3/stn_adapt/meson.build
> >
> > --
> > 2.45.1.windows.1
> >
> 
> Still lots of things that need to be addressed here.
> See the following AI patch review.
> 
> 
> Patch 1/7: net/hinic3: add support for new SPx series NIC
> 
> HINIC3_DEV_ID_SP230 is defined as 0X0229 with an uppercase X.
> Every other hex constant in the file uses lowercase 0x. Should be 0x0229.
[Feifei] Done
> 
> HINIC3_DEV_ID_SP920 has no VF counterpart and no entry in hinic3_is_vf_dev().
> Is that intentional?
[Feifei] Yes, SP920 now doesn't support VF function.
> 
> Patch 2/7: net/hinic3: add enhance cmdq support
> 
> The unified cmdq_sync_cmd() retains rte_smp_rmb() which is a deprecated barrier.
> Since this function is being rewritten, please convert to
> rte_atomic_thread_fence(rte_memory_order_acquire).
[Feifei] Done
> 
> HINIC3_CMDQ_BUF_SIZE changed from 2048 to 1024 without any explanation in
> the commit message. If existing normal-cmdq commands relied on the larger
> buffer this could silently truncate them.
[Feifei] Done, explanation is written in patch 2 commit message:
"HINIC3_CMDQ_BUF_SIZE changed from 2048 to 1024 to adapt to
the two types of NICs"
> 
> Patch 3/7: net/hinic3: use different callback func to split new/old cmdq operations
> 
> The call sites in hinic3_ethdev.c use hinic3_cmdq_get_stn_ops() and
> hinic3_cmdq_get_htn_ops() but the actual definitions are
> hinic3_nic_cmdq_get_stn_ops() and hinic3_nic_cmdq_get_htn_ops().
> This will not link. Patch 4 fixes the call site but each commit must compile
> independently.
[Feifei] Done.
> 
> In htn_adapt/hinic3_htn_cmdq.c the static function
> prepare_rss_indir_table_cmd_header() is called before it is defined, with no
> forward declaration. This will fail with -Werror=implicit-function-declaration.
[Feifei]Done.
> 
> Patch 4/7: net/hinic3: add fun init ops to support Compact CQE
> 
> In hinic3_rx_queue_setup(), when ci_mz allocation fails the code calls
> hinic3_memzone_free(ci_mz) on a NULL pointer before jumping to the error label.
> This call is either a NULL dereference or dead code and should be removed.
[Feifei]Done.
> 
> The error-path goto labels in hinic3_func_init() are misordered.
> The allocation order is mac_addrs, cmdq_ops, rx_ops, tx_ops, mc_list but the
> cleanup labels do not reverse this properly. For example if cmdq_ops allocation
> fails, mac_addrs is never freed.
> If tx_ops fails, neither cmdq_ops nor mac_addrs are freed. This leaks memory on
> every init failure path.
[Feifei]Done.
> 
> The ternary in hinic3_pf_get_default_cos() assigns HINIC3_COS_NUM_MAX (8)
> when NIC_F_HTN_CMDQ is set and HINIC3_COS_NUM_MAX_HTN (4) when it is
> not. The naming suggests this is backwards. Please verify.
[Feifei]Done
> 
> Patch 5/7: net/hinic3: add rx ops to support Compact CQE
> 
> hinic3_poll_integrated_cqe_rq_empty() uses __atomic_load_n() with
> __ATOMIC_ACQUIRE. New code must use rte_atomic_load_explicit() with
> rte_memory_order_acquire instead of the GCC built-in.
[Feifei] Done.
> 
> Patch 7/7: net/hinic3: use different callback func to support htn fdir
> 
> Copy-paste bug in hinic3_rss_hash_update() and hinic3_rss_conf_get().
> In both functions the else branch has:
> 
>     rss_type.ipv6_ext = 0;
>     rss_type.ipv6_ext = 0;
> 
> The second line should be rss_type.tcp_ipv6_ext = 0. As written tcp_ipv6_ext is
> never cleared on non-HTN hardware which will produce incorrect RSS
> configuration.
[Feifei] Done