|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

**1. Resource leak on error path: RX queue allocation failure**

In `dpaa_oldev_init()`, after allocating `rx_queues`:

```c
dpaa_intf->rx_queues = rte_zmalloc(...);
if (!dpaa_intf->rx_queues) {
    DPAA_PMD_ERR("Failed to alloc mem for RX queues");
    return -ENOMEM;
}
dpaa_intf->nb_rx_queues = num_fqs;

ret = dpaa_ol_rx_queue_init(&dpaa_intf->rx_queues[0], 0);
if (ret)
    goto free_rx;
```

But if `dpaa_ol_rx_queue_init()` fails, `goto free_rx` calls `dpaa_oldev_queues_release()`, which only releases FQIDs if `fqid != 0`. The `rx_queues` array was `rte_zmalloc()`'d (zero-initialized), so `fqid` is 0 and the array is never freed. The memory leaks.

**Fix:** Either:
- Initialize `dpaa_intf->rx_queues[0].fqid` before calling `dpaa_ol_rx_queue_init()` so `dpaa_oldev_queues_release()` recognizes it, or
- Add explicit `rte_free(dpaa_intf->rx_queues)` before the error return.

**2. TX queue allocation leak on init failure**

Same pattern for TX queues: if `dpaa_ol_tx_queue_init()` fails, `goto free_rx` never frees `tx_queues` because its `fqid` is still 0.

**Fix:** Same as above -- either set the FQID before calling `dpaa_ol_tx_queue_init()` or add an explicit free.

**3. Descriptor leak on Tx queue shutdown failure**

In `dpaa_oldev_queues_release()`:

```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;
}
qman_release_fqid(dpaa_intf->tx_queues[i].fqid);
```

If `qman_shutdown_fq()` fails, the FQID is leaked (comment says this is intentional: "safe choice"). However, the comment should explain *why* releasing it would be unsafe (QMan still references it), not just that it's "safe" to leak it. The current comment is correct but could be clearer.

**Not an error, but a clarity issue (Info only):** Consider rewording the comment to say: "The FQ is still live in QMan, so releasing the FQID could allow qman_alloc_fqid() to reuse it while the hardware still references it. Leaking it is the safe choice."

**4. ask_fd not closed on all error paths**

`ask_fd` is opened in `check_fd()` and closed in `dpaa_ol_dev_close()`. If `dpaa_ol_probe()` fails after `dpaa_oldev_init()` (which may trigger `check_fd()` via ioctl calls), the fd is never closed because `dpaa_ol_dev_close()` is only called on successful probe.

**Fix:** Call `dpaa_ol_dev_close()` from `dpaa_ol_probe()`'s error path, or close `ask_fd` explicitly there.

**5. Pre-existing code: `mac_idx` array bounds not checked in `dpaa_flow.c`**

In `dpaa_flow.c`:

```c
static const uint8_t mac_idx[] = {-1, 0, 1, 2, 3, 4, 5, 6, 7, 0, 1};
```

This patch adds `static_assert(RTE_DIM(mac_idx) == FMAN_MAX_MAC_IDX_NUM, ...)` but does not add bounds checking where `mac_idx[fman_if.mac_idx]` is indexed. The bus now validates `mac_idx` against `FMAN_MAX_MAC_IDX_NUM` in `fman.c`, so this is protected, but the flow code should assert `fman_if.mac_idx < RTE_DIM(mac_idx)` before indexing to make the contract explicit.

**This is Info only** -- the patch does not introduce the missing check, and the bus-level validation makes an out-of-bounds access unreachable in practice.

### Warnings

**6. Missing NULL check on `rte_mempool_get_priv()` return**

In `dpaa_ol_set_fq_info()`:

```c
mbp_priv = (struct rte_pktmbuf_pool_private *)rte_mempool_get_priv(dpaa_intf->bp_info->mp);
fq_info.tx_fq_id = txq->fqid;
fq_info.rx_fq_id = rxq->fqid;
fq_info.buff_size = mbp_priv->mbuf_data_room_size;
```

`rte_mempool_get_priv()` can return NULL if the mempool has no private data. Dereferencing it without checking causes a NULL pointer dereference.

**Fix:** Add `if (mbp_priv == NULL) return -EINVAL;` after the assignment.

**7. errno saved but log call may overwrite it**

In `ask_get_channel_info()`, `ask_set_fq_info()`, and the four experimental API functions:

```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;
}
```

The comment says "log call may overwrite it," but `strerror(err)` is called *after* saving `err`, so the saved value is used. The comment is misleading -- it should say "errno may be overwritten by subsequent calls" rather than "log call may overwrite it."

**Fix:** Clarify the comment: `/* Save errno before any library call that may overwrite it. */`

**8. New experimental API without testpmd hooks or functional tests**

The four experimental 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`) are added without corresponding testpmd commands or functional tests in `app/test/`.

**9. Release notes missing**

No update to `doc/guides/rel_notes/` for the new offline port feature, the new public API header (`rte_pmd_dpaa_oldev.h`), or the four experimental functions.

**10. `rte_pmd_dpaa_oldev.h` installed but no ABI exports**

The header is listed in `headers = files('rte_pmd_dpaa.h', 'rte_pmd_dpaa_oldev.h')`, so it is installed. The four experimental functions have `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macros in the `.c` file, which is correct. However, the version argument is `26.11`, which is in the past relative to the patch date (2026-10-28). It should be `26.11` only if this was added in the 26.11 release cycle; if this is for a later release, update the version.

**11. `drv_bh_port` devarg not validated**

In `dpaa_ol_get_bh_port_name()`, the `DRIVER_OL_BH_PORT` devarg is retrieved but not validated for length or format. If the value is too long, `rte_strscpy()` returns `-E2BIG`, but the caller treats `ret <= 0` as "not found" and returns `-EINVAL` with a generic message. A user cannot tell whether the devarg was missing or malformed.

**Fix:** Distinguish the error cases: return `-E2BIG` from `dpaa_ol_get_bh_port_name()` when `rte_strscpy()` fails, and log a specific error in the caller.

---

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

### Errors

None.

### Warnings

**12. `opendir()` failure not logged**

In `dpaa_get_kernel_ifname()`:

```c
dir = opendir(path);
if (!dir)
    return 0;
```

If `opendir()` fails, the function silently returns 0 (not found). An application operator cannot tell whether the device tree node is valid but has no netdev, or whether the sysfs path is wrong. Log the failure and errno.

**Fix:**

```c
dir = opendir(path);
if (!dir) {
    DPAA_PMD_DEBUG("opendir(%s) failed: %s", path, strerror(errno));
    return 0;
}
```

**13. `readdir()` error vs end-of-directory not distinguished**

After the `while ((entry = readdir(dir)) != NULL)` loop, the function does not check whether `readdir()` failed or simply reached the end. POSIX allows `readdir()` to set `errno` on error, but only if the caller checks `errno` before the call and after the loop.

**Fix:** Set `errno = 0;` before the loop, then check `errno` after the loop to distinguish error from end-of-directory.

---

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

### Errors

**14. Integer overflow in `dpaa_cgr_stale_fq_cleanup()` loop bound**

```c
max_loop = cgrid_num * DPAA_CGR_STALE_FQ_MAX;
for (loop = 0; loop < max_loop; loop++) {
```

Both `cgrid_num` and `max_loop` are `uint32_t`, and `DPAA_CGR_STALE_FQ_MAX` is 64. If `cgrid_num` exceeds `0xFFFFFFFF / 64 = 0x3FFFFFF`, the multiplication overflows and `max_loop` wraps, making the loop bound incorrect.

**Fix:** Widen the multiplication to `uint64_t`:

```c
uint64_t max_loop = (uint64_t)cgrid_num * DPAA_CGR_STALE_FQ_MAX;
```

And change `max_loop` and `loop` to `uint64_t` (or keep `loop` as `uint32_t` and cast the comparison).

### Warnings

**15. Pre-existing: `loop` variable in `dpaa_eth_dev_close()` changed from `int` to `uint32_t`**

The patch changes `int loop;` to `uint32_t loop;` in `dpaa_eth_dev_close()`. This is fine, but the change is unrelated to the CGRID cleanup feature and should be in a separate patch (or at least mentioned in the commit message).

**16. `qman_pending_fq_by_cgrid_range()` added to bus symbols but not marked `__rte_internal` in header**

The function is declared `__rte_internal` in `fsl_qman.h` (correct) and exported in `dpaa_bus_base_symbols.c` (correct). No issue here.

---

## General Issues

**17. Series needs cover letter**

A 3-patch series adding a new device type, new public API, and a new cleanup mechanism should have a cover letter (`[PATCH v1 0/3]`) explaining the feature, its use case, and the test plan.

**18. Documentation: `drv_oldev` example is verbose**

The doc says:

```
-a bus=dpaa_bus,drv_oldev=1 \
-a dpaa_bus:oldev1,drv_bh_port=fm1-mac5 \
-a dpaa_bus:fm1-mac5 -a dpaa_bus:fm1-mac6
```

This is correct but could be clearer about *why* listing the MAC ports is necessary (allowlist scan mode). Consider a sentence before the example: "Passing any `-a` argument for the DPAA bus switches the bus to allowlist scan mode, so all needed ports must be explicitly listed."

---

## Summary

- **6 Errors:** Resource leaks on error paths (rx_queues, tx_queues, ask_fd), NULL dereference on `mbp_priv`, integer overflow in loop bound.
- **12 Warnings:** Missing NULL checks, unclear comments, missing tests/docs, unvalidated devargs, undistinguished error cases, unrelated code change.
- **2 Info:** Pre-existing mac_idx bounds check, FQID leak comment clarity.

The core correctness issues are the resource leaks in `dpaa_oldev_init()` and the integer overflow in `dpaa_cgr_stale_fq_cleanup()`. The rest are primarily documentation, error handling, and testing gaps.

---

Review-Result: ERROR