Re: [PATCH v2 0/5] net/zxdh: add support some new features

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  5 Jan 2026 15:16:38 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:

> V2:
>   - add updated release note and the documentation.
>   - remove unnecessary initialization.
>   - using the compiler builtins(__rte_clz32).
>   - remove C++ style comments.
> 
> V1:
>   - support modifying queue depth.
>   - optimeze alloc queue resources.
>   - support set link speed.
>   - support primary/secondary process.
>   - support GENEVE TSO/chksum,and tunnel packets outer udp chksum.
> 
> Junlong Wang (5):
>   net/zxdh: add support for modifying queue depth
>   net/zxdh: optimize alloc queue resources
>   net/zxdh: add support set link speed get autoneg stats
>   net/zxdh: add support primary/secondary process
>   net/zxdh: add support GENEVE TSO and Rx outer chksum
> 
>  doc/guides/rel_notes/release_26_03.rst |   7 +
>  drivers/net/zxdh/zxdh_common.c         |  75 ++------
>  drivers/net/zxdh/zxdh_common.h         |   2 +-
>  drivers/net/zxdh/zxdh_ethdev.c         | 239 +++++++++++++++++--------
>  drivers/net/zxdh/zxdh_ethdev.h         |  23 ++-
>  drivers/net/zxdh/zxdh_ethdev_ops.c     | 165 +++++++++++++----
>  drivers/net/zxdh/zxdh_ethdev_ops.h     |  15 ++
>  drivers/net/zxdh/zxdh_msg.c            |  52 ++++++
>  drivers/net/zxdh/zxdh_msg.h            |  24 ++-
>  drivers/net/zxdh/zxdh_pci.h            |   2 +
>  drivers/net/zxdh/zxdh_queue.c          | 134 +++++++++++---
>  drivers/net/zxdh/zxdh_queue.h          |  12 +-
>  drivers/net/zxdh/zxdh_rxtx.c           | 116 +++++++++---
>  drivers/net/zxdh/zxdh_tables.h         |  12 +-
>  14 files changed, 651 insertions(+), 227 deletions(-)
> 

I did not do detailed manual review yet, but did use this as a test subject
patch for using Claude Opus 4.5 to review. This is using the recently
posted AGENTS.md as guidelines.

Do not get too worried. We are not going to switch to automated only
patch review. And everything it reported is something that is a big issue.
For example, the AGENTS.md file needs update to inform AI review that
C11 is allowed (even preferred) in DPDK.

https://claude.ai/public/artifacts/a9bc2f1d-46bf-4bb2-8f8c-886ed8fc9730

The result was:

# DPDK Patch Review: bundle-1621-zxdh-v2

**Series:** net/zxdh improvements v2  
**Author:** Junlong Wang <wang.junlong1@zte.com.cn>  
**Patches:** 5  
**Date:** Mon, 5 Jan 2026  

---

## Summary

| Patch | Subject | Verdict |
|-------|---------|---------|
| 1/5 | add support for modifying queue depth | ⚠️ Minor issues |
| 2/5 | optimize alloc queue resources | ⚠️ Minor issues |
| 3/5 | add support set link speed get autoneg stats | ❌ Subject grammar issue |
| 4/5 | add support primary/secondary process | ⚠️ Minor issues |
| 5/5 | add support GENEVE TSO and Rx outer chksum | ⚠️ Minor issues |

---

## Patch 1/5: net/zxdh: add support for modifying queue depth

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject length ≤60 chars | ✅ Pass | 47 characters |
| Lowercase after colon | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Proper prefix | ✅ Pass | `net/zxdh:` |
| Imperative mood | ✅ Pass | "add support" |
| Body wrapped at 75 chars | ✅ Pass | |
| Signed-off-by present | ✅ Pass | |

### Code Review

**Warning: Missing space before `{` in conditional**
```c
// Line in zxdh_queue.c
if (dev->data->nb_tx_queues != hw->tx_qnum ||
    valid_nb_desc != hw->queue_conf->conf[queue_idx].tx_nb_desc){  // Missing space before {
```
Should be:
```c
    valid_nb_desc != hw->queue_conf->conf[queue_idx].tx_nb_desc) {
```

**Info: Double blank line detected**
```c
+	return nb_desc;
+}
+
+
+int32_t
```
Two consecutive blank lines between functions - DPDK style typically uses one blank line.

**Style: Variable declaration after code**
```c
+int32_t
+zxdh_rx_queue_config(struct rte_eth_dev *dev, uint16_t queue_idx)
+{
    // ... code ...
+	struct rte_eth_rxconf *rx_conf = &hw->queue_conf->conf[queue_idx].zxdh_rx_conf;
```
In C89/DPDK style, variable declarations should typically be at the start of the block.

---

## Patch 2/5: net/zxdh: optimize alloc queue resources

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject length ≤60 chars | ✅ Pass | 40 characters |
| Lowercase after colon | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Proper prefix | ✅ Pass | |
| Body wrapped at 75 chars | ✅ Pass | |
| Signed-off-by present | ✅ Pass | |

### Code Review

**Warning: Magic number usage**
```c
+	if ((ph_chno) >=  ZXDH_QUEUES_PCIEID_SIZE) {
```
Good use of named constant.

**Info: Parentheses around `ph_chno` are unnecessary**
```c
if ((ph_chno) >=  ZXDH_QUEUES_PCIEID_SIZE)
```
Could simplify to:
```c
if (ph_chno >= ZXDH_QUEUES_PCIEID_SIZE)
```

**Info: Double space detected**
```c
if ((ph_chno) >=  ZXDH_QUEUES_PCIEID_SIZE)
              ^^
```

---

## Patch 3/5: net/zxdh: add support set link speed get autoneg stats

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject length ≤60 chars | ✅ Pass | 54 characters |
| Lowercase after colon | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Proper prefix | ✅ Pass | |
| **Grammar** | ❌ Fail | Missing "for" and "and" |
| Signed-off-by present | ✅ Pass | |

**Error: Subject line grammar issue**

Current:
```
net/zxdh: add support set link speed get autoneg stats
```

Should be:
```
net/zxdh: add support for setting link speed and getting autoneg stats
```
or shorter:
```
net/zxdh: add link speed set and autoneg get support
```

### Code Review

**Warning: Inconsistent return type initialization**
```c
-	int32_t ret = 0;
+	int32_t ret;
```
Changed from initialized to uninitialized in `zxdh_dev_link_update`. This is fine but should be consistent across the codebase.

**Info: Switch without default case comment**
The switch in `zxdh_speed_mode_to_spm` has a default case, which is good.

---

## Patch 4/5: net/zxdh: add support primary/secondary process

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject length ≤60 chars | ✅ Pass | 47 characters |
| Lowercase after colon | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Proper prefix | ✅ Pass | |
| **Grammar** | ⚠️ Warning | Missing "for" |
| Signed-off-by present | ✅ Pass | |

**Warning: Subject line grammar**

Current:
```
net/zxdh: add support primary/secondary process
```

Suggested:
```
net/zxdh: add support for primary/secondary process
```

### Code Review

**Good: Proper secondary process handling**
```c
+	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		eth_dev->dev_ops = &zxdh_eth_dev_secondary_ops;
+		ZXDH_VTPCI_OPS(hw) = &zxdh_dev_pci_ops;
+		return 0;
+	}
```
Correctly limits secondary process to read-only operations.

**Info: Removed extern declaration**
```c
-extern struct zxdh_net_hdr_dl g_net_hdr_dl[RTE_MAX_ETHPORTS];
```
Good cleanup by moving to per-device structure.

---

## Patch 5/5: net/zxdh: add support GENEVE TSO and Rx outer chksum

### Commit Message

| Check | Status | Notes |
|-------|--------|-------|
| Subject length ≤60 chars | ✅ Pass | 52 characters |
| Lowercase after colon | ✅ Pass | |
| No trailing period | ✅ Pass | |
| Proper prefix | ✅ Pass | |
| **Grammar** | ⚠️ Warning | Missing "for" |
| Case-sensitive terms | ✅ Pass | `Rx` is correct (per words-case.txt) |
| Signed-off-by present | ✅ Pass | |

**Warning: Subject line grammar**

Current:
```
net/zxdh: add support GENEVE TSO and Rx outer chksum
```

Suggested:
```
net/zxdh: add support for GENEVE TSO and Rx outer checksum
```

### Code Review

**Good: Release notes updated**
```c
+* **Updated ZTE zxdh ethernet driver.**
+
+  * Added support for modify queue depth.
+  * Optimized alloc queue resources.
+  * Added support for set link speed and get autoneg status.
+  * Added support for primary/secondary process.
+  * Added support for GENEVE-TSO and tunnel packet RX outer UDP chksum.
```
Documentation properly updated with new features.

**Warning: "RX" in release notes should be "Rx"**
```
+  * Added support for GENEVE-TSO and tunnel packet RX outer UDP chksum.
```
Per `words-case.txt`, should be `Rx` not `RX`.

**Info: Byte order handling**
Good use of `rte_be_to_cpu_16()` for network byte order conversion.

---

## Global Issues

### Missing Items

1. **No Cc: stable@dpdk.org** - If any of these patches fix bugs that should be backported, they should include `Cc: stable@dpdk.org`

2. **No Fixes: tag** - If any patches fix regressions from previous commits, they need a `Fixes:` tag

### Code Style Issues Across Series

1. **Variable declarations mid-block** - Several patches declare variables after executable code, which can cause issues with older C standards

2. **Inconsistent spacing** - Some double spaces and missing spaces before braces

---

## Recommendations

### Must Fix (Errors)

1. **Patch 3/5**: Fix subject line grammar - "add support set link speed get autoneg stats" → "add link speed and autoneg support"

### Should Fix (Warnings)

1. **Patch 1/5**: Add space before `{` in conditional statement
2. **Patch 4/5**: Subject grammar - add "for" 
3. **Patch 5/5**: 
   - Subject grammar - add "for"
   - Change "RX" to "Rx" in release notes

### Consider (Info)

1. Remove double blank lines between functions (Patch 1/5)
2. Remove unnecessary parentheses (Patch 2/5)
3. Fix double space typo (Patch 2/5)
4. Move variable declarations to block start for C89 compatibility

---

## Validation Commands

Before resubmitting, the author should run:

```bash
# Check commit messages
devtools/check-git-log.sh -n5

# Check patch format and forbidden tokens
devtools/checkpatches.sh *.patch

# Build validation
devtools/test-meson-builds.sh
```

---

## Verdict

**Series Status: Needs Minor Revision**

The patches are generally well-structured and follow DPDK conventions. The main issues are:
- Subject line grammar in patches 3, 4, and 5
- Minor code style issues (spacing, declarations)
- Case sensitivity in release notes ("RX" → "Rx")

After addressing the grammar issues in the subject lines and the style issues, this series should be ready for merge.