Re: [PATCH 0/7] net/nfb: ethernet enhancements

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri,  6 Feb 2026 18:04:28 +0100
spinler@cesnet.cz wrote:

> From: Martin Spinler <spinler@cesnet.cz>
> 
> 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.