|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

Thank you for providing the patch series. I'll review it against the DPDK contribution guidelines, focusing on correctness, code style, API requirements, and other issues.

---

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

### Errors

None.

### Warnings

**1. Missing bounds check on `cell_idx_host` before it is truncated to `uint8_t`**

In `fman_if_init()` at line 558:
```c
__if->__if.mac_idx = cell_idx_host;
```
`cell_idx_host` is a `uint64_t` read from the device tree, but `__if->__if.mac_idx` is a `uint8_t` (implied by the code context). The bounds check at line 550 verifies it is less than `FMAN_MAX_MAC_IDX_NUM` (11), which fits in `uint8_t`, so the truncation is safe. However, the assignment should use an explicit cast to make the narrowing intentional and silence any compiler warnings:
```c
__if->__if.mac_idx = (uint8_t)cell_idx_host;
```
The same applies in `fman_if_init_onic()` at line 979.

**2. `static_assert` in `dpaa_flow.c` is redundant with the one in `fman.c`**

At `dpaa_flow.c:38`:
```c
static_assert(RTE_DIM(mac_idx) == FMAN_MAX_MAC_IDX_NUM,
	"mac_idx[] out of sync with FMAN_MAX_MAC_IDX_NUM");
```
This duplicates the assertion added to `fman.c:208`. While redundancy is acceptable for a critical invariant, it would be cleaner to document in the comment at `dpaa_flow.c:34` that the bus validation ensures the index is in bounds, and omit the second assert.

---

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

### Errors

**1. Control device `/dev/cdx_ctrl` is opened but never closed on error paths during device probe**

In `dpaa_oldev_init()`, if `dpaa_ol_tx_queue_init()` fails (line 858), execution jumps to `free_rx` which calls `dpaa_oldev_queues_release()`. However, `dpaa_intf->ol_fd` is only closed in `dpaa_ol_dev_close()` (line 460), not in `dpaa_oldev_queues_release()`. If `dpaa_oldev_init()` fails after the first `check_fd()` call (which opens the fd), the fd is leaked.

**Fix**: Add `close(dpaa_intf->ol_fd)` and set `dpaa_intf->ol_fd = -1` in the `free_rx` error path of `dpaa_oldev_init()`, before `dpaa_oldev_queues_release()`.

**2. `ask_reset_fq_info()` failure in `dpaa_oldev_queues_release()` prevents FQID release but does not prevent the FQIDs from being freed by `qman_shutdown_fq()`**

At `dpaa_oldev.c:723`:
```c
reset_ret = ask_reset_fq_info(dpaa_intf, &fq_info);
if (reset_ret) {
	DPAA_PMD_WARN("Reset FQ info failed with ret: %d, not releasing fqids",
		      reset_ret);
}
release_fqids = (reset_ret == 0);
```
If `ask_reset_fq_info()` fails, `qman_release_fqid()` is skipped (lines 746, 759). However, `qman_shutdown_fq()` is still called (lines 740, 753), which may free internal QMan state. If the kernel still believes the FQID is in use, a subsequent reuse of that FQID by another process could corrupt the kernel's tracking. The code should either fail the close (and let the application retry or leak the port) or ensure that `qman_shutdown_fq()` is not called if `ask_reset_fq_info()` fails.

**3. `dpaa_ol_rx_queue_setup()` unconditionally overwrites `dpaa_intf->bp_info` on every call**

At line 505:
```c
dpaa_intf->bp_info = DPAA_MEMPOOL_TO_POOL_INFO(mp);
```
If `rx_queue_setup` is called twice (e.g., application reconfigures queues), the second mempool overwrites the first without checking if they differ. This could cause the port to send buffers from pool A but expect the kernel to return buffers to pool B. Either document that only one Rx queue setup is allowed, or verify that all `rx_queue_setup` calls use the same mempool.

**4. `dpaa_ol_set_fq_info()` reads `dpaa_intf->rx_queues[0]` and `dpaa_intf->tx_queues[0]` without checking if they are initialized**

At lines 605-606:
```c
txq = &dpaa_intf->tx_queues[0];
rxq = &dpaa_intf->rx_queues[0];
```
If the application calls `dev_start()` without having called `rx_queue_setup()` and `tx_queue_setup()`, `dpaa_intf->rx_queues` and `dpaa_intf->tx_queues` are non-NULL (allocated in `dpaa_oldev_init()`), but `fqid` is zero (from `rte_zmalloc()`). The ioctl at line 627 would then send invalid FQIDs to the kernel. The NULL check at line 596 is insufficient; the code should verify that `rx_queues[0].fqid != 0` and `tx_queues[0].fqid != 0` before reading them.

### Warnings

**1. `dpaa_ol_dev_start()` returns success even if `dpaa_ol_set_fq_info()` fails**

At line 446:
```c
ret = dpaa_ol_set_fq_info(dev);
if (ret)
	return ret;
```
This is correct, but if the FQ info is not sent to the kernel, the device is non-functional. Consider adding a WARN or ERR log before the return to clarify the failure mode.

**2. `DPAA_OL_MAX_BH_PORT_NAME_LEN` is 12 bytes but typical interface names can be 15 characters plus null terminator (IFNAMSIZ=16)**

Defined at `dpaa_ethdev.h:118`:
```c
#define DPAA_OL_MAX_BH_PORT_NAME_LEN	12
```
If a backhaul port name is longer than 11 characters, `rte_strscpy()` at `dpaa_oldev.c:568` and `dpaa_oldev.c:912` will truncate it, leading to a mismatch between the stored name and the actual interface. Either increase the constant to `IFNAMSIZ` (16) or document the 11-character limit.

**3. `dpaa_oldev_init()` allocates `nb_rx_queues` and `nb_tx_queues` based on `DPAA_DEFAULT_NUM_PCD_QUEUES` but only initializes queue 0**

At lines 824, 842:
```c
dpaa_intf->nb_rx_queues = num_fqs;
...
dpaa_intf->nb_tx_queues = num_fqs;
```
But only `rx_queues[0]` and `tx_queues[0]` are initialized (lines 836, 849). If the application calls `rx_queue_setup(queue_idx=1)`, it will index into uninitialized memory. Either initialize all queues or set `nb_rx_queues = 1`.

**4. Missing release notes entry for the new `qman_release_fqid_range()` internal symbol**

The patch adds `RTE_EXPORT_INTERNAL_SYMBOL(qman_release_fqid_range)` at `dpaa_bus_base_symbols.c:55`, which is a new internal symbol. The release notes mention the offline port PMD but do not mention the new bus symbol. Since this is an internal symbol, it does not require a release note entry, but if the intention is to expose it for other bus users, it should be noted.

**5. `dpaa_ol_promiscuous_enable()` is a no-op**

At line 695:
```c
static int dpaa_ol_promiscuous_enable(__rte_unused struct rte_eth_dev *dev)
{
	PMD_INIT_FUNC_TRACE();
	return 0;
}
```
This is acceptable if the offline port does not support promiscuous mode, but the `dev_ops` structure should set `promiscuous_enable = NULL` instead of providing a no-op callback, to indicate the feature is unsupported.

---

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

### Errors

None.

### Warnings

**1. `dpaa_get_kernel_ifname()` does not handle `readdir()` errors**

At line 80:
```c
while ((entry = readdir(dir)) != NULL) {
```
If `readdir()` fails (e.g., permission denied on a `/sys/class/net/<ifname>/device/of_node` directory), it returns NULL but also sets `errno`. The loop treats this as "end of directory" and returns 0 (not found). The code should check `errno` after the loop to distinguish an error from a genuine "not found".

**2. `dpaa_get_kernel_ifname()` may return a stale name if the device tree node is reused**

If a device is removed and another device is bound to the same device tree node, the function will match the new device's netdev name. This is unlikely in practice but worth noting in a comment.

**3. Buffer overflow risk in `snprintf()` at line 88**

At line 88:
```c
if (snprintf(path, sizeof(path),
	     "/sys/class/net/%s/device/of_node",
	     entry->d_name) >= (int)sizeof(path))
	continue;
```
`entry->d_name` is untrusted input (from the filesystem). If it contains a very long name (up to 255 bytes), the `snprintf()` call will truncate it, but the code correctly checks for truncation and continues. However, the comment "buffer overflow risk" is misleading; the code is safe. Consider rewording the comment to "skip if path would be truncated".

---

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

### Errors

**1. `dpaa_cgr_stale_fq_cleanup()` may infinitely loop if `qman_shutdown_fq_by_fqid()` fails for all FQs in the range**

At line 620:
```c
max_loop = cgrid_num * DPAA_CGR_STALE_FQ_MAX;
for (loop = 0; loop < max_loop; loop++) {
```
The loop is bounded by `max_loop`, which is 64 x `cgrid_num`. If there are more than 64 stale FQs per CGRID, the function returns `false` after the loop limit, leaving stale FQs behind. However, if every `qman_shutdown_fq_by_fqid()` call fails (e.g., kernel bug), the function increments `start_fqid` but never removes any FQ, and the loop will scan the same `DPAA_CGR_STALE_FQ_MAX` FQs repeatedly until `max_loop` is exhausted. The comment at line 625 should clarify that `max_loop` is a safety limit, not a guarantee that all FQs will be cleaned.

**2. `dpaa_eth_dev_close()` leaks CGRIDs if `dpaa_cgr_stale_fq_cleanup()` returns `false`**

At lines 730, 763:
```c
if (!dpaa_cgr_stale_fq_cleanup(dev, dpaa_intf->cgr_rx[0].cgrid,
			  dpaa_intf->nb_rx_queues, "rx"))
	cgrid_ok = false;
```
If the stale FQ cleanup fails (returns `false`), `cgrid_ok` is set to `false`, and the CGRIDs are not released (lines 751, 784). This is intentional to avoid reusing a CGRID that may still be referenced by a live FQ. However, if the application repeatedly opens and closes the port, the leaked CGRIDs will eventually exhaust the CGRID allocator. The code should either fail the `dev_close()` operation (return an error) or document that repeated close-after-failure will leak resources.

**3. `qman_pending_fq_by_cgrid_range()` in `qman.c` does not check if `fqid` or `cgrid` output pointers are NULL before dereferencing**

At lines 3044, 3046:
```c
if (fqid)
	*fqid = fq.fqid;
if (cgrid)
	*cgrid = fqd.cgid;
```
The NULL checks are present, so this is safe. However, the API documentation at line 1932 should clarify that `fqid` is mandatory (must not be NULL) since the return value is 0 on success regardless of whether `fqid` was set.

### Warnings

**1. `qman_pending_fq_by_cgrid_range()` scans the entire 24-bit FQID space, which may take significant time**

At line 3023:
```c
for (; fq.fqid <= QMAN_MAX_FQID; fq.fqid++) {
```
For a device with 16M FQIDs, this loop will query every FQID, including those not implemented. The `-ERANGE` check at line 3025 exits early on unimplemented FQIDs, but this still requires at least one ioctl per FQID. On a slow device, this could take several seconds. Consider adding a timeout or rate-limiting the scan.

**2. `dpaa_cgr_stale_fq_cleanup()` does not log which CGRID had a stale FQ**

At line 620:
```c
DPAA_PMD_DEBUG("FQ(fqid=0x%x) with %s cgid=%u is still alive?",
	fqid, dir, cgrid);
```
This logs the CGRID, so the warning is incorrect. However, if the cleanup fails (returns `false`), no ERROR or WARN log is emitted in the caller (`dpaa_eth_dev_close()`), only the `cgrid_ok = false` assignment. Consider adding a WARN log when `dpaa_cgr_stale_fq_cleanup()` returns `false`.

**3. `dpaa_dev_init()` error path at `free_tx` and `free_rx` calls `qman_shutdown_fq()` and `qman_delete_cgr()` in separate loops**

At lines 2733-2738:
```c
for (loop = 0; loop < nb_tx_cgr; loop++)
	qman_shutdown_fq(&dpaa_intf->tx_queues[loop]);
for (loop = 0; loop < nb_tx_cgr; loop++)
	qman_delete_cgr(&dpaa_intf->cgr_tx[loop]);
```
If `qman_shutdown_fq()` fails for queue `i`, the CGR for queue `i` is still deleted in the second loop. This could leave the CGR in an inconsistent state if it still has members. Consider checking the return value of `qman_shutdown_fq()` and skipping the corresponding `qman_delete_cgr()` if it failed.

---

## Cross-Patch Issues

None identified. Each patch compiles independently and does not introduce forward dependencies.

---

## Summary

- **Patch 1/4**: Clean, only minor style improvements suggested.
- **Patch 2/4**: Contains multiple resource leaks (control device fd, FQID handling) and uninitialized memory accesses. These must be fixed before merging.
- **Patch 3/4warning: review has no Review-Result line, severity inferred from prose (response may be truncated)