[REVIEW] doc/nfb: cleanup and update guide

[Stephen Hemminger <stephen@networkplumber.org>]

AI-generated review of bundle-1720-nfb-multiport.mbox
Reviewed using Claude (claude-opus-4-5-20251101) on 2026-02-02

This is an automated review. Please verify all suggestions.

---

# DPDK Patch Review: net/nfb Multi-port Series (v6)

## Overview
This is an 8-patch series that enhances the NFB driver to create one ethdev per physical Ethernet port instead of one ethdev per PCI device.

---

## Patch 1/8: net/nfb: prepare for indirect queue mapping scheme

### Errors
None.

### Warnings

1. **Line length exceeds 100 characters** (nfb_ethdev.c)
   - Line 580: `priv->queue_map_rx = rte_calloc("NFB queue map", (max_rx_queues + max_tx_queues),`
   - Split across multiple lines to stay within 100 characters.

### Info
- Clean preparation patch that adds queue mapping infrastructure for the multi-port implementation.

---

## Patch 2/8: net/nfb: create one ethdev per ethernet port

### Errors

1. **`__rte_internal` functions in header file without the tag appearing alone on its own line** (nfb.h:89-92)
   ```c
   __rte_internal
   int nfb_eth_common_probe(struct nfb_probe_params *params);
   __rte_internal
   int nfb_eth_common_remove(struct rte_device *dev);
   ```
   The `__rte_internal` tag should be alone on its line, but the function signature should have the return type on a separate line per DPDK style:
   ```c
   __rte_internal
   int
   nfb_eth_common_probe(struct nfb_probe_params *params);
   ```

### Warnings

1. **Extra blank line** (nfb.h:93)
   - There are two blank lines before the `__rte_internal` declaration; one is sufficient.

2. **Line exceeds 100 characters** (nfb_ethdev.c:704)
   ```c
   ret = snprintf(pp->name + cp->basename_len, sizeof(pp->name) - cp->basename_len,
   ```
   This line is approximately 101 characters.

### Info
- Good refactoring to support multi-port architecture.

---

## Patch 3/8: net/nfb: add vdev as alternative device probe method

### Errors
None.

### Warnings

1. **Unnecessary cast of void pointer** (nfb_vdev.c:62)
   ```c
   ret = rte_kvargs_process(kvlist, NFB_ARG_PORT,
           nfb_eth_dev_create_for_ifc_by_port, (void *)&ifc_params);
   ```
   The cast `(void *)` is unnecessary in C.

2. **Missing newline at end of file** (nfb_vdev.c)
   - The file appears to end without a trailing newline after line 108.

### Info
- Useful addition for virtual/simulated NFB devices.

---

## Patch 4/8: net/nfb: add device argument "port" to limit used ports

### Errors
None.

### Warnings

1. **Unnecessary cast of void pointer** (nfb_ethdev.c:774)
   ```c
   nfb_eth_dev_create_for_ifc_by_port, (void *)&ifc_params);
   ```
   The cast is unnecessary.

### Info
- Good feature addition for selecting specific ports.

---

## Patch 5/8: net/nfb: init only MACs associated with device

### Errors
None.

### Warnings

1. **Using `calloc()` instead of `rte_calloc()`** (nfb_ethdev.c:74-78)
   ```c
   intl->rxmac = calloc(ifc->eth_cnt, sizeof(*intl->rxmac));
   ...
   intl->txmac = calloc(ifc->eth_cnt, sizeof(*intl->txmac));
   ```
   For consistency within the driver and proper NUMA-aware allocation, consider using `rte_calloc()` or `rte_zmalloc()`. However, since these are control structures (not DMA-accessible), standard `calloc()` is acceptable.

2. **Using `free()` instead of `rte_free()`** (nfb_ethdev.c:114-115)
   ```c
   free(intl->txmac);
   free(intl->rxmac);
   ```
   Should match the allocation function. If using `calloc()`, then `free()` is correct.

### Info
- Properly scopes MAC initialization to the associated device.

---

## Patch 6/8: net/nfb: add compatible cards to driver PCI ID table

### Errors
None.

### Warnings
None.

### Info
- Straightforward addition of PCI IDs for additional compatible cards.

---

## Patch 7/8: net/nfb: report firmware version

### Errors
None.

### Warnings
None.

### Info
- Clean implementation of firmware version reporting.

---

## Patch 8/8: doc/nfb: cleanup and update guide

### Errors
None.

### Warnings

1. **HTTP link instead of HTTPS** (nfb.rst:20)
   ```rst
   `historical cards <https://www.liberouter.org/technologies/cards/>`_
   ```
   This appears to be HTTPS, which is correct. No issue.

### Info
- Documentation updates align well with the code changes.
- Release notes properly document the new features.

---

## Summary

### Errors (Must Fix)
1. **Patch 2**: `__rte_internal` function declarations should have return type on separate line per DPDK style.

### Warnings (Should Fix)
1. **Patch 1**: Line exceeds 100 characters in nfb_ethdev.c.
2. **Patch 2**: Extra blank line in nfb.h; line exceeds 100 characters.
3. **Patches 3, 4**: Unnecessary `(void *)` cast.
4. **Patch 3**: Verify file ends with newline.

### General Observations
- The patch series is well-structured with clear separation of concerns.
- Commit messages are well-written and follow DPDK conventions.
- The code changes are logical and the multi-port architecture is cleanly implemented.
- Documentation is updated appropriately with release notes.