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

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 21 Jan 2026 18:03:25 +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.
> 
> ---

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.