Thanks for your reviewing Stephen. Sorry for that we upload this patch series which issues have been addressed. Currently, we have superseded this patch series. And after fix these issues, we will updating a new version.
> -----邮件原件----- > 发件人: Stephen Hemminger <[email protected]> > 发送时间: 2026年2月1日 2:18 > 收件人: [email protected] > 抄送: Stephen Hemminger <[email protected]> > 主题: [REVIEW] net/hinic3: use different callback func to support htnfdir > > AI-generated review of hinic3.mbox > Reviewed using Claude (claude-opus-4-5-20251101) on 2026-01-31 > > This is an automated review. Please verify all suggestions. > > --- > > # DPDK Patch Review: net/hinic3 Series > > This is a review of a 7-patch series adding support for new SPx series NICs > to the > hinic3 driver. > > --- > > ## Overall Series Issues > > ### Errors > > 1. **Multiple patches have compilation issues** - Several patches introduce > syntax > errors, missing semicolons, and undefined symbols that would prevent > compilation. > > 2. **Missing meson.build updates** - Patches 3-7 add new source directories > (`htn_adapt/`, `stn_adapt/`) with their own `meson.build` files, but the main > `drivers/net/hinic3/meson.build` is not updated to include these > subdirectories. > > 3. **Inconsistent function naming** - The series introduces functions with > inconsistent naming between patches (e.g., `hinic3_cmdq_get_stn_ops()` vs > `hinic3_nic_cmdq_get_stn_ops()`). > > --- > > ## Patch 1/7: net/hinic3: add support for new SPx series NIC > > ### Errors > > 1. **Line 38 (hinic3_csr.h)**: Uppercase `X` in hex literal: > ```c > #define HINIC3_DEV_ID_SP230 0X0229 > ``` > Should be lowercase `0x0229`. > > 2. **Line 48 (hinic3_ethdev.c)**: Undefined symbol `HINIC3_DEV_ID_VF_SP920` - > this macro is not defined in the header file, only `HINIC3_DEV_ID_920` is > defined. > > ### Warnings > > 1. **Commit message**: "suuport" should be "support" (typo). > > 2. **Alignment inconsistencies** in `hinic3_cmd.h` - the alignment changes are > inconsistent (some use tabs, some use spaces to different column positions). > > --- > > ## Patch 2/7: net/hinic3: add enhance cmdq support for new SPx series NIC > > ### Errors > > 1. **Line 85 (hinic3_cmdq_enhance.c)**: Missing `RTE_UNUSED` or parameter > `nic_dev` is unused in `prepare_rss_indir_table_cmd_header`. > > 2. **Line 169 (hinic3_cmdq_enhance.h)**: Missing space before closing comment: > ```c > #endif /*_HINIC3_CMDQ_ENHANCE_H_ */ > ``` > Should be `/* _HINIC3_CMDQ_ENHANCE_H_ */`. > > 3. **Line 1 (meson.build)**: Missing comma in sources list: > ```python > base_sources = files( > 'hinic3_cmdq_enhance.c' > 'hinic3_cmdq.c', > ``` > Should have comma after first file. > > 4. **Multiple typedef changes** - Using `u8`, `u16`, `u32`, `u64` instead of > standard `uint8_t`, `uint16_t`, etc. DPDK uses standard C types. > > ### Warnings > > 1. **Inconsistent indentation** in `cmdq_sync_cmd()` function - some blocks > have incorrect indentation levels. > > 2. **Missing Doxygen documentation** for new public functions in > `hinic3_cmdq_enhance.h`. > > --- > > ## Patch 3/7: net/hinic3: use different callback func to split new/old cmdq > operations > > ### Errors > > 1. **Line 55-60 (hinic3_nic_io.h)**: Missing semicolons in struct definition: > ```c > struct hinic3_nic_cmdq_ops { > prepare_cmd_buf_clean_tso_lro_space_t > prepare_cmd_buf_clean_tso_lro_space > prepare_cmd_buf_qp_context_multi_store_t > prepare_cmd_buf_qp_context_multi_store > ``` > Each line needs a semicolon. > > 2. **Line 80-81 (hinic3_htn_cmdq.c)**: Function > `prepare_rss_indir_table_cmd_header` is called before it's defined (defined > at line > 93). > > 3. **Line 38 (hinic3_stn_cmdq.h)**: Missing closing brace or semicolon at end > of > struct. > > 4. **Undefined types** - `u32`, `u16`, `u8` used instead of `uint32_t`, > `uint16_t`, > `uint8_t`. > > ### Warnings > > 1. **Missing header guards** check - `hinic3_htn_cmdq.h` and > `hinic3_stn_cmdq.h` header guards don't match standard format. > > --- > > ## Patch 4/7: net/hinic3: add fun init ops to support Compact CQE > > ### Errors > > 1. **Line 614 (hinic3_nic_io.c)**: Unterminated string literal: > ```c > PMD_DRV_LOG(ERR, "Set rq cqe context failed, > qid: %d, err: %d, status: 0x%x, out_size: 0x%x", > ``` > String cannot span lines like this. > > 2. **Line 666 (hinic3_nic_io.c)**: Redefinition of `nic_dev`: > ```c > hinic3_set_rq_enable(struct hinic3_nic_dev *nic_dev, uint16_t q_id, bool > enable) > { > struct hinic3_nic_dev *nic_dev = NULL; > ``` > > 3. **Line 669 (hinic3_nic_io.c)**: Undefined variable `dev` used: > ```c > if (!dev) > return -EINVAL; > ``` > > 4. **Missing function declaration** - `hinic3_nic_tx_rx_ops_init` is called > but not > declared. > > ### Warnings > > 1. **Commit message**: "fun init" should be "func init" or "function init". > > 2. **Signed-off-by has leading space** in commit message. > > --- > > ## Patch 5/7: net/hinic3: add rx ops to support Compact CQE > > ### Errors > > 1. **Line 476 (hinic3_rx.h)**: Missing comma in function declaration: > ```c > hinic3_rx_get_compact_cqe_info(struct hinic3_rxq *rxq, volatile struct > hinic3_rq_cqe *rx_cqe > struct hinic3_cqe_info *cqe_info); > ``` > > 2. **Line 790 (hinic3_rx.c)**: `if` statement without space: > ```c > if(err) { > ``` > Should be `if (err)`. > > 3. **Undefined macro** `HINIC3_PKT_TX_TUNNEL_GENEVE` and > `HINIC3_PKT_TX_TUNNEL_IPIP` used but not defined. > > ### Warnings > > 1. **Inconsistent brace style** - some functions use opening brace on same > line, > others on new line. > > --- > > ## Patch 6/7: net/hinic3: add tx ops to support Compact CQE > > ### Errors > > 1. **Line 379 (hinic3_tx.c)**: Undefined variable `wqe`: > ```c > wqe->task->pkt_info0 = > ``` > Should be `task->pkt_info0`. > > 2. **Line 606 (hinic3_tx.c)**: Typo in function name: > ```c > return inic3_non_tso_pkt_pre_process(mbuf, wqe_info); > ``` > Should be `hinic3_non_tso_pkt_pre_process`. > > 3. **Line 328 (hinic3_tx.h)**: Duplicate enum value: > ```c > enum sq_wqe_data_format { > SQ_NORMAL_WQE = 0, > ``` > `SQ_NORMAL_WQE` is already defined in `enum sq_wqe_type`. > > ### Warnings > > 1. **Missing `RTE_MBUF_F_TX_OUTER_UDP_CKSUM`** - > `HINIC3_PKT_TX_OUTER_UDP_CKSUM` is used but the corresponding RTE flag > mapping is missing. > > --- > > ## Patch 7/7: net/hinic3: use different callback func to support htn fdir > > ### Errors > > 1. **Line 926 (hinic3_fdir.c)**: Wrong parameter type: > ```c > hinic3_tcam_alloc_index(struct hinic3_nic_dev nic_dev, uint16_t *block_id) > ``` > Should be `struct hinic3_nic_dev *nic_dev` (pointer). > > 2. **Line 942 (hinic3_fdir.c)**: Same issue - missing pointer for `nic_dev` > parameter. > > 3. **Line 656 (hinic3_fdir.h)**: Missing semicolon at end of function > declaration: > ```c > void hinic3_tcam_index_free(struct hinic3_nic_dev nic_dev, uint16_t index, > uint16_t *block_id) > ``` > > 4. **Line 750 (hinic3_fdir.c)**: Missing closing brace: > ```c > if (index == HINIC3_TCAM_INVALID_INDEX) { > return -ENOMEM; > } > ``` > The outer `if` block is not closed properly. > > ### Warnings > > 1. **Inconsistent parameter types** - Some functions use `struct > hinic3_nic_dev` > while others use `struct hinic3_nic_dev *`. > > --- > > ## Summary > > This patch series has significant issues that need to be addressed before it > can be > accepted: > > 1. **Compilation errors** - Multiple syntax errors prevent building 2. > **Undefined > symbols** - References to macros and functions that don't exist 3. **Type > inconsistencies** - Mix of kernel-style types (`u32`) and standard types > (`uint32_t`) > 4. **Missing build system updates** - New directories not integrated into > meson > build 5. **Documentation gaps** - New APIs lack Doxygen comments > > The patches should be thoroughly tested for compilation before resubmission.
