On Fri, 6 Feb 2026 18:04:28 +0100
[email protected] wrote:
> From: Martin Spinler <[email protected]>
>
> This set includes minor enhancements to the Ethernet
> configuration and overall usage.
>
> ---
> Depends-on: series-37261 ("net/nfb: rework to real multiport")
>
> Martin Spinler (7):
> net/nfb: use MAC address assigned to card
> net/nfb: get correct link speed
> net/nfb: support real link-up/down config
> net/nfb: support setting RS-FEC mode
> net/nfb: support setting Rx MTU
> net/nfb: read total stats from macs
> doc/nfb: update release notes for nfb driver
>
> doc/guides/rel_notes/release_26_03.rst | 3 +-
> drivers/net/nfb/meson.build | 1 +
> drivers/net/nfb/nfb.h | 7 +
> drivers/net/nfb/nfb_ethdev.c | 228 +++++++++++++++++++++----
> drivers/net/nfb/nfb_mdio.c | 42 +++++
> drivers/net/nfb/nfb_mdio.h | 34 ++++
> drivers/net/nfb/nfb_stats.c | 40 +++--
> 7 files changed, 308 insertions(+), 47 deletions(-)
> create mode 100644 drivers/net/nfb/nfb_mdio.c
> create mode 100644 drivers/net/nfb/nfb_mdio.h
>
The CI and I don't have hardware to do deep check so have to rely
on my and AI review.
Bundle 1747 (Feature Additions - 7 patches)
3 Critical Issues:
Patch 2: MDIO resource leak on error path - opened handles not closed
Patch 3: Missing NULL check before accessing eth_node[0]
Patch 4: Same NULL check issue in FEC get/set functions
Quick Summary: The feature additions are mostly solid. The main issue is a
consistent pattern of accessing eth_node[0] without verifying the pointer is
non-NULL. One resource leak needs cleanup code added.
Detailed output:
# NFB Driver Review - Bundle 1747 (Feature Additions)
## Series: 7 patches adding MAC, link speed, FEC, MTU, and stats features
---
## PATCH 1/7: net/nfb: use MAC address assigned to card
**Subject**: net/nfb: use MAC address assigned to card (47 chars) ✓
### Summary
Retrieves MAC address from card firmware via `nc_ifc_get_default_mac()` instead
of always generating random MAC with OUI prefix.
### Status: ✓ No errors found
This patch is clean. Falls back gracefully to random MAC generation if firmware
MAC retrieval fails.
---
## PATCH 2/7: net/nfb: get correct link speed
**Subject**: net/nfb: get correct link speed (38 chars) ✓
### Summary
Adds MDIO infrastructure to read accurate link speed from PHY PMA registers
instead of MAC status register. Introduces `struct eth_node` with MDIO
interface info.
### Errors
**Error 1: Resource leak on error path**
```c
intl->eth_node[eth].if_info.dev = nc_mdio_open(intl->nfb, node, node_cp);
if (intl->eth_node[eth].if_info.dev) {
// ... set up interface info
eth++;
}
```
Location: `nfb_nc_eth_init()`, loop at line ~126
If `nc_mdio_open()` succeeds for some iterations but then a later error occurs
(e.g., memory allocation for subsequent MACs fails), the function jumps to
error labels `err_alloc_ethnode`, `err_alloc_txmac`, or `err_alloc_rxmac`. None
of these labels close the already-opened MDIO handles.
**Suggested fix**: Add cleanup loop before freeing eth_node:
```c
err_alloc_ethnode:
for (i = 0; i < eth; i++)
if (intl->eth_node[i].if_info.dev)
nc_mdio_close(intl->eth_node[i].if_info.dev);
free(intl->eth_node);
err_alloc_txmac:
free(intl->txmac);
err_alloc_rxmac:
free(intl->rxmac);
return ret;
```
---
## PATCH 3/7: net/nfb: support real link-up/down config
**Subject**: net/nfb: support real link-up/down config (47 chars) ✓
### Summary
Implements actual PHY link control via MDIO PMA reset register
(`NFB_MDIO_PMA_CTRL`) instead of just enabling/disabling MAC layer.
### Errors
**Error 1: Missing NULL check before array access**
```c
static int
nfb_eth_dev_set_link(struct pmd_internals *intl, bool val)
{
// ...
if (!intl->max_eth)
return 0;
if_info = &intl->eth_node[0].if_info; // <-- dereference without NULL check
```
Location: `nfb_eth_dev_set_link()`
If `intl->eth_node` is NULL (allocation failed but not caught), this crashes.
While initialization should guarantee that `max_eth > 0` implies `eth_node !=
NULL`, defensive coding requires verification.
**Suggested fix**: Add NULL check:
```c
if (!intl->max_eth || !intl->eth_node)
return 0;
if_info = &intl->eth_node[0].if_info;
```
---
## PATCH 4/7: net/nfb: support setting RS-FEC mode
**Subject**: net/nfb: support setting RS-FEC mode (44 chars) ✓
### Summary
Implements FEC (Forward Error Correction) get/set operations via MDIO PMA
RS-FEC control register. Supports RS-FEC enable/disable; rejects AUTO mode.
### Errors
**Error 1: Same NULL check issue as Patch 3**
```c
static int
nfb_eth_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
{
// ...
if (!intl->max_eth)
return 0;
if_info = &intl->eth_node[0].if_info; // <-- dereference without NULL check
```
Location: `nfb_eth_fec_get()` and `nfb_eth_fec_set()`
**Suggested fix**: Add NULL check in both functions:
```c
if (!intl->max_eth || !intl->eth_node)
return 0;
```
---
## PATCH 5/7: net/nfb: support setting Rx MTU
**Subject**: net/nfb: support setting Rx MTU (38 chars) ✓
### Summary
Implements MTU setting by querying maximum frame length capability from all RX
MACs, taking minimum, and configuring all MACs with requested MTU.
### Status: ✓ No errors found
The logic correctly handles the case of no RX MACs (`if (!intl->max_rxmac)
return 0`), queries capability limits, validates against minimum capability,
and applies MTU to all MACs.
One minor observation: `status.frame_length_max_capable` is initialized to 0,
and the function checks `frame_length_max_capable != 0` before rejecting MTU.
This allows unlimited MTU if all MACs report 0 capability (meaning unlimited).
This seems intentional.
---
## PATCH 6/7: net/nfb: read total stats from macs
**Subject**: net/nfb: read total stats from macs (41 chars) ✓
### Summary
Aggregates statistics from all MAC instances instead of reading from queues.
Changes `nfb_eth_stats_get()` to sum across all RX/TX MACs.
### Status: ✓ No errors found
The aggregation logic is straightforward and correct. Iterates over all MACs
and accumulates statistics into the stats structure.
---
## PATCH 7/7: doc/nfb: update release notes for nfb driver
**Subject**: doc/nfb: update release notes for nfb driver (52 chars) ✓
### Status: ✓ Not reviewed (documentation)
Documentation patch - should verify that release notes accurately describe all
changes in the series.
---
## Summary - Bundle 1747
### Critical Issues: 3
1. **Patch 2/7**: Resource leak - MDIO handles not closed on error path in
`nfb_nc_eth_init()`
2. **Patch 3/7**: Missing NULL check before accessing `eth_node[0]` in
`nfb_eth_dev_set_link()`
3. **Patch 4/7**: Missing NULL check before accessing `eth_node[0]` in
`nfb_eth_fec_get()` and `nfb_eth_fec_set()`
### All Patches: Format ✓
- Proper Signed-off-by tags ✓
- Subject lines within 60 chars ✓
- Commit message format correct ✓
---
## Recommended Actions
### Must Fix:
1. **Patch 2**: Add MDIO handle cleanup loop on error paths
2. **Patches 3-4**: Add NULL check for `eth_node` before accessing array element
### Pattern to Apply:
For all functions accessing `intl->eth_node[0]`, use:
```c
if (!intl->max_eth || !intl->eth_node)
return 0; // or appropriate error code
```
This defensive pattern prevents crashes if initialization invariants are
violated.
