On Wed, 21 Jan 2026 18:03:25 +0100
[email protected] wrote:
> From: Martin Spinler <[email protected]>
>
> 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.
>
> ---
TLDR; Fix the use of sprintf() in patch 3 and resubmit
Wordy AI review output...
## NFB Multiport Patch Series v3 Review
### Series Overview
This is a significant feature series that transforms the NFB driver from
single-port-per-PCI to multi-port-per-PCI architecture, with vdev support and
new card additions.
---
### Patch 1/8: `net/nfb: prepare for indirect queue mapping scheme`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 54 characters |
| Lowercase after colon | ✅ PASS | |
| Imperative mood | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- Adds `queue_map_rx` and `queue_map_tx` arrays to `pmd_priv`
- Allocates both in a single `rte_calloc()` call with `queue_map_tx =
queue_map_rx + max_rx_queues` - efficient
- Proper error handling with goto cleanup
- Removes unused `rx_queue_id` / `tx_queue_id` from queue structures
- Changes `nfb_eth_rx_queue_init()` and `nfb_eth_tx_queue_init()` signatures
from `uint16_t rx_queue_id` to `int qid`
**Verdict: ✅ Ready for merge**
---
### Patch 2/8: `net/nfb: create ethdev for every eth port/channel`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 52 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- Major architectural change: replaces `rte_eth_dev_pci_generic_probe()` with
multiple `rte_eth_dev_create()` calls
- Uses TAILQ for tracking eth_dev instances for cleanup
- Adds `nfb_eth_common_probe()` and `nfb_eth_common_remove()` as internal
symbols
- Properly uses `nc_ifc_map_info_create_ordinary()` for port/queue mapping info
- `<netcope/info.h>` added for new API
**Issue - `__rte_internal` placement:**
```c
__rte_internal
int nfb_eth_common_probe(struct rte_device *device,
```
Per AGENTS.md: "`__rte_internal` alone on line, only in headers". This is
correct ✅
**Verdict: ✅ Ready for merge**
---
### Patch 3/8: `net/nfb: add vdev as alternative device probe method`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 55 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- New file `nfb_vdev.c` with proper SPDX header
- Uses `rte_kvargs` for argument parsing
- Registers both `net_vdev_nfb` and alias `eth_vdev_nfb`
- Proper error handling with goto-style cleanup
- Uses `nfb_default_dev_path()` for default device
**Minor style note:** The `sprintf()` call at line 897-899 uses
`dev_params_mod` both as target and in format string. This is technically
undefined behavior in C, though it works in practice. Consider using a separate
buffer or `snprintf()` with proper handling.
**Verdict: ⚠️ Minor concern** - The `sprintf(dev_params_mod, "%s%s=%s",
dev_params_mod == dev_params ? "" : ",", ...)` pattern is risky. Consider
refactoring.
---
### Patch 4/8: `net/nfb: add device argument "port" to limit used ports`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 53 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- Adds `port=N` argument support, can be used multiple times
- Uses bitmask for port selection (up to 64 ports via `uint64_t`)
- Proper validation with `fill_port_mask()` callback
- `RTE_PMD_REGISTER_PARAM_STRING` updated for both PCI and vdev drivers
**Code quality:**
```c
if (port >= ULONG_WIDTH)
return -1;
```
Good bounds checking for the bitmask.
**Verdict: ✅ Ready for merge**
---
### Patch 5/8: `net/nfb: init only MACs associated with device`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 49 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- Converts fixed-size arrays `rxmac[RTE_MAX_NC_RXMAC]` to dynamically allocated
arrays
- Removes `RTE_MAX_NC_RXMAC` and `RTE_MAX_NC_TXMAC` defines (no longer needed)
- `nfb_nc_rxmac_init()` / `nfb_nc_txmac_init()` now take `priv` and `params`
and return error codes
- Proper cleanup in `nfb_nc_rxmac_deinit()` / `nfb_nc_txmac_deinit()` with
`rte_free()`
- Error path properly unwinds: `err_txmac_init` → `nfb_nc_rxmac_deinit()` →
`err_rxmac_init`
**Verdict: ✅ Ready for merge**
---
### Patch 6/8: `net/nfb: add compatible cards to driver PCI ID table`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 56 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- Adds new PCI IDs:
- `PCI_VENDOR_ID_CESNET` (0x18ec)
- `PCI_DEVICE_ID_NFB_200G2QL_E1` (secondary endpoint)
- `PCI_DEVICE_ID_FB2CGHH`
- `PCI_DEVICE_ID_COMBO400G1`
- `PCI_DEVICE_ID_CESNET_NDK_COMMON`
- Straightforward addition to PCI table
**Verdict: ✅ Ready for merge**
---
### Patch 7/8: `net/nfb: report firmware version`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 34 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
**Code Review:**
- Implements `fw_version_get` dev_op
- Returns "project_name;project_version" format
- Handles NULL returns from `nc_info_get_fw_project_*()` gracefully
- Returns required buffer size when truncated (per API contract)
**Verdict: ✅ Ready for merge**
---
### Patch 8/8: `doc/nfb: cleanup and update guide`
**Commit Message:**
| Check | Status | Notes |
|-------|--------|-------|
| Subject ≤60 chars | ✅ PASS | 33 characters |
| Lowercase after colon | ✅ PASS | |
| `Signed-off-by:` | ✅ PASS | |
| Body starts with lowercase | ⚠️ WARNING | Body starts with "changes:" which
is unconventional |
**Code Review:**
- Updates `doc/guides/nics/nfb.rst` comprehensively
- Updates `doc/guides/nics/features/nfb.ini` with new features:
- `Unicast MAC filter = Y`
- `Timestamp offload = P`
- `FW version = Y`
- `Multiprocess aware = Y`
- Adds release notes entry in `doc/guides/rel_notes/release_26_03.rst`
**Documentation quality:** Much improved with clearer card list, links to
GitHub and DYNANIC, and better architecture explanation.
**Minor issue:** The commit body format is unusual:
```
changes:
- overall cleanup
- added list of up-to-date cards...
```
Convention is to use prose in imperative mood. Suggest rewording to something
like:
```
Clean up documentation and update the guide with:
- Current card list with firmware download links
- Updated port argument description
```
**Verdict: ⚠️ Minor style issue** in commit body, but content is good
---
## Summary
| Patch | Status | Action Required |
|-------|--------|-----------------|
| 1/8 | ✅ Ready | None |
| 2/8 | ✅ Ready | None |
| 3/8 | ⚠️ Warning | Consider fixing `sprintf()` self-referencing pattern |
| 4/8 | ✅ Ready | None |
| 5/8 | ✅ Ready | None |
| 6/8 | ✅ Ready | None |
| 7/8 | ✅ Ready | None |
| 8/8 | ⚠️ Minor | Consider rewording commit body to use imperative prose |
### Overall Assessment
This is a well-structured series that makes a significant architectural
improvement to the NFB driver. The code quality is good, error handling is
thorough, and the documentation updates are comprehensive. The series properly
includes release notes as required.
**Recommendation:** The series can be merged with the minor fixes noted above,
or accepted as-is given the issues are relatively minor.
