Re: [PATCH 0/8] net/nfb: rework to real multiport

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 15 Jan 2026 16:16:48 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> This series implements real multiport for better user experience.
> 
> The existing driver creates one ethdev/port for one PCI device.
> As the CESNET-NDK based cards aren't capable to represent each
> Ethernet port by own PCI device, new driver implementation
> processes real port configuration from firmware/card and switches
> from rte_eth_dev_pci_generic_probe to multiple rte_eth_dev_create calls.
> 
> ---
> Depends-on: series-37064 ("net/nfb: code cleanup")
> 
> Martin Spinler (8):
>   net/nfb: prepare for indirect queue mapping scheme
>   net/nfb: create ethdev for every eth port/channel
>   net/nfb: add vdev as alternative device probe method
>   net/nfb: add device argument "port" to limit used ports
>   net/nfb: init only MACs associated with device
>   net/nfb: add compatible cards to driver PCI ID table
>   net/nfb: report firmware version
>   doc/nfb: cleanup and update guide
> 
>  doc/guides/nics/features/nfb.ini |   4 +
>  doc/guides/nics/nfb.rst          | 187 +++++++--------
>  drivers/net/nfb/meson.build      |   1 +
>  drivers/net/nfb/nfb.h            |  48 +++-
>  drivers/net/nfb/nfb_ethdev.c     | 399 +++++++++++++++++++++++--------
>  drivers/net/nfb/nfb_rx.c         |  21 +-
>  drivers/net/nfb/nfb_tx.c         |  21 +-
>  drivers/net/nfb/nfb_vdev.c       |  97 ++++++++
>  8 files changed, 551 insertions(+), 227 deletions(-)
>  create mode 100644 drivers/net/nfb/nfb_vdev.c
> 
> --
> 2.52.0


AI had some good feedback on this patch series.
Some of this noise though.

# DPDK NFB Multiport Patch Series Review

**Series:** `[PATCH 1-8/8]` NFB multiport support  
**Author:** Martin Spinler <spinler@cesnet.cz>  
**Date:** Thu, 15 Jan 2026  

---

## Executive Summary

This patch series adds multiport support to the NFB driver, changing from a single ethdev per PCI device to one ethdev per Ethernet channel. The series is generally well-structured and follows DPDK conventions, but has several issues requiring attention before merging.

---

## Patch-by-Patch Review

### Patch 1/8: `net/nfb: prepare for indirect queue mapping scheme`

**Subject Line:** ✅ OK (52 chars, correct prefix, lowercase)

**Commit Message:**
- ⚠️ **WARNING:** Body is minimal - could use more context explaining *why* queue mapping needs to change
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 113-118 | ⚠️ Warning | Unnecessary braces for single-statement `for` loops |
| 161 | ⚠️ Warning | Comment typo: "neccessary" → "necessary" |
| 205 | ⚠️ Warning | Same typo: "neccessary" → "necessary" |

```c
// Current (unnecessary braces):
for (i = 0; i < priv->max_rx_queues; i++) {
    internals->queue_map_rx[i] = i;
}

// Preferred:
for (i = 0; i < priv->max_rx_queues; i++)
    internals->queue_map_rx[i] = i;
```

**Style:** ✅ OK - uses `rte_malloc`/`rte_free` appropriately for driver code

---

### Patch 2/8: `net/nfb: create ethdev for every eth port/channel`

**Subject Line:** ✅ OK (49 chars)

**Commit Message:**
- ✅ Good explanation of the change rationale
- ⚠️ **WARNING:** Line 297 "informations" → "information" (grammar)
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 341-342 | ⚠️ Warning | Double blank line after struct definition |
| 373 | ℹ️ Info | Consider using `TAILQ_HEAD_INITIALIZER` inline instead of separate declaration |
| 545-547 | ⚠️ Warning | Comment says "doesn't belong" but code checks `ep != ep_index` which is confusing |

**Structural Issues:**
- ✅ Good use of TAILQ for device list management
- ✅ Proper error cleanup with `goto` labels
- ⚠️ **WARNING:** `nfb_eth_common_probe()` declared in header but missing `__rte_internal` tag for internal API

---

### Patch 3/8: `net/nfb: add vdev as alternative device probe method`

**Subject Line:** ✅ OK (50 chars)

**Commit Message:**
- ✅ Clear explanation
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 719 | ❌ Error | Copyright year "2019" should be updated to current year (2026) for new file |
| 729-730 | ℹ️ Info | Double blank line |
| 766 | ⚠️ Warning | Cast `(signed)` is unusual - prefer explicit `int` comparison |
| 778 | ⚠️ Warning | `strcpy()` is unsafe - use `strlcpy()` or `snprintf()` |

```c
// Current (unsafe):
strcpy(params.name, vdev_name);

// Preferred:
strlcpy(params.name, vdev_name, sizeof(params.name));
// or
snprintf(params.name, sizeof(params.name), "%s", vdev_name);
```

| Line | Severity | Issue |
|------|----------|-------|
| 751, 784 | ⚠️ Warning | Using libc `strdup()`/`free()` - should use `rte_strdup()`/`rte_free()` for consistency |

**License:** ✅ SPDX-License-Identifier present and correct (BSD-3-Clause)

---

### Patch 4/8: `net/nfb: add device argument "port" to limit used ports`

**Subject Line:** ✅ OK (52 chars)

**Commit Message:**
- ⚠️ **WARNING:** Line 876 - "NFB devices does not" → "NFB devices do not" (grammar)
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 905-908 | ⚠️ Warning | `static const char * const` array defined in header - should be in .c file to avoid multiple definitions |
| 927 | ⚠️ Warning | Using hex base (16) for port parsing but doc says `port=<number>` - inconsistent |
| 971 | ⚠️ Warning | Missing space after cast: `(void*)` → `(void *)` |

```c
// Current:
if (rte_kvargs_process(kvlist, NFB_ARG_PORT, fill_port_mask, (void*) &port_mask))

// Preferred:
if (rte_kvargs_process(kvlist, NFB_ARG_PORT, fill_port_mask, (void *)&port_mask))
```

---

### Patch 5/8: `net/nfb: init only MACs associated with device`

**Subject Line:** ✅ OK (46 chars)

**Commit Message:**
- ⚠️ **WARNING:** "informations" → "information" (grammar)
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 1147-1148, 1155-1161, 1200-1206 | ⚠️ Warning | Unnecessary braces around single `if` body |
| 1192 | ℹ️ Info | Extra blank line at start of function body |

**Good practices observed:**
- ✅ Dynamic allocation of MAC arrays based on actual count
- ✅ Proper cleanup with `rte_free()`

---

### Patch 6/8: `net/nfb: add compatible cards to driver PCI ID table`

**Subject Line:** ✅ OK (52 chars)

**Commit Message:**
- ✅ Clear list of added cards
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 1421-1423 | ℹ️ Info | Inconsistent spacing in `RTE_PCI_DEVICE` calls - some have 1 space, some have 2 |

```c
// Inconsistent:
{ RTE_PCI_DEVICE(PCI_VENDOR_ID_SILICOM, PCI_DEVICE_ID_FB2CGHH) },
{ RTE_PCI_DEVICE(PCI_VENDOR_ID_CESNET,  PCI_DEVICE_ID_COMBO400G1) },  // 2 spaces

// Should be consistent throughout
```

**Documentation:** ⚠️ **WARNING** - New device IDs should be mentioned in release notes

---

### Patch 7/8: `net/nfb: report firmware version`

**Subject Line:** ✅ OK (33 chars)

**Commit Message:**
- ❌ **ERROR:** Missing commit body - should explain what firmware version info is reported and why
- ✅ Signed-off-by present

**Code Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 1508-1509 | ⚠️ Warning | Unnecessary cast from `void *` - `dev->process_private` is already `void *` |
| 1521 | ⚠️ Warning | Cast `(signed)` is unusual - prefer `(int)fw_size` or comparison with explicit types |

```c
// Current (unnecessary cast):
struct pmd_internals *priv = (struct pmd_internals *)
        dev->process_private;

// Preferred:
struct pmd_internals *priv = dev->process_private;
```

**Documentation:** ✅ Features matrix updated in patch 8

---

### Patch 8/8: `doc/nfb: cleanup and update guide`

**Subject Line:** ✅ OK (31 chars)

**Commit Message:**
- ❌ **ERROR:** Missing commit body - should summarize what's changed in docs
- ✅ Signed-off-by present

**Documentation Issues:**

| Line | Severity | Issue |
|------|----------|-------|
| 1674-1675 | ❌ Error | URL uses `http://` - should be `https://` per DPDK guidelines |
| 1761 | ⚠️ Warning | Typo: "lack a of lot" → "lack a lot" |
| 1767-1770 | ℹ️ Info | Extra blank lines in Features section |
| 1827 | ⚠️ Warning | Stray backtick at end: `-- -i\`` → `-- -i` |

```rst
// Current (line 1674):
`Liberouter website <http://www.liberouter.org/>`_.

// Should be https
```

**Features Matrix:**
- ✅ Added: Unicast MAC filter, Timestamp offload (P), FW version, Multiprocess aware
- ⚠️ Should verify these actually work as documented

---

## Summary of Issues

### Errors (Must Fix)

| Patch | Issue |
|-------|-------|
| 3/8 | Copyright year 2019 in new file should be 2026 |
| 7/8 | Missing commit body |
| 8/8 | Missing commit body |
| 8/8 | URL uses `http://` instead of `https://` |

### Warnings (Should Fix)

| Patch | Issue |
|-------|-------|
| 1/8 | Typo: "neccessary" (2 occurrences) |
| 2/8 | Grammar: "informations" |
| 2/8 | Missing `__rte_internal` on internal API |
| 3/8 | Unsafe `strcpy()` - use `strlcpy()` |
| 3/8 | libc `strdup()`/`free()` instead of `rte_*` variants |
| 4/8 | Grammar: "does not" → "do not" |
| 4/8 | `static const` array in header file |
| 4/8 | Missing space after cast |
| 5/8 | Grammar: "informations" |
| 6/8 | New device IDs need release notes |
| 7/8, 8/8 | Unnecessary casts from `void *` |
| 8/8 | Typo: "lack a of lot" |
| 8/8 | Stray backtick in command example |

### Info (Consider)

| Patch | Issue |
|-------|-------|
| 1/8, 5/8 | Unnecessary braces for single statements |
| 3/8, 4/8 | Double blank lines |
| 6/8 | Inconsistent spacing in macro calls |

---

## Missing Items

1. **Release Notes:** No `doc/guides/rel_notes/` entry for this significant driver change
2. **Cc: stable@dpdk.org:** Not applicable (new feature, not a fix)
3. **Cover Letter:** Series lacks a cover letter explaining the overall motivation and design

---

## Recommendations

1. Add a cover letter (patch 0/8) explaining the multiport architecture changes
2. Update release notes for the new multiport support and new device IDs
3. Fix all typos and grammar issues in commit messages
4. Add commit bodies to patches 7/8 and 8/8
5. Update copyright year in new files
6. Replace unsafe string functions with safe variants
7. Consider using `rte_strdup()` instead of libc `strdup()` for consistency

---

## Verdict

**Needs Revision (v2)**

The patch series is fundamentally sound and addresses a real limitation of the existing driver. However, there are several documentation and style issues that should be addressed before merging. The most critical issues are the missing commit bodies, documentation URL protocol, and the unsafe `strcpy()` usage.