|WARNING| [v3,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/172366

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-03

# DPDK Patch Review

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

### Errors

**Use of static_assert without header inclusion:**
The code uses `static_assert` but does not include `<assert.h>`.
While modern toolchains provide `static_assert` as a keyword in C11/C23,
the DPDK coding guidelines and portability requirements expect the header to be included
when using assert family macros/functions.

```c
/* drivers/bus/dpaa/base/fman/fman.c */
/* Missing: #include <assert.h> before static_assert use */
static_assert(RTE_DIM(mac_idx) == FMAN_MAX_MAC_IDX_NUM,
              "mac_idx[] out of sync with FMAN_MAX_MAC_IDX_NUM");
```

**Suggested fix:** Add `#include <assert.h>` at the top of `fman.c` and `dpaa_flow.c`.

**Error message formatting inconsistency:**
In `fman.c` lines 550-555, the error message uses `PRIu64` format for `cell_idx_host`,
but `cell_idx_host` is typed as `uint64_t` from `of_read_number()` whose return type
is documented only by context. The code should verify that `of_read_number()` returns `uint64_t`,
or cast the value explicitly in the format string to match `PRIu64`.
The same applies to lines 971-977 in the second location.

Line 550 code:
```c
cell_idx_host = of_read_number(cell_idx, lenp / sizeof(phandle));

if (cell_idx_host >= FMAN_MAX_MAC_IDX_NUM) {
    FMAN_ERR(-EINVAL,
             "%s: cell-index(%" PRIu64 ") >= max(%d)",
             oh_node->full_name, cell_idx_host,
             FMAN_MAX_MAC_IDX_NUM);
```

Without seeing the definition of `of_read_number()`, the type safety of this format string cannot be confirmed.
If `of_read_number()` does not return `uint64_t`, the format specifier and the argument mismatch,
which is undefined behavior.

**Suggested fix:** Cast `cell_idx_host` explicitly:
```c
FMAN_ERR(-EINVAL,
         "%s: cell-index(%" PRIu64 ") >= max(%d)",
         oh_node->full_name, (uint64_t)cell_idx_host,
         FMAN_MAX_MAC_IDX_NUM);
```
or verify and document that `of_read_number()` always returns `uint64_t`.

---

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

### Errors

**Missing validation of back-half port name length:**
In `dpaa_ol_probe()`, the code retrieves `bh_port_name` via `dpaa_ol_get_bh_port_name()`,
which internally uses `rte_strscpy()` to bounds-check the copy into a local buffer.
However, a second `rte_strscpy()` at line 909 copies that name into `dpaa_intf->ol_bh_port_name`
without checking the return value. If the name is too long, `rte_strscpy()` returns `-E2BIG`,
which the code ignores, leaving `dpaa_intf->ol_bh_port_name` truncated.
The subsequent `ask_set_fq_info()` at line 662 then copies this potentially truncated string
into the kernel ioctl structure, where a truncated name will mismatch the intended port
and cause configuration to fail or target the wrong port.

```c
/* drivers/net/dpaa/dpaa_oldev.c:909 */
dpaa_intf = eth_dev->data->dev_private;
rte_strscpy(dpaa_intf->ol_bh_port_name, bh_port_name,
            sizeof(dpaa_intf->ol_bh_port_name));
```

**Suggested fix:**
```c
ret = rte_strscpy(dpaa_intf->ol_bh_port_name, bh_port_name,
                  sizeof(dpaa_intf->ol_bh_port_name));
if (ret < 0) {
    DPAA_PMD_ERR("Backhaul port name too long: \"%s\"", bh_port_name);
    rte_eth_dev_release_port(eth_dev);
    return -EINVAL;
}
```

**Error-path resource leak in dpaa_oldev_init():**
If `dpaa_ol_tx_queue_init()` fails at line 844, the code jumps to `free_rx`,
which calls `dpaa_oldev_queues_release()`. However, `dpaa_intf->ol_fd` was initialised to `-1`
at line 797 and is never opened on this path (it is opened lazily in `check_fd()` on first use).
If an earlier successful call to `ask_get_channel_info()` inside `dpaa_ol_tx_queue_init()`
did open `ol_fd`, and then the subsequent `qman_create_fq()` or `qman_init_fq()` fails,
the fd is never closed because `dpaa_ol_dev_close()` is not called on the probe failure path
and `dpaa_oldev_queues_release()` does not close it.

```c
/* drivers/net/dpaa/dpaa_oldev.c:843 */
ret = dpaa_ol_tx_queue_init(dpaa_intf, &dpaa_intf->tx_queues[0], 0);
if (ret)
    goto free_rx;  /* leaks dpaa_intf->ol_fd if it was opened */
```

**Suggested fix:** Close `ol_fd` in the `free_rx` error path:
```c
free_rx:
    dpaa_oldev_queues_release(dpaa_intf);
    if (dpaa_intf->ol_fd >= 0) {
        close(dpaa_intf->ol_fd);
        dpaa_intf->ol_fd = -1;
    }
    return ret;
```

**Missing release notes section identifier:**
The release notes addition at line 210-217 of `doc/guides/rel_notes/release_26_11.rst`
does not include the standard section heading markup (`* **Added ...`).
The entry is formatted as a bullet list but lacks the bold lead-in expected by DPDK release notes style.

**Suggested fix:** Add the bold marker:
```rst
* **Added NXP DPAA offline (O/H) port support.**
```

---

### Warnings

**Exported symbol without Doxygen documentation:**
`qman_release_fqid_range()` is marked `__rte_internal` and exported at line 56
of `drivers/bus/dpaa/dpaa_bus_base_symbols.c`, but the function has no Doxygen
comment in `drivers/bus/dpaa/include/fsl_qman.h`. Internal symbols should still
be documented so other DPDK components can use them correctly.

**Suggested fix:** Add a Doxygen comment block before the `qman_release_fqid_range()` declaration.

**errno saved but not all code paths check it:**
In `ask_get_channel_info()`, `ask_set_fq_info()`, and `ask_reset_fq_info()`,
the code saves `errno` after an ioctl failure and logs it, then returns `-err`.
However, `errno` is only meaningful if `ioctl()` returned `-1`.
The code does not verify `ret == -1` before reading `errno`, so if `ioctl()` returned
a different negative value (which is unusual but not impossible for a custom ioctl),
the saved errno could be stale from a prior system call.

Lines 179-187 in `dpaa_oldev.c`:
```c
ret = ioctl(fd, ASK_CTRL_GET_OFFLINE_CHANNEL_INFO, ch_info);
if (ret) {
    int err = errno;
    /* Save errno first: the log call may overwrite it. */
    DPAA_PMD_ERR("Get channel info ioctl failed with errno: %s", strerror(err));
    return -err;
}
```

**Suggested fix:** Change to `if (ret < 0)` and explicitly check `ret == -1`
before saving `errno`, or document that this ioctl only fails with `-1`.

**Use of `assert()` in production code:**
`check_fd()` at line 159 uses `assert()` on `pthread_mutex_lock()` and `pthread_mutex_unlock()`.
`assert()` is compiled out in release builds (`-DNDEBUG`), so a mutex failure in production
would be silently ignored. Mutex failures are rare but when they occur (e.g., recursive lock
without `PTHREAD_MUTEX_RECURSIVE`), ignoring them leads to data races.

**Suggested fix:** Check the return value and handle it explicitly:
```c
ret = pthread_mutex_lock(&fd_init_lock);
if (ret) {
    DPAA_PMD_ERR("mutex lock failed: %s", strerror(ret));
    return -ret;
}
```

---

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

### Warnings

**Potential resource leak on readdir loop early exit:**
In `dpaa_get_kernel_ifname()`, if `snprintf()` fails (line 85-87) or if `realpath()` fails
(line 89-90), the loop continues to the next `readdir()` entry without setting `ret`.
If the loop exhausts all entries without finding a match, `closedir()` is called at line 109
and the function returns `ret = 0`. However, if an error occurs in the final iteration
(e.g., `rte_strscpy()` returns `-ENAMETOOLONG` at line 101), `ret` is set to the error code,
`closedir()` is called, and the error is returned. This is correct. But if an error occurs
in an early iteration and `continue` is taken, the error is lost. This is acceptable
given the function is a lookup and missing one sysfs entry is not fatal, but the asymmetry
is worth noting.

No action required if the behaviour is intentional, but consider logging a warning
when `snprintf()` or `realpath()` fails, so the user knows a sysfs entry was skipped.

---

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

### Errors

**qman_pending_fq_by_cgrid_range() has no Doxygen:**
The function `qman_pending_fq_by_cgrid_range()` is marked `__rte_internal` and exported,
but has no Doxygen comment block in `drivers/bus/dpaa/include/fsl_qman.h`.
The comment at lines 1922-1945 in the patch is a C-style comment, not Doxygen.

**Suggested fix:** Convert the comment to Doxygen format:
```c
/**
 * qman_pending_fq_by_cgrid_range - Find an FQ still attached to a CGR range.
 * @cgrid_start: first congestion group id of the range to search for.
 * ...
 */
```

---

### Warnings

**Stale FQ scan performance:**
`dpaa_cgr_stale_fq_cleanup()` scans the entire 24-bit FQID space (16 million FQIDs)
once per port close. For a port with 8 Rx queues, this is 8 full scans (one per CGR)
if the code did not batch them. The patch does batch the CGRID range, so one scan covers
all Rx CGRs and another scan covers all Tx CGRs, which is acceptable. However, the scan
is O(N) in the FQID space size, and `qman_query_fq_np()` and `qman_query_fq()` are not
free. On a device with many ports, closing all of them sequentially could take seconds.

The patch documents this as acceptable because it only runs on the close path,
but consider whether a warning should be logged if the scan finds more than a few stale FQs,
indicating a previous crash or leak.

**Maximum loop count (`DPAA_CGR_STALE_FQ_MAX`) is arbitrary:**
The define `DPAA_CGR_STALE_FQ_MAX` is set to 64 at line 118 of `dpaa_ethdev.c`.
The comment at line 117 says "Maximum number of stale frame queues cleaned up per congestion group."
However, the loop limit at line 611 is `cgrid_num * DPAA_CGR_STALE_FQ_MAX`, which is 64 FQs per CGR.
If a pathological case leaves 65 stale FQs in one CGR, the 65th is not cleaned up,
the function returns `false`, the CGRIDs are leaked, and the warning at line 738
("delete txq's cgr err") is logged.

**Suggested fix:** Either increase the limit, or log a warning when the limit is hit
so the user knows FQs were left behind.

---

## Summary of Findings

**Patch 1/4:**
- Error: Missing `#include <assert.h>` for `static_assert`.
- Error: `PRIu64` format string used without verifying `of_read_number()` return type is `uint64_t`.

**Patch 2/4:**
- Error: `rte_strscpy()` return value not checked in `dpaa_ol_probe()`, risking truncated backhaul port name.
- Error: `dpaa_intf->ol_fd` leaked on `dpaa_oldev_init()` error path if opened by `ask_get_channel_info()`.
- Error: Release notes entry missing bold section heading.
- Warning: `qman_release_fqid_range()` exported without Doxygen.
- Warning: `errno` read without verifying `ioctl()` returned `-1`.
- Warning: `assert()` used on mutex operations in production code.

**Patch 3/4:**
- No errors.
- Info: `readdir()` loop continues on `snprintf`/`realpath` failure without logging.

**Patch 4/4:**
- Error: `qman_pending_fq_by_cgrid_range()` exported without Doxygen (C-style comment used instead).
- Warning: Stale FQ scan performance acceptable but could log a warning if many FQs found.
- Warning: `DPAA_CGR_STALE_FQ_MAX` limit arbitrary; hitting it leaks CGRIDs.

---

Review-Result: ERROR