|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

# DPDK Patch Review

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

### Errors

None identified.

### Warnings

None identified.

### Info

The bounds check and `static_assert` are correctly placed. The check catches oversized `cell-index` values before they are used as an index, and the `static_assert` ensures the lookup table stays in sync with the new constant.

---

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

### Errors

**Control device descriptor race condition:**

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

The `check_fd()` function uses a double-checked locking pattern, but its implementation has a critical error in the load path. The fast-path load:

```c
cur_fd = rte_atomic_load_explicit(&ask_fd, rte_memory_order_acquire);
if (cur_fd >= 0)
    return cur_fd;
```

reads `ask_fd` with acquire ordering, but the subsequent `open()` call stores the fd with release ordering only:

```c
rte_atomic_store_explicit(&ask_fd, cur_fd, rte_memory_order_release);
```

This creates a problem: the release store pairs with the acquire load inside the locked region, but the unlocked fast-path load has no synchronization with the side effects of `open()`. If another thread observes `ask_fd >= 0` via the fast-path load, there is no guarantee it sees the file descriptor in a usable state--the descriptor could be partially initialized, or the kernel-side state might not yet be visible.

The correct pattern is to use `rte_memory_order_acq_rel` for the store:

```c
rte_atomic_store_explicit(&ask_fd, cur_fd, rte_memory_order_release);
```

should be:

```c
rte_atomic_store_explicit(&ask_fd, cur_fd, rte_memory_order_release);
```

Wait--actually, the issue is different. The problem is that the fast-path acquire load needs to synchronize with a release store that happens-after all the setup work (the `open()` call). The current code does store with release, which is correct. But the concern is whether the file descriptor returned by `open()` is itself safe to use immediately after being read from the atomic.

Actually, re-reading the code: the pattern is correct. The `open()` system call completes before the `rte_atomic_store_explicit(..., release)`, so all side effects of `open()` (kernel state, fd table entry) are visible to any thread that observes `ask_fd >= 0` via an acquire load. The release store on the writer side ensures everything before it (including `open()`) is visible to the acquire load on the reader side.

Correction: the code is correct. The release/acquire pair suffices here. Discard this finding.

---

**Errno clobbering in error paths:**

In `ask_get_channel_info()`:

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

This pattern is used consistently in all the `ask_*` functions and is correct--the comment even explains why `errno` is saved before the log call. This is not an error.

---

**Missing error check on `rte_strscpy()` return value:**

In `dpaa_ol_get_bh_port_name()`:

```c
ret = rte_strscpy(val, str, size);
rte_kvargs_free(kvlist);
if (ret < 0)
    return ret;
```

The error check is present. This is correct.

---

**Resource leak on `kvlist` in error path:**

In `dpaa_ol_get_bh_port_name()`:

```c
kvlist = rte_kvargs_parse(devargs->args, valid_keys);
if (!kvlist) {
    DPAA_PMD_ERR("Invalid devargs for offline port \"%s\"",
                 devargs->args);
    return -EINVAL;
}
```

All subsequent error paths (`!rte_kvargs_count()`, `rte_kvargs_get() == NULL`, `rte_strscpy() < 0`) call `rte_kvargs_free(kvlist)` before returning. This is correct.

---

**Frame queue cleanup error handling:**

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

When `qman_shutdown_fq()` fails, the FQID is deliberately *not* released, and the comment in the code explains why: "The FQ is still live in QMan, so the FQID cannot be handed back: `qman_alloc_fqid()` could give it to another user while the hardware still references it. Leaking it is the safe choice."

This is intentional and correct. The same pattern is used for both Tx and Rx queues. Not an error.

---

**Offline port not supported in secondary processes:**

In `dpaa_ol_probe()`:

```c
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
    DPAA_PMD_WARN("%s: not supported in secondary process",
                  dpaa_dev->device.name);
    return -ENOTSUP;
}
```

The comment in `dpaa_ol_probe()` explains why: "The offline port's frame queues are created with dynamic FQIDs and programmed into the kernel through the control device, all of which is owned by the primary. A secondary has no way to attach to that state."

This is a deliberate design choice, documented in the code and in the user-facing documentation patch. Not an error.

---

### Warnings

**New experimental APIs missing Doxygen `@warning` block:**

The four new functions in `rte_pmd_dpaa_oldev.h` are marked `__rte_experimental` and have `@warning` blocks at the file level, but each function declaration lacks an individual `@warning` block stating "EXPERIMENTAL: this API may change without prior notice."

DPDK convention is to mark each experimental function with its own warning. Add:

```c
/**
 * @warning
 * @b EXPERIMENTAL: this API may change without prior notice.
 *
 * Program the uplink classification information...
 */
```

to each of the four function declarations.

---

**Control device descriptor closed without checking current users:**

In `dpaa_ol_dev_close()`:

```c
cur_fd = rte_atomic_exchange_explicit(&ask_fd, -1,
                                      rte_memory_order_acq_rel);
if (cur_fd >= 0)
    close(cur_fd);
```

If the application has multiple offline ports (though the current design only supports one, per `DPAA_OLDEV_NAME`), closing the control device on the first port's `dev_close()` would invalidate the descriptor for the others.

However, reading the bus code in patch 2/4, only one offline device is ever created (`oldev1`), so this scenario cannot occur. The warning is moot given the single-instance design. This is acceptable.

---

**Experimental APIs exported with `26.11` version:**

```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_dpaa_ol_set_classif_info, 26.11)
```

If this patch is targeting DPDK 25.03, the version should be `25.03`, not `26.11`. Verify the target release and adjust the version accordingly.

---

### Info

**Devargs parsing in `dpaa_ol_get_bh_port_name()`:**

The function correctly validates the `drv_bh_port` devarg using `rte_kvargs_parse()` with a valid keys list. It handles all error cases (parse failure, missing key, NULL value, copy truncation) and frees `kvlist` on all paths. This is a good example of correct devargs handling.

---

**FQ and CGR lifecycle management:**

The FQ and CGR creation, initialisation, shutdown, and deletion sequence is carefully ordered throughout `dpaa_oldev_init()`, `dpaa_oldev_queues_release()`, and `dpaa_ol_dev_close()`. The error path in `dpaa_oldev_init()` correctly calls the release helper, which shuts down and releases only the FQs whose `.fqid` is nonzero (indicating successful creation). This is correct.

---

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

### Errors

None identified.

### Warnings

**Missing check on `opendir()` and `readdir()` system errors:**

In `dpaa_get_kernel_ifname()`:

```c
dir = opendir("/sys/class/net");
if (!dir)
    return 0;
```

When `opendir()` fails, the function returns 0 (not found), swallowing the error. If `/sys` is not mounted or `/sys/class/net` does not exist due to a misconfigured kernel, the caller has no way to distinguish "no matching netdev found" from "sysfs not available."

Return `-errno` on `opendir()` failure so the caller can distinguish an error from a clean "not found" result:

```c
dir = opendir("/sys/class/net");
if (!dir)
    return -errno;
```

Similarly, after the loop, if `readdir()` itself failed (as opposed to reaching end-of-directory), `errno` will be set but the function returns 0. Check `errno` after the loop to catch `readdir()` errors:

```c
while ((entry = readdir(dir)) != NULL) {
    /* ... */
}
if (errno) {
    int err = errno;
    closedir(dir);
    return -err;
}
```

---

**Unbounded `snprintf()` result not checked:**

```c
if (snprintf(path, sizeof(path),
             "/sys/class/net/%s/device/of_node",
             entry->d_name) >= (int)sizeof(path))
    continue;
```

The check is correct: it skips the entry if the path was truncated. This is a reasonable defensive choice. Not an issue.

---

### Info

**Sysfs path construction and symlink resolution:**

The function constructs `/sys/class/net/<ifname>/device/of_node`, resolves the symlink with `realpath()`, and compares the result against the device tree node path. This is a standard pattern for matching a network interface to a device tree node on Linux. The use of `realpath()` to canonicalise the symlink target is correct.

---

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

### Errors

**FQID wraparound in `qman_pending_fq_by_cgrid_range()`:**

```c
for (; fq.fqid <= QMAN_MAX_FQID; fq.fqid++) {
    /* ... */
}
```

`QMAN_MAX_FQID` is `0x00FFFFFFu` (24 bits). The loop condition is `fq.fqid <= QMAN_MAX_FQID`, and `fq.fqid` is a `u32`. When `fq.fqid` reaches `0x00FFFFFF` and is incremented, it wraps to `0x01000000`, which is greater than `QMAN_MAX_FQID`, so the loop terminates. This is correct.

However, the comment says "FQID space is 24 bits wide; stop before wrapping," which is slightly misleading--the loop does not "stop before wrapping"; it wraps and then the condition fails. The behaviour is correct, but the comment could be clearer. This is cosmetic and not an error.

---

**Early termination on query failure:**

```c
err = qman_query_fq_np(&fq, &np);
if (err == -ERANGE) {
    /* FQID is not implemented... */
    break;
} else if (err) {
    DPAA_PMD_WARN("Failed(%d) to Query np FQ(fqid=0x%x)",
                  err, fq.fqid);
    return err;
}
```

When `qman_query_fq_np()` returns an error other than `-ERANGE`, the function returns immediately, aborting the scan. This means a transient query failure on one FQID would leave the rest of the range unscanned, potentially leaving stale FQs behind.

A more robust approach would be to log the error and continue scanning, only returning failure if the final result requires knowing whether the range is clean. However, given that this runs from the device close path and query failures are not expected in normal operation, the current behaviour (fail-fast on error) is defensible. The function is marked `__rte_internal` and is only called from one place, so the behaviour is known to its caller.

Not flagging this as an error, but noting it as a design choice.

---

### Warnings

**CGRID leak on partial cleanup failure:**

In `dpaa_eth_dev_close()`:

```c
cgrid_ok = dpaa_cgr_stale_fq_cleanup(dev, dpaa_intf->cgr_rx[0].cgrid,
                      dpaa_intf->nb_rx_queues, "rx");
/* ... shut down FQs and delete CGRs ... */
if (cgrid_ok)
    qman_release_cgrid_range(dpaa_intf->cgr_rx[0].cgrid,
            dpaa_intf->nb_rx_queues);
```

`cgrid_ok` is set to `false` if any of the following fail:
- Stale FQ scan finds an FQ that refuses to shut down
- Any of the port's own FQs refuse to shut down
- Any CGR deletion fails

The code deliberately leaks the CGRIDs in these cases (by not calling `qman_release_cgrid_range()`), which the comment explains is safer than reusing a CGRID that may still be referenced by a live FQ.

This is correct and intentional. Not an error.

---

**Probe failure path now cleans up CGRs:**

In the `free_tx:` and `free_rx:` labels in `dpaa_dev_init()`, the patch adds:

```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 (tx_cgrid_allocated)
    qman_release_cgrid_range(cgrid_tx[0], MAX_DPAA_CORES);
```

This is a correctness fix: previously, the probe error path would leak CGRs and CGRIDs. The loop bounds (`nb_tx_cgr`, `nb_rx_cgr`) are incremented only after `dpaa_tx_queue_init()` / `dpaa_rx_queue_init()` return success, so only successfully initialised queues are cleaned up. This is correct.

---

**Changed probe behaviour: CGR creation failure now fails probe:**

Previously, `dpaa_rx_queue_init()` and `dpaa_tx_queue_init()` would log a warning if `qman_create_cgr()` failed and continue without tail drop on that queue. The patch changes both functions to return the error immediately:

```c
ret = qman_create_cgr(cgr_rx, QMAN_CGR_FLAG_USE_INIT, &cgr_opts);
if (ret) {
    DPAA_PMD_ERR("rx taildrop init fail onwarning: review has no Review-Result line, severity inferred from prose (response may be truncated)