On Thu, 22 Jan 2026 08:27:11 +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.
>
> ---
> v4:
> * Cleaned usage of sprintf
> * Updated commit message for doc cleanup
=== Patch Review: bundle-1675-nfb-multiport.mbox (via Claude) ===
# DPDK Patch Review: NFB Multi-port Support
## ERRORS
### 1. Commit Message - Missing Fixes Tag (Patch 1/8)
**File:** Patch 1/8 - "net/nfb: prepare for indirect queue mapping scheme"
**Issue:** The commit body states "This prepares queue mapping for port-aware
driver implementation" which suggests this is fixing or preparing for
something, but there's no explanation of what problem this solves. While not
strictly a bug fix requiring a `Fixes:` tag, the commit message should better
explain the rationale.
**Recommendation:** Expand the commit body to explain why the indirect mapping
is needed before implementing multi-port support.
---
### 2. Forbidden Token - RTE_EXPORT_INTERNAL_SYMBOL (Patch 2/8)
**File:** drivers/net/nfb/nfb_ethdev.c (Line after nfb_eth_common_probe
definition)
**Location:** Lines 667, 780
```c
RTE_EXPORT_INTERNAL_SYMBOL(nfb_eth_common_probe)
int
nfb_eth_common_probe(struct rte_device *device,
...
RTE_EXPORT_INTERNAL_SYMBOL(nfb_eth_common_remove)
int
nfb_eth_common_remove(struct rte_device *dev)
```
**Issue:** According to AGENTS.md, `__rte_internal` must appear **alone on the
line** immediately preceding the return type, and only in header files.
`RTE_EXPORT_INTERNAL_SYMBOL` is the wrong way to mark internal APIs.
**Required Fix:** Move the `__rte_internal` attribute to the function
declarations in `nfb.h`:
```c
// In nfb.h
__rte_internal
int nfb_eth_common_probe(struct rte_device *device,
ethdev_bus_specific_init specific_init, void *specific_device,
struct nfb_init_params *params, int ep_index);
__rte_internal
int nfb_eth_common_remove(struct rte_device *dev);
```
And remove the `RTE_EXPORT_INTERNAL_SYMBOL` macros from the .c file.
---
### 3. Memory Leak on Error Path (Patch 2/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Function:** nfb_eth_common_probe
**Location:** Lines 713-735
```c
nc_ifc_map_info_create_ordinary(nfb_dev, ¶ms->map_info);
if (params->args != NULL && strlen(params->args) > 0) {
kvlist = rte_kvargs_parse(params->args, VALID_KEYS);
if (kvlist == NULL) {
NFB_LOG(ERR, "Failed to parse device arguments %s", params->args);
return -EINVAL; // ← Leaks map_info and nfb_dev
}
// ... more code ...
if (port_mask == 0) {
NFB_LOG(ERR, "Failed to parse device port argument");
return -EINVAL; // ← Leaks map_info and nfb_dev
}
}
```
**Issue:** Early returns leak `params->map_info` (allocated by
`nc_ifc_map_info_create_ordinary`) and don't close `nfb_dev`.
**Required Fix:**
```c
if (kvlist == NULL) {
NFB_LOG(ERR, "Failed to parse device arguments %s", params->args);
ret = -EINVAL;
goto err_parse_args;
}
// ...
if (port_mask == 0) {
NFB_LOG(ERR, "Failed to parse device port argument");
ret = -EINVAL;
goto err_parse_args;
}
// At end of function, before existing cleanup:
err_parse_args:
nc_map_info_destroy(¶ms->map_info);
nfb_close(nfb_dev);
return ret;
```
---
### 4. Incorrect Line Length (Patch 2/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Location:** Line 802
```c
return nfb_eth_common_probe(&pci_dev->device,
eth_dev_pci_specific_init, pci_dev, ¶ms,
```
**Issue:** This line exceeds 100 characters (appears to be ~105 characters
based on context). C code lines must not exceed 100 characters.
**Required Fix:** Break the line appropriately:
```c
return nfb_eth_common_probe(&pci_dev->device,
eth_dev_pci_specific_init, pci_dev, ¶ms,
comp_dev_info.ep_index);
```
---
### 5. Missing Release Notes for API Changes (Patches 2-8)
**File:** doc/guides/rel_notes/release_26_03.rst
**Location:** Only appears in Patch 8/8
**Issue:** Per AGENTS.md: "Changes to existing APIs require release notes" and
"New drivers or subsystems must have release notes." The release notes are
added in patch 8/8, but significant driver behavior changes occur in patches
2-5. According to AGENTS.md, "Code and documentation must be updated atomically
in same patch."
**Required Fix:** Move relevant portions of the release notes update to the
patches where the changes actually occur:
- Patch 2/8: Note about one ethdev per port
- Patch 4/8: Note about the `port` argument
- Patch 7/8: Note about firmware version reporting
---
## WARNINGS
### 1. Commit Message Body - Starts with "It" (Patch 1/8)
**Issue:** Per AGENTS.md, commit message bodies must not start with "It". While
this patch doesn't explicitly start with "It", the body is minimal and doesn't
provide sufficient context.
**Current:**
```
This prepares queue mapping for port-aware driver implementation.
```
**Recommendation:**
```
Introduce indirect queue mapping to support multiple ethdev ports.
The NFB driver is being enhanced to create one ethdev per physical
Ethernet port instead of one ethdev per PCI device. This requires
an indirect mapping layer because DPDK queue indices no longer
directly correspond to firmware queue indices.
This change adds queue_map_rx and queue_map_tx arrays to track the
mapping between DPDK queue indices and firmware queue IDs, preparing
for the multi-port implementation in subsequent patches.
```
---
### 2. Subject Line Length (Patch 2/8)
**File:** Patch 2/8 subject
**Current:** "net/nfb: create ethdev for every eth port/channel" (50 chars)
**Issue:** While under the 60-character limit, the subject could be more
descriptive about the impact.
**Recommendation:** Consider:
```
net/nfb: create one ethdev per ethernet port
```
(More clear about the 1:1 relationship)
---
### 3. Inappropriate Use of rte_zmalloc (Patch 2/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Location:** Line 585
```c
priv->queue_map_rx = rte_calloc("NFB queue map", (max_rx_queues +
max_tx_queues),
sizeof(*priv->queue_map_rx), 0);
```
**Issue:** Per AGENTS.md "Appropriate Use of rte_malloc()": Use `rte_malloc()`
only for DMA-accessible memory or memory shared between processes. Control
structures should use standard `malloc()`.
**Reasoning:** Queue mapping arrays are control plane data structures that
don't require hugepage memory. Using `rte_calloc` wastes limited hugepage
resources.
**Recommendation:**
```c
priv->queue_map_rx = calloc((max_rx_queues + max_tx_queues),
sizeof(*priv->queue_map_rx));
```
And use `free()` in cleanup path instead of `rte_free()`.
---
### 4. Unnecessary NULL Check Before free (Patches 1/8, 5/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Location:** Multiple locations
**Patch 1/8, Line 655:**
```c
if (rte_eal_process_type() == RTE_PROC_PRIMARY)
rte_free(priv->queue_map_rx);
```
**Issue:** The `rte_eal_process_type()` check is appropriate (different process
model), but similar patterns elsewhere may be checking for NULL unnecessarily.
**Patch 5/8, Lines 118-122:**
```c
for (i = 0; i < priv->max_rxmac; i++)
nc_rxmac_close(priv->rxmac[i]);
rte_free(priv->rxmac);
```
**Potential Issue:** If any `priv->rxmac[i]` could be NULL, the code should
check before calling `nc_rxmac_close`, but this depends on `nc_rxmac_close`
behavior (not visible in patch).
**Recommendation:** Verify that `nc_rxmac_close` and `nc_txmac_close` handle
NULL pointers safely, or add NULL checks:
```c
for (i = 0; i < priv->max_rxmac; i++) {
if (priv->rxmac[i] != NULL)
nc_rxmac_close(priv->rxmac[i]);
}
```
---
### 5. Missing Documentation for New Struct Member (Patch 1/8)
**File:** drivers/net/nfb/nfb.h
**Location:** Lines 68-69
```c
struct pmd_priv {
uint16_t max_rx_queues;
uint16_t max_tx_queues;
int *queue_map_rx;
int *queue_map_tx;
};
```
**Issue:** New fields lack Doxygen-style comments explaining their purpose,
while the file shows other structures are documented.
**Recommendation:**
```c
struct pmd_priv {
uint16_t max_rx_queues;
uint16_t max_tx_queues;
/** Mapping from DPDK RX queue index to firmware queue ID */
int *queue_map_rx;
/** Mapping from DPDK TX queue index to firmware queue ID */
int *queue_map_tx;
};
```
---
### 6. Global Variable Without Unique Prefix (Patch 2/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Location:** Lines 28-29
```c
TAILQ_HEAD(pmd_internals_head, pmd_internals);
static struct pmd_internals_head nfb_eth_dev_list =
TAILQ_HEAD_INITIALIZER(nfb_eth_dev_list);
```
**Issue:** Per AGENTS.md "Symbol Naming for Static Linking": While this is a
`static` variable (file scope) and doesn't strictly violate the guideline, the
type name `pmd_internals_head` is generic and could clash if the static is
removed in the future.
**Recommendation:** Use more specific names:
```c
TAILQ_HEAD(nfb_pmd_internals_head, pmd_internals);
static struct nfb_pmd_internals_head nfb_eth_dev_list =
TAILQ_HEAD_INITIALIZER(nfb_eth_dev_list);
```
---
### 7. Inconsistent Error Handling Pattern (Patch 4/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Function:** fill_port_mask
**Location:** Lines 672-686
```c
static int fill_port_mask(const char *key __rte_unused, const char *value, void
*opaque)
{
char *end;
uint64_t *port_mask = opaque;
unsigned long port;
if (value == NULL || strlen(value) == 0 || !isdigit(*value))
return -1;
port = strtoul(value, &end, 0);
if (*end != '\0' || port >= ULONG_WIDTH)
return -1;
```
**Issue:** Function returns generic `-1` instead of standard error codes like
`-EINVAL`. This is inconsistent with DPDK style where functions typically
return `-errno` values.
**Recommendation:**
```c
if (value == NULL || strlen(value) == 0 || !isdigit(*value))
return -EINVAL;
port = strtoul(value, &end, 0);
if (*end != '\0' || port >= ULONG_WIDTH)
return -EINVAL;
```
---
### 8. Potential Integer Overflow (Patch 4/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Location:** Line 728
```c
if (ret || port_mask >= RTE_BIT64(params->map_info.ifc_cnt))
port_mask = 0;
```
**Issue:** If `params->map_info.ifc_cnt >= 64`, then
`RTE_BIT64(params->map_info.ifc_cnt)` will overflow (undefined behavior for
shift >= 64).
**Recommendation:**
```c
if (ret || params->map_info.ifc_cnt > 64 ||
(params->map_info.ifc_cnt < 64 && port_mask >=
RTE_BIT64(params->map_info.ifc_cnt)))
port_mask = 0;
```
Or add a check earlier:
```c
if (params->map_info.ifc_cnt > 64) {
NFB_LOG(ERR, "Too many interfaces: %u", params->map_info.ifc_cnt);
return -EINVAL;
}
```
---
### 9. Documentation Not Matching Code (Patch 8/8)
**File:** doc/guides/nics/nfb.rst
**Location:** Lines 58-60
```rst
.. note::
Normally, one port corresponds to one channel, but ports can often be
configured in a separate
manner. For example one 100G port can be used as 4x25G or 4x10G independent
Ethernet channels.
```
**Issue:** The documentation states "Normally, one port corresponds to one
channel" but doesn't explain what a "channel" is in the context of the firmware
vs DPDK ports, which could confuse users.
**Recommendation:** Clarify the terminology:
```rst
.. note::
The driver creates one DPDK port for each firmware Ethernet interface. Some
cards support
port breakout, where a physical port can be reconfigured (e.g., 1x100G to
4x25G). Each
resulting channel appears as a separate DPDK port.
```
---
### 10. Missing NULL Checks After rte_zmalloc (Patch 2/8)
**File:** drivers/net/nfb/nfb_ethdev.c
**Location:** Line 537
```c
internals = rte_zmalloc_socket("nfb_internals",
sizeof(struct pmd_internals), RTE_CACHE_LINE_SIZE,
rte_socket_id());
if (internals == NULL) {
NFB_LOG(ERR, "Failed to allocate private data");
ret = -ENOMEM;
goto err_nfb_open;
}
```
**Issue:** The error path `err_nfb_open` doesn't check if `internals` is NULL
before potentially dereferencing it in cleanup. While this specific code path
is safe, it's worth noting for consistency.
**Status:** Currently safe, but should be verified if cleanup code changes.
---
## INFO
### 1. Code Style - Variable Declaration at Point of Use (Multiple Patches)
**Observation:** The patches use both C89-style (declarations at start of
block) and C99-style (declarations at point of use) variable declarations.
**Example (Patch 1/8):**
```c
int ret;
int qid;
struct ndp_rx_queue *rxq;
```
vs.
**Example (Patch 5/8):**
```c
for (i = 0, j = 0
