|WARNING| [v11, 25/25] doc: update release notes with NXP DPAA changes

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-13

# DPAA Patch Series Review

This is a 25-patch series enhancing the NXP DPAA bus, network, crypto, and DMA drivers. The patches cover correctness fixes, code style updates, new features (offline port, VSP enhancements, rate limiting, BMI statistics), and infrastructure improvements (BPID/CGRID cleanup, destructor gating, atomic API migration).

I reviewed all patches against the guidelines in AGENTS.md, focusing on correctness bugs and high-confidence issues. Below are the findings organized by severity and patch.

---

## Patch 01: net/dpaa: fix device remove

### Errors

**1. Resource leak on `rte_eth_dev_release_port()` error (Error)**

If `rte_eth_dev_release_port()` fails (non-zero return), the patch logs a warning but continues to decrement `dpaa_valid_dev`. The allocated device remains in the system but the driver bookkeeping is corrupted.

```c
if (eth_dev && eth_dev->state != RTE_ETH_DEV_UNUSED) {
    int close_ret = dpaa_eth_dev_close(eth_dev);
    if (close_ret)
        DPAA_PMD_WARN("%s: close failed (%d), releasing port anyway",
            dpaa_dev->device.name, close_ret);
    ret = rte_eth_dev_release_port(eth_dev);  // ret not checked before decrement
}
dpaa_valid_dev--;  // decremented even if release_port() fails
```

**Fix**: Check `ret` from `rte_eth_dev_release_port()` and only decrement `dpaa_valid_dev` if it succeeds. If release fails, return the error code without decrementing.

```c
ret = rte_eth_dev_release_port(eth_dev);
if (ret == 0)
    dpaa_valid_dev--;
else
    DPAA_PMD_ERR("%s: release_port failed (%d), port leaked", dpaa_dev->device.name, ret);
return ret;
```

---

## Patch 04: drivers: add process-type guards for secondary process

### Info

**1. Early return style inconsistency (Info)**

In `rte_dpaa_remove()` the early return for secondary processes uses a ternary operator where a simple if-statement would be clearer:

```c
if (rte_eal_process_type() != RTE_PROC_PRIMARY)
    return eth_dev ? rte_eth_dev_release_port(eth_dev) : 0;
```

Suggested (clearer control flow):

```c
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
    if (eth_dev)
        return rte_eth_dev_release_port(eth_dev);
    return 0;
}
```

---

## Patch 06: bus/dpaa: improve FQ shutdown with channel validation

No issues found. The patch correctly validates channel range against DTS-derived values and only drains FQs when the channel is in a drainable range (pool or dedicated). The error path for out-of-range channels returns `-EBUSY` as expected.

---

## Patch 07: bus/dpaa: add DPAA cgrid cleanup support

No issues found. The `qman_find_fq_by_cgrid()` loop is bounded by `QMAN_MAX_FQID` (0x00FFFFFF) and the function correctly handles the case where no FQ is found by returning `-ERANGE`.

---

## Patch 11: drivers: release DPAA bpid on driver destructor

### Warnings

**1. Hardcoded RTE_FINI priority may conflict with other destructors (Warning)**

The patch defines a new priority macro `RTE_PRIORITY_104` and uses it for the mempool destructor. The DPAA bus destructor (added in patch 24) uses priority 102. These priority values are hardcoded without documentation of the ordering requirement. If the mempool destructor runs before the bus destructor, BPIDs may be freed while still in use by active pools.

**Suggested fix**: Add a comment documenting the required destructor order (bus 102 - mempool 104) and why.

```c
/* Priority 104 ensures this runs after the DPAA bus destructor (102)
 * so that all pools are released before BPIDs are freed.
 */
RTE_FINI_PRIO(dpaa_mpool_finish, 104)
```

---

## Patch 12: dma/dpaa: add SG data validation and ERR050757

No correctness issues found. The ERR050757 workaround logic is only active when `s_pci_read` is set (via devarg or default true when the errata define is present). The SG and data validation features are both opt-in via devargs. Descriptor setup code correctly handles the conditional workaround.

---

## Patch 13: net/dpaa: support Rx/Tx taildrop threshold devarg

No issues found. The devarg parsing uses `strtol()` with proper error checking (`errno`, end pointer), and the parsed value is range-checked against `UINT16_MAX` before assignment.

---

## Patch 14: net/dpaa: add Tx rate limiting API

### Errors

**1. Resource leak on early error path (Error)**

In `rte_pmd_dpaa_port_set_rate_limit()`, if the port handle does not exist the function calls `fm_port_open()` and stores the result in `handle`. On success it later calls `fm_port_close(handle)`. However if `fm_port_set_rate_limit()` or `fm_port_delete_rate_limit()` fails, the function returns the error code without closing `handle` in the `!port_handle_exists` case:

```c
if (!dpaa_intf->port_handle) {
    handle = fm_port_open(&fm_port_params);  // allocated
    if (!handle) {
        DPAA_PMD_ERR("fm_port_open failed for port %s", dpaa_intf->name);
        return -ENODEV;
    }
    port_handle_exists = false;
}

if (burst == 0 || rate == 0)
    ret = fm_port_delete_rate_limit(handle);
else
    ret = fm_port_set_rate_limit(handle, &port_rate_limit);

if (ret) {
    DPAA_PMD_ERR("Failed to set rate limit ret = %d", ret);
    if (!port_handle_exists)
        fm_port_close(handle);  // only closed here
    return ret;
}

// success path
if (!port_handle_exists)
    fm_port_close(handle);

return 0;
```

This is correct. On re-inspection the error path **does** close the handle before returning. No leak.

**Correction**: No issue.

---

## Patch 18: bus/dpaa: optimize DPAA multi-entry buffer pool operations

No issues found. The change from hardcoded `8` to `FSL_BM_BURST_MAX` is a cleanup and the descriptor copy logic is correct (copy first entry opaque, then memcpy remaining entries).

---

## Patch 20: drivers: improve shutdown fq with channel

### Errors

**1. CGRID release on error path after partial deletion (Error)**

In `dpaa_eth_dev_close()`, the Rx CGR cleanup loop calls `qman_find_fq_by_cgrid()` to check if any FQ still uses the CGR. If found, it calls `qman_shutdown_fq_by_fqid()` on that FQ. If shutdown fails, the code logs a warning but continues. After the loop, it unconditionally calls `qman_release_cgrid_range()`:

```c
for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++) {
    ret = qman_find_fq_by_cgrid(dpaa_intf->cgr_rx[loop].cgrid, &fqid);
    if (!ret) {
        ret = qman_shutdown_fq_by_fqid(fqid);
        if (ret) {
            DPAA_PMD_WARN("Failed(%d) to shutdown fq(fqid=0x%x)", ret, fqid);
        }
    }
    ret = qman_delete_cgr(&dpaa_intf->cgr_rx[loop]);
    if (ret) {
        DPAA_PMD_WARN("%s: delete rxq%d's cgr err(%d)", dev->data->name, loop, ret);
    }
}
qman_release_cgrid_range(dpaa_intf->cgr_rx[0].cgrid, dpaa_intf->nb_rx_queues);
```

If `qman_shutdown_fq_by_fqid()` or `qman_delete_cgr()` fails for any CGR, the loop continues and the CGRID range is released anyway. This can result in releasing a CGRID that is still in use by an un-deleted CGR, corrupting the CGRID allocator state.

**Fix**: Only call `qman_release_cgrid_range()` if all CGRs were successfully deleted. Track deletion status in the loop and skip the release if any failed.

```c
int all_deleted = 1;
for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++) {
    // ... shutdown FQ if found ...
    ret = qman_delete_cgr(&dpaa_intf->cgr_rx[loop]);
    if (ret) {
        DPAA_PMD_WARN("%s: delete rxq%d's cgr err(%d)", dev->data->name, loop, ret);
        all_deleted = 0;
    }
}
if (all_deleted)
    qman_release_cgrid_range(dpaa_intf->cgr_rx[0].cgrid, dpaa_intf->nb_rx_queues);
else
    DPAA_PMD_WARN("%s: CGR deletion incomplete, CGRID range leaked", dev->data->name);
```

The same pattern appears for Tx CGRs. Both should be fixed.

---

## Patch 23: drivers: improve crypto fq resource handling

### Errors

**1. FQID range leak on partial init failure (Error)**

In `dpaa_sec_dev_init()`, if Tx FQ creation succeeds for some queues but fails for queue `i`, the code jumps to `init_error1` which shuts down the first `i` Tx FQs and then releases the entire FQID range:

```c
for (i = 0; i < internals->max_nb_queue_pairs; i++) {
    ret = dpaa_sec_init_tx(&qp->outq, fqids[i]);
    if (ret) {
        DPAA_SEC_ERR(...);
        goto init_error1;
    }
}
// ...
init_error1:
for (j = 0; j < i; j++) {
    qp = &internals->qps[j];
    ret = qman_shutdown_fq(&qp->outq);
    if (ret) {
        DPAA_SEC_ERR(...);
    }
}
qman_release_fqid_range(fqids[0], internals->max_nb_queue_pairs);
```

If `qman_shutdown_fq()` fails for any of the FQs, that FQ is still allocated but the FQID is released back to the allocator. This corrupts the FQID allocator state.

**Fix**: Track how many FQs were successfully shut down and only release the corresponding FQID count. Alternatively, log a fatal error and do not release the range if shutdown fails.

The same issue exists for Rx FQs at `init_error3`.

---

## Summary

**Errors** (must fix):
1. **Patch 01**: Resource leak when `rte_eth_dev_release_port()` fails but `dpaa_valid_dev` is still decremented
2. **Patch 20**: CGRID range released after partial CGR deletion failure, corrupting allocator state (affects both Rx and Tx CGRs)
3. **Patch 23**: FQID range released after partial FQ shutdown failure, corrupting FQID allocator state (affects both Tx and Rx FQIDs)

**Warnings**:
1. **Patch 11**: Hardcoded destructor priorities without documentation of ordering requirement

**Info**:
1. **Patch 04**: Ternary operator in early return could be clearer as an if-statement

All other patches are correct.