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
> 

Less issues in this but AI did find some errors around checking values.
AI is pickier than is fully required, just figured you might want
to look at these. Since this patch depends on multiport
will defer this until that is updated.

The highest priority item is the unchecked MDIO read in
nfb_mdio_cl45_pma_get_speed_capa() — a failed read gets silently
interpreted as a capability bitmask, which could advertise nonsensical
speeds to applications.

# Review: NFB PMD Enhancement Series (7 Patches)

**Series**: `[PATCH 1/7]` through `[PATCH 7/7]`
**Author**: Martin Spinler `<spinler@cesnet.cz>`
**Delegate**: Stephen Hemminger

This series adds hardware feature support to the NFB PMD, building on the multi-port architecture from the preceding series. It covers: card-assigned MAC addresses, MDIO-based link speed, PMA link up/down control, RS-FEC configuration, RX MTU setting, and MAC-counter-based statistics.

---

### Patch 2/7: `net/nfb: get correct link speed`

Introduces MDIO infrastructure (`nfb_mdio.c` / `nfb_mdio.h`), `eth_node` array in `pmd_internals`, and PMA register-based speed capability and link speed reporting.

**Error: `nfb_mdio_cl45_pma_get_speed_capa()` does not check for MDIO read failure**

```c
reg = nfb_mdio_if_read_pma(info, NFB_MDIO_PMA_SPEED_ABILITY);

for (i = 0; i < NFB_MDIO_WIDTH; i++) {
    if (reg & NFB_MDIO_BIT(i))
        *capa |= speed_ability[i];
}
```

`mdio_read` can return negative values on error. A negative `reg` gets interpreted as a bitmask, producing garbage speed capability flags advertised to the application.

**Fix**: Check `if (reg < 0) return;` before the loop.

**Confidence**: ~85%.

**Warning: `nfb_mdio.h` missing include guard**

The header has no `#ifndef _NFB_MDIO_H_` / `#define _NFB_MDIO_H_` guard. Multiple or transitive inclusion could cause redefinition errors for the `static inline` functions and macros.

**Confidence**: ~80%.

**Warning: `eth_node` population loop lacks bounds check**

```c
intl->eth_node = calloc(ifc->eth_cnt, sizeof(*intl->eth_node));
// ...
eth = 0;
for (i = 0; i < mi->eth_cnt; i++) {
    if (mi->eth[i].ifc != ifc->id)
        continue;
    // ... populate intl->eth_node[eth] ...
    eth++;
}
```

Allocated with `ifc->eth_cnt` elements. If the firmware map has more matching entries, `eth` overflows the allocation. Same pattern exists for `rxm`/`txm` in the base series, but this patch extends it to a third array.

**Confidence**: ~55%.

---

### Patch 3/7: `net/nfb: support real link-up/down config`

Adds PMA reset bit manipulation for link up/down.

**Warning: MACs left in inconsistent state on MDIO failure**

```c
for (i = 0; i < internals->max_rxmac; ++i)
    nc_rxmac_enable(internals->rxmac[i]);
for (i = 0; i < internals->max_txmac; ++i)
    nc_txmac_enable(internals->txmac[i]);
return nfb_eth_dev_set_link(internals, true);
```

If the MDIO PMA write fails, the function returns an error but the MACs have already been enabled. The caller sees failure while the hardware is in a half-configured state (MACs enabled, PMA still in reset). The symmetric problem exists in `set_link_down`.

**Confidence**: ~65%.

---

### Patch 4/7: `net/nfb: support setting RS-FEC mode`

**Error: `nfb_eth_fec_set()` ignores MDIO write return value**

```c
nfb_mdio_if_write_pma(if_info, NFB_MDIO_PMA_RSFEC_CR, reg);
ret = nfb_eth_fec_get(dev, &fec_capa2);
```

The write return value is discarded. If the write fails, the code proceeds to read back and compare — which may coincidentally detect the failure, but the actual error code is lost and the user gets `-EINVAL` instead of `-EIO`.

**Fix**:
```c
ret = nfb_mdio_if_write_pma(if_info, NFB_MDIO_PMA_RSFEC_CR, reg);
if (ret < 0)
    return -EIO;
```

**Confidence**: ~75%.

---

### Patch 5/7: `net/nfb: support setting Rx MTU`

**Warning: `uint16_t` MTU addition can overflow**

```c
mtu += RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
```

`mtu` is `uint16_t`. Adding 18 bytes wraps around near `UINT16_MAX`. The driver sets `max_rx_pktlen = (uint32_t)-1` in `dev_info`, so ethdev's validation won't reject unreasonably large MTU values.

**Fix**: Promote to `uint32_t` before the addition, or set a realistic `max_rx_pktlen` in `dev_info_get`.

**Confidence**: ~70%.

---

### Patch 6/7: `net/nfb: read total stats from macs`

**Warning: `ierrors` subtraction may underflow**

```c
stats->ierrors += rx_cntrs.cnt_drop - rx_cntrs.cnt_overflowed;
```

If firmware counters are inconsistent and `cnt_overflowed > cnt_drop`, this produces a large wrapped unsigned value. A guard would be safer:
```c
if (rx_cntrs.cnt_drop >= rx_cntrs.cnt_overflowed)
    stats->ierrors += rx_cntrs.cnt_drop - rx_cntrs.cnt_overflowed;
```

**Confidence**: ~55%.

---

## Summary

| Severity | Patch | Finding |
|----------|-------|---------|
| **Error** | 2/7 | `nfb_mdio_cl45_pma_get_speed_capa()` doesn't check for negative MDIO read return — garbage speed capabilities on error |
| **Error** | 4/7 | `nfb_eth_fec_set()` ignores MDIO write return value |
| **Warning** | 2/7 | `nfb_mdio.h` missing include guard |
| **Warning** | 2/7 | `eth_node` population loop lacks bounds check against allocated size |
| **Warning** | 3/7 | `set_link_up`/`set_link_down` leaves MACs inconsistent if MDIO fails |
| **Warning** | 5/7 | `uint16_t` MTU overflow; `max_rx_pktlen = (uint32_t)-1` lets it through |
| **Warning** | 6/7 | `ierrors` subtraction can underflow |

Patches 1/7 and 7/7 have no issues.