Re: [PATCH v2 0/6] net/nfb: code cleanup

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 15 Jan 2026 15:40:34 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> This patchset mainly cleans up the code and prepare it for another
> quite large rework. Also it resolves some unpleasant behavior.
> 
> ---
> v2:
> * Fixed coding style issues
> 
> Martin Spinler (6):
>   net/nfb: use constant values for max Rx/Tx queues count
>   net/nfb: fix bad pointer access in queue stats
>   net/nfb: update timestamp calculation to meaningful value
>   net/nfb: use process private variable for internal data
>   net/nfb: release allocated resources correctly
>   net/nfb: stop only started queues in fail path
> 
>  doc/guides/nics/nfb.rst      |   6 +-
>  drivers/net/nfb/nfb.h        |   5 ++
>  drivers/net/nfb/nfb_ethdev.c | 165 ++++++++++++++++++++---------------
>  drivers/net/nfb/nfb_rx.c     |   2 +-
>  drivers/net/nfb/nfb_rx.h     |  13 +--
>  drivers/net/nfb/nfb_rxmode.c |  12 +--
>  drivers/net/nfb/nfb_stats.c  |  46 +++++-----
>  drivers/net/nfb/nfb_tx.c     |   2 +-
>  8 files changed, 142 insertions(+), 109 deletions(-)
> 
> --
> 2.52.0
> 

Looks ok but AI code review spotted somethings that need to be addressed.


---

## DPDK NFB Driver Patchset Review

**Series:** net/nfb cleanup (6 patches, v2)
**Submitter:** Martin Spinler <spinler@cesnet.cz>

---

### Patch 1/6: `net/nfb: use constant values for max Rx/Tx queues count`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 51 chars (≤60) |
| Subject format | ✅ PASS | Lowercase after colon, no trailing period |
| Component prefix | ✅ PASS | `net/nfb:` is correct |
| Signed-off-by | ✅ PASS | Present with valid name/email |
| Fixes tag | ✅ PASS | `Fixes: 6435f9a0ac22` (12-char SHA) |
| Cc: stable | ✅ PASS | Present |
| Tag order | ✅ PASS | Fixes before Cc, blank line before Signed-off-by |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |

**Code Review:**
- Clean addition of `max_rx_queues` and `max_tx_queues` to `pmd_internals` structure
- Proper initialization from hardware counts instead of mutable `nb_*_queues`

**Issue (Warning):** The log message at line 543 still references `data->nb_rx_queues` / `data->nb_tx_queues` which will be 0 at that point since they're no longer initialized. Should log `internals->max_rx_queues` and `internals->max_tx_queues` instead.

---

### Patch 2/6: `net/nfb: fix bad pointer access in queue stats`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 47 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ✅ PASS | Present with 12-char SHA |
| Cc: stable | ✅ PASS | Present |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |

**Code Review:**
- Correctly fixes the pointer dereference pattern from `rx_queue[i].rx_pkts` to `rx_queue->rx_pkts`
- The original code treated an array of pointers as an array of structures — this is a legitimate bug fix

**Issue (Warning):** Still missing NULL pointer checks before dereferencing `dev->data->rx_queues[i]` and `dev->data->tx_queues[i]`. The commit message mentions the pointer can be NULL, but the fix doesn't add the NULL check.

---

### Patch 3/6: `net/nfb: update timestamp calculation to meaningful value`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 55 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This is a bug fix (non-monotonic timestamp) |
| Cc: stable | ⚠️ **MISSING** | Should be present for bug fixes |
| Body doesn't start with "It" | ✅ PASS | Starts with "The" |
| Doc + code atomic | ✅ PASS | Both `nfb.rst` and `nfb_rx.h` updated together |

**Recommendation:** Add:
```
Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org
```

---

### Patch 4/6: `net/nfb: use process private variable for internal data`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 56 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This fixes multi-process support |
| Cc: stable | ⚠️ **MISSING** | Should be present for fixes |

**Code Issues:**

1. **Error (Style):** Line 148 changes a Doxygen comment to broken format:
   ```c
   -/**
   +/* *
   ```
   This breaks Doxygen documentation parsing. Should remain `/**` or be changed to `/*` (without the space after asterisk).

2. **Warning:** The new `struct pmd_priv` only contains `max_rx_queues` and `max_tx_queues` as `int` types. These should likely be `uint16_t` to match the DPDK convention for queue counts.

---

### Patch 5/6: `net/nfb: release allocated resources correctly`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 49 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This fixes resource leaks |
| Cc: stable | ⚠️ **MISSING** | Should be present |

**Code Review:**
- Good refactoring of cleanup paths using `goto` error handling
- Properly separates primary vs secondary process cleanup

**Issue (Warning):** Forward declaration added at line 21:
```c
static int nfb_eth_dev_uninit(struct rte_eth_dev *dev);
```
This is fine, but the blank line before it creates two consecutive blank lines which is non-standard.

---

### Patch 6/6: `net/nfb: stop only started queues in fail path`

| Check | Status | Notes |
|-------|--------|-------|
| Subject length | ✅ PASS | 47 chars |
| Subject format | ✅ PASS | Correct format |
| Signed-off-by | ✅ PASS | Present |
| Fixes tag | ⚠️ **MISSING** | This is a bug fix |
| Cc: stable | ⚠️ **MISSING** | Should be present |

**Code Review:**
- Correctly uses reverse iteration to only stop queues that were successfully started
- Loop logic is correct: `for (; i > 0; i--)` then `stop(dev, i - 1)`

---

## Summary

### Errors (Must Fix)

| Patch | Issue |
|-------|-------|
| 4/6 | Broken Doxygen comment: `/* *` should be `/**` or `/*` |

### Warnings (Should Fix)

| Patch | Issue |
|-------|-------|
| 1/6 | Log message references uninitialized `data->nb_*_queues` |
| 2/6 | Missing NULL checks for queue pointers (mentioned in commit but not implemented) |
| 3/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org` |
| 4/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org`; consider `uint16_t` for queue counts |
| 5/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org`; extra blank line before forward declaration |
| 6/6 | Missing `Fixes:` tag and `Cc: stable@dpdk.org` |

### Info (Consider)

- The series is well-structured with logical patch ordering
- Good that patches 1 and 2 have proper `Fixes:` tags — patches 3-6 should follow the same pattern
- Documentation is updated atomically with code in patch 3

### Verdict

**Needs revision.** The main blocker is the broken Doxygen comment in patch 4/6. Additionally, patches 3-6 should have `Fixes:` tags and `Cc: stable@dpdk.org` since they all fix bugs in the original driver.