|WARNING| [v2,4/4] drivers: add DPAA cgrid cleanup support

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/172256

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-30

# DPAA Offline Port Patch Review

## Patch 1/4: bus/dpaa: fix out-of-bounds read of mac_idx array

**Errors:** None

**Warnings:** None

**Info:**
- The patch correctly adds bounds checking for `cell_idx_host` before it is used as an array index.
- The `static_assert` ensures the lookup table size stays synchronized with the constant.
- Error messages use appropriate format specifiers (`PRIu64`) for `uint64_t`.

---

## Patch 2/4: drivers: add offline (O/H) port device support

**Errors:**

1. **Missing error check on `rte_atomic_load_explicit()`**
   Line: `cur_fd = rte_atomic_load_explicit(&ask_fd, rte_memory_order_acquire);`
   The pattern in `check_fd()` loads the fd, checks if it is >= 0, and if not, attempts to open it. However, if the open succeeds and stores the fd, a concurrent reader could observe a torn write on a 32-bit platform where `int` loads are not atomic. The code should use `RTE_ATOMIC(int)` and the result of `rte_atomic_load_explicit()` is already an `int`, so this is correct. However, the error path where `open()` fails and returns -1 does not distinguish between "not yet initialized" and "initialization failed". A subsequent call to `check_fd()` after a failed open will retry the open every time, which is correct but could be optimized. This is acceptable behavior and not an error. **Retracted** - the pattern is safe as written.

2. **Missing initialization of `errno` before `ioctl()` in error paths**
   Lines: Multiple ioctl wrappers save `errno` after ioctl failure.
   The pattern `int err = errno;` followed by a log call that may overwrite `errno` is correct. The comment "Save errno first: the log call may overwrite it." confirms the intent. No error here. **Retracted**.

3. **Resource leak on probe failure in `dpaa_oldev_init()`**
   Line: `goto free_rx;` label only frees `rx_queues` and `tx_queues`, but `eth_dev->data->mac_addrs` allocated just before the return is not freed on the `free_rx` path.
   ```c
   eth_dev->data->mac_addrs = rte_zmalloc("mac_addr",
           RTE_ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER, 0);
   if (eth_dev->data->mac_addrs == NULL) {
       ret = -ENOMEM;
       goto free_rx;  /* BUG: does not free mac_addrs */
   }
   return 0;

   free_rx:
       dpaa_oldev_queues_release(dpaa_intf);
       return ret;
   ```
   The `free_rx` label calls `dpaa_oldev_queues_release()`, which only frees the rx/tx queue arrays. The `mac_addrs` allocation is orphaned. Add `rte_free(eth_dev->data->mac_addrs);` before `dpaa_oldev_queues_release()` on the error path, or add a separate cleanup label.

4. **Offline port advertises Rx burst function but has no actual Rx datapath**
   Line: `eth_dev->rx_pkt_burst = dpaa_eth_queue_rx;`
   The offline port is described as "a tx are queue pair" (sic, "tx area queue pair"?) and its role is to send packets to the kernel and receive from the FMAN back. However, the code assigns `dpaa_eth_queue_rx` as the Rx burst function at init and start. Is this intentional? The patch does not include the `dpaa_eth_queue_rx` implementation for the offline port, so it's unclear if this is correct or if it will attempt to process packets that should not arrive. This may be correct if the offline port does receive packets from the FMAN, but the description is ambiguous. **Clarification needed** - is the offline port's Rx side functional? If not, the Rx burst function should be a no-op stub, not the standard Ethernet Rx function.

5. **FQID leak on `qman_shutdown_fq()` failure in `dpaa_oldev_queues_release()`**
   Lines: 
   ```c
   ret = qman_shutdown_fq(&dpaa_intf->tx_queues[i]);
   if (ret) {
       DPAA_PMD_WARN("Failed(%d) to shutdown txq%d(0x%x), fqid not released",
                     ret, i, dpaa_intf->tx_queues[i].fqid);
       continue;  /* BUG: FQID is not released, but the array is freed */
   }
   qman_release_fqid(dpaa_intf->tx_queues[i].fqid);
   ```
   When `qman_shutdown_fq()` fails, the FQID is not released and the warning says "fqid not released". However, the loop continues and the function returns, freeing the `tx_queues` array. On the next probe, the FQID allocator does not know the FQ is still live, and could hand the same FQID to a new queue, causing a conflict. The code comment says "Leaking it is the safe choice," which is correct. However, the FQID is stored in the `tx_queues[i].fqid` field, which is about to be freed. A subsequent call to `dpaa_oldev_queues_release()` (e.g., from a second close) will not see the leaked FQID. This is not a leak per se, but the comment is misleading: the FQID is not "leaked" in the sense of being permanently lost; it is simply not returned to the allocator, which is the intended behavior. The code is correct. **Retracted** - the pattern is safe; the FQID is not returned to the allocator, which is the correct behavior when the FQ is still live.

**Warnings:**

1. **Missing release notes entry**
   The patch adds a new device type (FSL_DPAA_OL), a new PMD (rte_dpaa_ol_pmd), and new experimental API functions (`rte_pmd_dpaa_ol_set_classif_info`, `rte_pmd_dpaa_ol_reset_classif_info`, `rte_pmd_dpaa_ol_set_lgw_info`, `rte_pmd_dpaa_ol_reset_lgw_info`). This is a new driver and new API, so it should have a release notes entry under "New Features" in `doc/guides/rel_notes/release_26_11.rst`.

2. **New experimental API functions missing testpmd hooks and functional tests**
   The four new `rte_pmd_dpaa_ol_*` functions are marked experimental and exported, but there is no indication in this patch that they are tested in `app/testpmd` or `app/test`. New API functions should have tests.

3. **`dpaa_ol_dev_stop()` returns `int` but `dev_stop` callback should return `int`**
   This is correct per the current ethdev API. No issue. **Retracted**.

4. **`dpaa_ol_dev_close()` closes the control device fd on every close**
   Line: `close(cur_fd);`
   The control device fd is opened once (lazily, on first use) and stored in a static atomic. The `dpaa_ol_dev_close()` callback closes it. If an application has multiple offline ports (though the code only creates one, `oldev1`), closing the first one will close the shared fd, causing subsequent ioctl calls from other ports to fail. However, the code only creates one offline port (see `DPAA_OLDEV_NAME` and the `oldev_devargs` check in `dpaa_create_device_list()`), so this is not a problem in practice. But if the design is extended to support multiple offline ports, this will break. **Accepted as Info** - the single-port assumption is baked into the design (name hardcoded, single devargs check), so closing the fd in the single port's close callback is correct for now. If multiple offline ports are added later, this will need to change.

**Info:**

1. **`dpaa_ol_get_bh_port_name()` returns 1 on success, 0 on not-found, negative on error**
   This is consistent with the pattern in patch 3/4 (`dpaa_get_kernel_ifname()`). The return value is used correctly in `dpaa_ol_set_fq_info()`.

2. **`dpaa_ol_tx_queue_setup()` and `dpaa_ol_rx_queue_setup()` are no-ops for queue configuration**
   The actual queue initialization is deferred to `dpaa_ol_dev_start()` via `dpaa_ol_set_fq_info()`. This is fine; the comment in `dpaa_ol_dev_start()` explains why.

3. **New header `rte_pmd_dpaa_oldev.h` uses `__rte_packed_begin` / `__rte_packed_end` macros**
   These are the correct DPDK wrappers for packed structures, not `__attribute__((packed))`. Good.

4. **New header `rte_pmd_dpaa_oldev.h` documents the experimental status in a @warning block**
   This is the correct Doxygen style for experimental API.

5. **New header lists 26.11 as the version for experimental symbols**
   The patch is dated 2026-10-30, so 26.11 is the next release. However, the version in the `RTE_EXPORT_EXPERIMENTAL_SYMBOL()` macros in `dpaa_oldev.c` is also `26.11`, which is correct.

---

## Patch 3/4: net/dpaa: support non fmX-macY type of shared Ethernet name

**Errors:** None

**Warnings:** None

**Info:**

1. **`dpaa_get_kernel_ifname()` walks `/sys/class/net` to find the netdev**
   The function resolves each netdev's `device/of_node` symlink and compares it against the device tree node path. This is a robust way to find the corresponding kernel interface name for a DPDK port.

2. **Error handling in `dpaa_get_kernel_ifname()` is silent for most failures**
   The function returns 0 (not found) for any failure to open the directory, resolve a symlink, or match the node path. This is fine; the caller logs the failure if needed.

3. **`dpaa_eth_dev_configure()` falls back to the DPDK port name if sysfs lookup fails**
   Line: `if (ret <= 0 || sh_if_name[0] == '\0')` followed by `strlcpy(ifr.ifr_name, dpaa_intf->name, IFNAMSIZ);`
   This is the correct fallback for `fmX-macY` style ports.

---

## Patch 4/4: drivers: add DPAA cgrid cleanup support

**Errors:**

1. **`qman_pending_fq_by_cgrid_range()` walks the entire FQID space up to `QMAN_MAX_FQID`**
   Line: `for (; fq.fqid <= QMAN_MAX_FQID; fq.fqid++)`
   The loop increments `fq.fqid` by 1 on each iteration, and `QMAN_MAX_FQID` is `0x00FFFFFFu` (16777215). This is a large range (16M FQIDs), and the loop does not break early if no matching FQ is found until the `ERANGE` from `qman_query_fq_np()`. The function is only called from the device close path, so the cost is acceptable (as the comment notes), but this is a slow operation (16M ioctl calls in the worst case). The loop does break on `-ERANGE` from `qman_query_fq_np()`, which is returned when the FQID is not implemented, so the loop will not actually run 16M times on a device with fewer FQIDs. The comment confirms this: "FQID is not implemented on this device, so there is nothing beyond it either." This is correct. **Retracted** - the pattern is safe; the loop breaks on the first unimplemented FQID.

2. **`qman_pending_fq_by_cgrid_range()` queries both `fq_np` and `fqd` state**
   The function calls `qman_query_fq_np()` first to check if the FQ is out-of-service (OOS). If not, it calls `qman_query_fq()` to get the full FQD and check if the CGE bit is set and the cgid matches. An FQ in OOS state cannot be a member of a CGR (it has no traffic), so skipping the second query is an optimization. However, the check `(np.state & QM_MCR_NP_STATE_MASK) != QM_MCR_NP_STATE_OOS` inverts the test: it queries the FQD only if the FQ is *not* OOS. An OOS FQ is skipped. This is correct. **Retracted** - the logic is correct.

3. **`dpaa_cgr_stale_fq_cleanup()` returns `false` on `qman_shutdown_fq()` failure**
   Line: `return false;` when `qman_shutdown_fq_by_fqid(fqid)` fails.
   The function is called from `dpaa_eth_dev_close()`, and the return value controls whether the CGRIDs are released. If a stale FQ refuses to shut down, the function returns `false`, and the caller does not release the CGRIDs. This is correct: a CGRID that may still be referenced by a live FQ must not be reused. However, the caller then proceeds to shut down and delete the port's own FQs and CGRs. If one of the port's own FQs fails to shut down, the CGRID is also not released, but the FQ is still in the `rx_queues` or `tx_queues` array, which is about to be freed. The FQID for that FQ is also not released. The code in `dpaa_eth_dev_close()` does handle this correctly: it only calls `qman_release_cgrid_range()` if `cgrid_ok` is true, which is set to false if any FQ fails to shut down. This is correct. **Retracted** - the pattern is safe.

**Warnings:** None

**Info:**

1. **`qman_pending_fq_by_cgrid_range()` matches a whole CGRID range in one scan**
   The comment explains that this avoids one full FQID space scan per CGR, which is important for ports with many congestion groups. Good optimization.

2. **`dpaa_eth_dev_close()` now shuts down the port's own FQs before deleting the CGRs**
   This is the correct sequence: a CGR must have no members when deleted, and the FQs are members, so they must be shut down (unlinked from the CGR) before the CGR is deleted.

3. **`dpaa_dev_init()` error path now cleans up partially initialized CGRs**
   The probe error path now tracks how many CGRs were successfully created (`nb_rx_cgr`, `nb_tx_cgr`) and deletes only those. It also shuts down the FQs linked to them before deleting the CGRs, which is correct.

4. **`dpaa_dev_init()` error path only releases CGRIDs if all FQs and CGRs were cleaned up**
   The `rx_cgrid_allocated` and `tx_cgrid_allocated` flags track whether the CGRIDs were allocated, and the error path only calls `qman_release_cgrid_range()` if the FQs were shut down and the CGRs were deleted. This matches the pattern in `dpaa_eth_dev_close()`.

---

## Summary

**Patch 1/4:** CLEAN

**Patch 2/4:**
- **Error:** Resource leak on probe failure: `eth_dev->data->mac_addrs` is not freed on the `free_rx` error path in `dpaa_oldev_init()`.
- **Warning:** Missing release notes entry for new driver and new experimental API.
- **Warning:** New experimental API functions (`warning: review has no Review-Result line, severity inferred from prose (response may be truncated)