|WARNING| [v1,3/3] drivers: add DPAA cgrid cleanup support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/3: drivers: add offline (O/H) port device support

### Errors

None.

### Warnings

1. **`drivers/net/dpaa/dpaa_oldev.c:54` - Non-const function pointer array**

```c
static RTE_ATOMIC(int)ask_fd = -1;
```

The atomic `ask_fd` control descriptor is correct, but later in the file the dev_ops structure is assigned to `eth_dev->dev_ops` which is fine. However, this is a minor observation about the pattern of assignment.

2. **`drivers/net/dpaa/dpaa_oldev.c:586-587` - Queue release on close duplicates error path cleanup**

The `dpaa_ol_dev_close()` function calls `dpaa_oldev_queues_release()` to free tx/rx queues, and the comment explicitly states this is "so that an application that only calls rte_eth_dev_close() (without removing the device) still shuts down the FQs". This duplicates the same cleanup that `dpaa_ol_remove()` would do through `dpaa_ol_dev_close()` again. While not a bug per se (the function checks for NULL), the pattern means `dpaa_oldev_queues_release()` is called twice on a normal remove path. Consider whether the nulling pattern is sufficient or if there's a cleaner separation.

3. **`drivers/net/dpaa/dpaa_oldev.c:717-733` - Resource leak on queue setup failure**

If `dpaa_ol_tx_queue_init()` in `dpaa_oldev_init()` fails after `dpaa_ol_rx_queue_init()` succeeds, the error path `goto free_rx` will call `dpaa_oldev_queues_release()` which only attempts to shut down queues whose `fqid != 0`. The Rx queue was initialized and has a non-zero FQID, but since `dpaa_ol_rx_queue_init()` creates the FQ with `QMAN_FQ_FLAG_DYNAMIC_FQID`, the FQID was allocated by QMan and must be explicitly released via `qman_release_fqid()` on failure. The existing shutdown+release pattern in `dpaa_oldev_queues_release()` is correct for the close path, but the probe error path needs to ensure FQIDs are released even if shutdown succeeds.

However, reviewing the full `dpaa_oldev_queues_release()` implementation shows it does call `qman_release_fqid()` for each queue after `qman_shutdown_fq()` succeeds, so this is handled correctly. No change needed.

4. **`drivers/net/dpaa/rte_pmd_dpaa_oldev.h:23` - Missing `__rte_experimental` on experimental symbol exports**

The four exported functions are marked `__rte_experimental` in the header, which is correct.

5. **`drivers/bus/dpaa/dpaa_bus.c:87-121` - Bus argument parsing without error propagation**

The `dpaa_bus_parse_bus_args()` function walks `RTE_EAL_DEVARGS_FOREACH` and parses kvargs, but it always returns 0 even when individual `rte_kvargs_parse()` calls fail (the failure is silently ignored with `continue`). While this may be intentional (to allow other devargs to succeed), it means invalid bus arguments are silently dropped. Consider whether parse failures should be logged or returned.

6. **`drivers/net/dpaa/dpaa_flow.c:38-40` - Redundant `static_assert` with identical condition**

Both `drivers/bus/dpaa/base/fman/fman.c:207-208` and `drivers/net/dpaa/dpaa_flow.c:38-40` contain the same `static_assert(RTE_DIM(mac_idx) == FMAN_MAX_MAC_IDX_NUM, ...)`. This is fine for defense in depth, but the comment in `dpaa_flow.c` that says "indexed by fman_if.mac_idx, which the bus validates" now refers to the bounds check added in `fman.c` by this patch, making the second assert somewhat redundant. Not an error, but worth noting.

### Info

1. **`drivers/bus/dpaa/base/fman/fman.c:547-555` and `fman.c:971-979` - Cell index validation**

The patch adds bounds checking on `cell_idx_host` against `FMAN_MAX_MAC_IDX_NUM` before assigning to `__if->__if.mac_idx`. This is a good safety improvement. The error messages are clear and include the node name, cell-index value, and limit.

2. **`drivers/net/dpaa/dpaa_oldev.c:176-181` - errno saved before log call**

The pattern of saving `errno` immediately after `ioctl()` before calling logging functions is correct and follows the guideline (log calls may overwrite errno).

3. **`drivers/net/dpaa/dpaa_oldev.c:491-500` - drv_bh_port devarg validation**

The function `dpaa_ol_get_bh_port_name()` returns 1 on success, 0 if not found, and negative on error (e.g., `-ENAMETOOLONG`). The caller checks `ret <= 0` and returns `-EINVAL`, which correctly catches both the "not found" (0) and error (<0) cases and maps them to a single error for the calling context.

4. **`drivers/net/dpaa/dpaa_oldev.c:808-809` - Secondary process exclusion**

The offline port's frame queues use dynamic FQIDs and kernel control device interaction that is owned by the primary process. The check in `dpaa_ol_probe()` correctly refuses to probe in secondary processes and logs a clear warning. This is appropriate given the architectural constraints.

5. **`drivers/net/dpaa/dpaa_oldev.c:857-858` - Remove idempotency**

The `dpaa_ol_remove()` function checks `rte_eth_dev_allocated()` before attempting to close and release, making it safe to call even if the device was never successfully probed. This is good defensive practice.

---

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

### Errors

None.

### Warnings

1. **`drivers/net/dpaa/dpaa_ethdev.c:60-105` - Resource leak on readdir failure**

The function `dpaa_get_kernel_ifname()` opens a directory with `opendir()` and walks it with `readdir()`. If the `rte_strscpy()` call returns an error (e.g., `-ENAMETOOLONG`), the function sets `ret` to that error, breaks out of the loop, and closes the directory. This is correct. However, if `opendir()` succeeds but the `while` loop finds no matching entry (no non-dot files), the function closes the directory and returns 0, which is also correct. The logic appears sound. No issue found on closer inspection.

2. **`drivers/net/dpaa/dpaa_ethdev.c:65-71` - node_path validation**

The function checks `!node_path || node_path[0] == '\0'` and returns 0, which is the "not found" sentinel. This is appropriate--if there's no device tree path, there's no sysfs node to look up, so the caller will fall back to the DPDK port name. The function also checks that the last node of the path is non-empty before constructing the sysfs path. This is good defensive coding.

3. **`drivers/net/dpaa/dpaa_ethdev.c:82-84` - snprintf return value check**

The function checks both `ret < 0` (snprintf error, which is non-standard but possible on some platforms) and `(size_t)ret >= sizeof(path)` (truncation). This is a thorough check for path construction failure. Returning `-ENAMETOOLONG` is appropriate. Note: on Linux, `snprintf()` returns a non-negative value on success (the number of characters that would have been written, excluding the null terminator), so `ret < 0` is defensive but not harmful.

4. **`drivers/net/dpaa/dpaa_ethdev.c:324-331` - Fallback logic for shared interface name**

The code calls `dpaa_get_kernel_ifname()` to look up the kernel netdev name from sysfs, and only if that returns <= 0 or produces an empty string does it fall back to copying the DPDK port name into `ifr.ifr_name`. This means if the lookup fails or returns nothing, the old behavior (using the DPDK port name) is preserved. This is safe and maintains backward compatibility.

### Info

1. **`doc/guides/nics/dpaa.rst:306-309` - Documentation update**

The documentation now notes that the kernel interface name is looked up from sysfs, so non-`fmX-macY` naming is supported. This accurately reflects the code change. The wording is clear.

---

## PATCH 3/3: drivers: add DPAA cgrid cleanup support

### Errors

None.

### Warnings

1. **`drivers/bus/dpaa/base/qbman/qman.c:2996-3056` - qman_pending_fq_by_cgrid_range**

The function walks the entire FQID space (24 bits, 0x00FFFFFF) looking for frame queues that are still attached to a congestion group in the specified range. This is an expensive operation (potentially millions of queries), but it is noted in the comments and the caller is the device close path where the cost is acceptable. The function correctly handles the case where an FQID is not implemented (`-ERANGE` on query_fq_np) by breaking out of the loop, since FQIDs beyond that point are also unimplemented. The logic is sound, though performance-sensitive callers should be aware of the cost. The debug log clearly states it is scanning.

2. **`drivers/net/dpaa/dpaa_ethdev.c:596` - Loop bound on stale FQ cleanup**

The function `dpaa_cgr_stale_fq_cleanup()` sets a loop bound of `cgrid_num * DPAA_CGR_STALE_FQ_MAX`, which is the number of congestion groups times a per-group limit of 64 stale FQs. This bounds the cleanup effort but may not clean up *all* stale FQs if there are more than 64 per group. The comment in the code states this is intentional ("Do not spin on an FQ that refuses to shut down"), and the implementation resumes the scan past each handled FQID rather than restarting from the beginning, so it is O(N) rather than O(N^2) for N stale FQs. This is a reasonable tradeoff.

3. **`drivers/net/dpaa/dpaa_ethdev.c:706-730` - CGRID release on close**

The close path now calls `qman_release_cgrid_range()` for both Rx and Tx congestion groups after deleting all the CGRs. The Rx range is `[cgr_rx[0].cgrid, cgr_rx[0].cgrid + nb_rx_queues)`, and Tx is `[cgr_tx[0].cgrid, cgr_tx[0].cgrid + MAX_DPAA_CORES)`. Both assume the CGRIDs were allocated as contiguous ranges by `qman_alloc_cgrid_range()`, which is true based on the init code. This is correct.

4. **`drivers/net/dpaa/dpaa_ethdev.c:2088-2095` - Error handling in dpaa_rx_queue_init**

Previously, a `qman_create_cgr()` failure would log a warning and continue without the CGR (`goto without_cgr`), leaving the queue without tail drop. The patch changes this to return the error immediately, propagating the failure to the caller. The comment explains that falling back to no-CGR would break the error-path cleanup in the caller (which deletes `cgr_rx[0..n)` and assumes each queue either has a CGR or was never initialized). This is a correct fix, though it does mean a CGR creation failure now fails the entire probe. This is the safer choice.

5. **`drivers/net/dpaa/dpaa_ethdev.c:2108-2115` - CGR cleanup on qman_init_fq failure**

If `qman_init_fq()` fails after `qman_create_cgr()` succeeds, the function now deletes the CGR before returning the error. This prevents the CGR from being left linked into the portal's `cgr_cbs` list pointing into memory the caller will free. The comment clearly explains why this is needed. This is a good fix for a resource leak.

6. **`drivers/net/dpaa/dpaa_ethdev.c:2188-2195` - Same for Tx**

The equivalent fix is applied to `dpaa_tx_queue_init()`, both for the `qman_create_cgr()` failure (now returns error instead of falling through) and the `qman_init_fq()` failure (now deletes the CGR on error). The comments and logic mirror the Rx case. This is consistent.

7. **`drivers/net/dpaa/dpaa_ethdev.c:2680-2690` - Probe error path CGRID release**

The probe error path (`free_tx:`) now walks the created Tx CGRs (up to `nb_tx_cgr`, which is incremented only after each successful `dpaa_tx_queue_init()`) and deletes them before freeing `cgr_tx`. It also releases the reserved CGRID range if it was allocated (`tx_cgrid_allocated` flag). The Rx path (`free_rx:`) does the same. This ensures CGRIDs are returned to the allocator and CGRs are unlinked from the portal on probe failure. This is a correct fix for a resource leak.

---

## Final Summary

This three-patch series adds support for the DPAA offline (O/H) port, fixes a kernel netdev name lookup issue for shared MACs, and fixes congestion group cleanup on close and probe failure.

**Correctness:** The patches do not introduce use-after-free, resource leaks (the CGR cleanup in patch 3 fixes existing leaks), or race conditions. The atomic descriptor in patch 1 is correctly used with acquire/release ordering. Error paths are mostly correct, though patch 1's queue release logic is slightly convoluted (but ultimately safe due to nulling).

**Style and API:** The new API functions in `rte_pmd_dpaa_oldev.h` are correctly marked `__rte_experimental` and have `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macros. The header is properly installed in meson.build. The code follows DPDK conventions. The use of `bool` for flags is appropriate.

**Release notes:** The series does not update `doc/guides/rel_notes/`. A new driver feature (offline port support) and new experimental API functions should be documented in the release notes.

Review-Result: WARNING