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

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 21 Jan 2026 18:01:14 +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.
> 
> ---
> v4:
> * Added comment for process private/shared data.
> * Added Fixes+Cc tag for patch 4,5,6
> * Moved shared data assignments to RTE_PROC_PRIMARY block
> 
> v3:
> * Changed type of queue count variable to uint16_t.
> * Fixed wrong queue count variable usage in log.
> * Fixed another coding style issues.
> 
> 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        |  14 ++-
>  drivers/net/nfb/nfb_ethdev.c | 166 +++++++++++++++++++++--------------
>  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, 152 insertions(+), 109 deletions(-)
> 
> --
> 2.52.0
> 

Queued to next-net

Verbose AI patch review (if you care) mostly it wants more Fixes: tags.


## NFB Driver Patch Series v4 Review

### Series Overview

This is a 6-patch series fixing multiple bugs in the NFB driver:
1. Use constant values for max Rx/Tx queues count
2. Fix bad pointer access in queue stats  
3. Update timestamp calculation to meaningful value
4. Use process private variable for internal data
5. Release allocated resources correctly
6. Stop only started queues in fail path

---

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

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 54 characters |
| Lowercase after colon | ✅ PASS | |
| Correct prefix (`net/nfb:`) | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| No trailing period | ✅ PASS | |
| Body ≤75 chars/line | ✅ PASS | |
| Body doesn't start with "It" | ⚠️ WARNING | Starts with "The" - acceptable but close |
| `Signed-off-by:` present | ✅ PASS | Valid name and email |
| `Fixes:` tag present | ✅ PASS | 12-char SHA with quoted subject |
| `Cc: stable@dpdk.org` | ✅ PASS | |
| Tag order | ✅ PASS | Fixes, then Cc, then blank, then Signed-off-by |

**Code Review:**
- Clean change, stores max queue counts in the `pmd_internals` structure
- Properly uses the hardware-reported counts instead of the dynamic `data->nb_*_queues` values
- No style issues detected

**Verdict: ✅ Ready for merge**

---

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

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 46 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
- Fixes incorrect array-of-structures vs array-of-pointers dereference
- The fix correctly iterates through the queue pointers
- Moves queue pointer fetching inside the loop

**Verdict: ✅ Ready for merge**

---

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

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 53 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ❌ MISSING | This changes timestamp semantics - is it a bug fix? |
| `Cc: stable@dpdk.org` | ❌ MISSING | If this is a behavioral fix, it should have these |

**Code Review:**
- Changes timestamp from packed seconds:nanoseconds format to nanoseconds since epoch
- Documentation updated atomically - good
- Uses `NSEC_PER_SEC` from `<rte_time.h>` - appropriate

**Issue:** The commit body states "The resulting timestamp wasn't monotonically increasing value" which suggests this IS a bug fix. If so, it should have:
```
Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver")
Cc: stable@dpdk.org
```

**Verdict: ⚠️ Needs revision** - Missing Fixes/Cc tags if this is indeed a bug fix

---

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

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 56 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| Body ≤75 chars/line | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
- Properly separates process-private (libnfb handles) from shared data
- Introduces new `struct pmd_priv` for shared data (`max_rx_queues`, `max_tx_queues`)
- `struct pmd_internals` now contains process-local handles
- Comments added explaining the purpose of each structure - good

**Style note:** The new struct comment style is good:
```c
/*
 * Handles obtained from the libnfb: each process must use own instance.
 * Stored inside dev->process_private.
 */
```

**Verdict: ✅ Ready for merge**

---

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

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 47 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
- Refactors error handling in `nfb_eth_dev_init()` to use goto-style cleanup
- Properly sequences resource cleanup:
  - `err_ts_register` → calls `nfb_eth_dev_uninit()`
  - `err_malloc_mac_addrs` → deinits rxmac/txmac, closes nfb
  - `err_nfb_open` → frees internals
  - `err_alloc_internals` → returns error
- Moves rxmac/txmac deinit and nfb_close from `close()` to `uninit()`
- Adds forward declaration for `nfb_eth_dev_uninit()`

**Minor observation:** The refactored `nfb_eth_dev_close()` properly handles queue cleanup with NULL checks, which is good defensive programming.

**Verdict: ✅ Ready for merge**

---

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

**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 48 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` present | ✅ PASS | |
| `Fixes:` tag | ✅ PASS | |
| `Cc: stable@dpdk.org` | ✅ PASS | |

**Code Review:**
```c
err_tx:
-	for (i = 0; i < nb_tx; i++)
-		nfb_eth_tx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_tx_queue_stop(dev, i - 1);
+	i = nb_rx;
err_rx:
-	for (i = 0; i < nb_rx; i++)
-		nfb_eth_rx_queue_stop(dev, i);
+	for (; i > 0; i--)
+		nfb_eth_rx_queue_stop(dev, i - 1);
```

- Properly stops only the queues that were successfully started
- Uses the loop counter `i` which indicates where the failure occurred
- The `i = nb_rx` assignment before `err_rx` ensures all Rx queues are stopped when coming from `err_tx`

**Verdict: ✅ Ready for merge**

---

## Summary

| Patch | Status | Action Required |
|-------|--------|-----------------|
| 1/6 | ✅ Ready | None |
| 2/6 | ✅ Ready | None |
| 3/6 | ⚠️ Warning | Add `Fixes:` and `Cc: stable@dpdk.org` if this is a bug fix |
| 4/6 | ✅ Ready | None |
| 5/6 | ✅ Ready | None |
| 6/6 | ✅ Ready | None |

### Recommendation

The series is well-structured with proper inter-patch dependencies. The v4 revision addresses the issue from earlier reviews by properly separating process-private and shared data.

**For patch 3/6:** The commit message describes non-monotonic timestamp behavior as a defect. Please either add the `Fixes:` and `Cc: stable@dpdk.org` tags, or clarify in the commit message that this is an intentional behavioral change rather than a bug fix.

With that clarification/fix, this series is ready for merge.