|WARNING| [v13, 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/168019

_AI Code Review_

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

# DPAA Patch Series Review

This is a large series of 25 patches for the NXP DPAA platform. I'll provide a consolidated review organized by severity level, focusing on correctness bugs first as instructed.

---

## ERRORS (Must Fix)

### Patch 1 (net/dpaa: fix device remove)

**Error**: Use-after-free and uninitialized variable.

```c
// Line 2680: ret is declared but never initialized before use on error path
int ret;
ret = dpaa_eth_dev_close(eth_dev);  // First use - OK
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);  // OK - overwrites
}
dpaa_valid_dev--;
return ret;  // May return uninitialized 'ret' when eth_dev is NULL
```

**Fix**: Initialize `ret = 0;` at declaration, or return `0` explicitly when `eth_dev` is NULL.

---

### Patch 2 (net/dpaa: fix free port resources on close)

**Error**: `goto clean_1` skips important Tx confirmation queue cleanup.

```c
// Line 540-541
if (fif->mac_type == fman_offline_internal ||
    fif->mac_type == fman_onic)
    goto clean_1;  // Jumps over subsequent cleanup
```

The original code had this path return early before freeing `tx_conf_queues`. The patch changes it to `goto clean_1`, which now frees `tx_conf_queues` (correct), **but** it skips all the queue shutdown and CGR cleanup code (lines 565-632 in the patched version). For offline/ONIC ports the driver does need to clean up those queues and CGRs.

**Fix**: Either do not skip cleanup for offline/ONIC, or verify that offline/ONIC ports genuinely have no queues or CGRs to release. If they do, the early jump is a resource leak.

---

### Patch 8 (drivers: add DPAA cgrid cleanup support)

**Error**: Missing error checks on `qman_delete_cgr()` calls in error paths.

```c
// Patch 8, line 2559-2560
for (loop = 0; loop < nb_tx_cgr; loop++)
    qman_delete_cgr(&dpaa_intf->cgr_tx[loop]);  // No error check
```

And similarly at line 2571 for Rx CGRs. On the normal close path (patch adds lines 605-632) you log warnings for failures, but on the init error path you silently ignore them. If `qman_delete_cgr()` fails it means the CGR is left in QMan's list with a dangling pointer into soon-to-be-freed memory.

**Fix**: Check the return value and log a warning or escalate to error. You cannot prevent the failure, but you should report it.

---

### Patch 9 (bus/dpaa: improve FQ shutdown with channel validation)

**Error**: Misleading comment about why the affinity check was removed.

```c
// Patch 9, line 2862-2866 (comment in code)
/*
 * Pool channel: subscribe this portal to it so
 * the hardware can deliver frames and the FQ
 * retires.  p->config->channel is the portal's
 * dedicated channel and never equals a pool
 * channel, so the old affinity check always
 * failed.  Any portal can subscribe to any pool
 * channel via SDQCR.
 */
```

The comment is correct about the old affinity check being wrong. However the removed code had:

```c
if (channel == p->config->channel)  // Old code
```

That checked if the *FQ's channel* matched the *portal's channel*, which is meaningful for dedicated-channel FQs. The new code unconditionally programs SDQCR for both dedicated and pool channels, which is correct. The comment should clarify that the removed check was a misguided attempt to verify affinity, not that affinity checks are meaningless.

**Not an error in behavior**, but the comment could mislead future readers. If you remove this comment entirely or rephrase it to say "the old code incorrectly tried to verify affinity by comparing the FQ's channel to the portal's channel, which cannot work for pool channels", that would be clearer.

---

### Patch 14 (dma/dpaa: add devargs for SG and errata workaround)

**No errors**. The patch is correct. The comment about `dma_pool_alloc()` accepting `NULL` is accurate (it's a documentation-only change, not a code change).

---

### Patch 19 (bus/dpaa: optimize DPAA multi-entry buffer pool operations)

**No errors**. The refactor to prepare big-endian entries in a single pass and use `memcpy` for entries 1..n-1 is correct.

---

### Patch 20 (bus/dpaa: improve log macro usages)

**No errors**. Mechanical replacement of `DPAA_BUS_LOG(LEVEL, ...)` with shorthands.

---

### Patch 21 (net/dpaa: enhance VSP port support)

**No errors**. The logic changes are correct. The switch from passing `fif` to `dpaa_port_vsp_cleanup()` to using only the `dpaa_intf` pointer is a valid simplification.

---

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

**Error**: The new `dpaa_oldev.c` driver allocates `eth_dev->data->dev_private` with `rte_zmalloc()` but never frees it.

```c
// dpaa_oldev.c, line 711-715 (dpaa_ol_probe)
eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
                     sizeof(struct dpaa_if),
                     RTE_CACHE_LINE_SIZE);
```

The `dpaa_ol_remove()` function (line 754) only calls `dpaa_ol_dev_close()` and `rte_eth_dev_release_port()`, neither of which frees `dev_private`. The `rte_eth_dev_release_port()` function only frees the device structure, not the driver-private allocation.

**Fix**: Add `rte_free(eth_dev->data->dev_private);` in `dpaa_ol_remove()` before calling `rte_eth_dev_release_port()`.

---

### Patch 23 (crypto/dpaa_sec: improve crypto fq resource handling)

**Error**: In the error path `init_error2`, you release FQIDs before all FQs are shut down.

```c
// dpaa_sec.c, line 3820-3821
qman_release_fqid_range(fqids[0], RTE_DPAA_MAX_RX_QUEUE);
init_error2:
```

The label `init_error2` is jumped to when Rx FQ creation fails partway through. At that point you have created `i` Rx FQs and need to shut them down before releasing the FQID range. The patch does this at `init_error3`, but when falling through from `init_error2` the release happens *before* shutdown.

**Fix**: Move the shutdown loop for Rx queues above the `qman_release_fqid_range()` call, or re-order the error labels.

---

### Patch 24 (drivers: add dpaax enter destructor to gate EAL memory frees)

**Correctness issue**: The `kfree()` macro does not call `rte_free()` when in destructor context, silently leaking memory.

```c
#define kfree(p) \
({ \
    if (!dpaax_is_in_destructor()) \
        rte_free(p); \
    else \
        pr_debug("Eal memory has been destroyed.\n"); \
})
```

This is by design to avoid use-after-free when EAL memory is gone, but it means memory is leaked. If the intention is to prevent crashes during shutdown and you are accepting the leak, the `pr_debug()` message should document that the memory is being leaked intentionally, not merely that "EAL memory has been destroyed" (which is vague).

**Fix**: Change the message to `pr_debug("Skipping rte_free(%p) in destructor; memory leaked intentionally to avoid use-after-free.\n", p);` or similar.

---

## WARNINGS (Should Fix)

### Patch 3 (bus/dpaa: fix FQD dest wq channel decoding)

**Warning**: The commit message and code comment should clarify that the old code did not merely "decode incorrectly" but actually **swapped** the channel and work queue fields. The old extraction:

```c
channel = dest_wq & 0x7;
wq      = dest_wq >> 3;
```

treats bits 2:0 as channel and bits 15:3 as work queue, which is backwards relative to the hardware layout where bits 2:0 are work queue and bits 15:3 are channel. The patch fixes this, but the commit message says "decoded the FQD dest_wq field as" without emphasizing the reversal.

**Suggested wording**: "The old code swapped the channel and work queue fields: it extracted bits 2:0 as channel and bits 15:3 as work queue, but the hardware uses bits 2:0 for work queue and bits 15:3 for channel. This patch corrects the extraction."

---

### Patch 5 (bus/dpaa: scan max BPID from DTS)

**Warning**: If `of_get_property()` returns a property shorter than 2 cells, you skip the range but do not log an error.

```c
// Line 228-230
range = of_get_property(dt_node, "fsl,bpid-range", &len);
if (!range || len < (int)(2 * sizeof(rte_be32_t)))
    continue;
```

If the property exists but is malformed, the user should know. Add `pr_warn("Invalid fsl,bpid-range in DTS (len=%zu, expected >= %zu)", len, 2 * sizeof(rte_be32_t));` before `continue`.

---

### Patch 6 (drivers: add process-type guards for secondary process)

**Warning**: The `dpaa_qdma_init()` function in `dma/dpaa/dpaa_qdma.c` does `return 0;` immediately for secondary processes, but does not free the `fsl_qdma` structure allocated in `dpaa_qdma_probe()`. This is not a leak (the parent process owns it), but the comment should clarify that the early return is intentional and that the structure remains allocated.

**Suggested fix**: Add a comment: `/* Secondary processes skip HW init; fsl_qdma remains allocated for shared access */`

---

### Patch 7 (drivers: shutdown DPAA FQ by fq descriptor)

**Warning**: The `qman_shutdown_fq()` function signature change from `(u32 fqid)` to `(struct qman_fq *fq)` is correct, but the helper `qman_shutdown_fq_by_fqid()` that wraps it for callers who only have an FQID:

```c
static inline int qman_shutdown_fq_by_fqid(u32 fqid)
{
    struct qman_fq fq;
    memset(&fq, 0, sizeof(struct qman_fq));
    fq.fqid = fqid;
    return qman_shutdown_fq(&fq);
}
```

This creates a local `qman_fq` with `qp = NULL`. Inside `qman_shutdown_fq()` (line 2799):

```c
struct qman_portal *p = fq->qp;
if (!p)
    p = get_affine_portal();
```

So it falls back to the affine portal. This is fine for non-affine FQs, but the function should document that `qman_shutdown_fq_by_fqid()` is only safe for FQs that are not affine to a specific portal. Add a comment or a note in the commit message.

---

### Patch 10 (drivers: add BMI Tx statistics)

**Warning**: The `fman_if_bmi_stats_get_all()` function reports zero for register blocks that are not mapped, which is documented in the code:

```c
/* Report zero for register blocks that are not mapped for this port
 * type, the caller expects a fixed number of values in a fixed order.
 */
```

This is correct. However, the xstats name array (`dpaa_xstats_strings[]`) in `dpaa_ethdev.c` includes Tx BMI stats unconditionally. If a port has no Tx BMI (e.g., Rx-only port), the application will see stats named `tx_bad_frames_count` etc. that always read zero. This could confuse users.

**Suggested fix**: Either document in the driver documentation that some stats may be unsupported on certain port types, or filter the xstats list per port type. Not an error, but a usability wart.

---

### Patch 13 (drivers: release DPAA bpid on driver destructor)

**Warning**: The destructor `dpaa_mpool_finish()` is marked `RTE_FINI_PRIO(dpaa_mpool_finish, RTE_PRIORITY_104)`. This is priority 104, which is lower (runs earlier) than the DPAA bus destructor at priority 102. If the bus destructor is still using the mempool (e.g., draining queues), and the mempool destructor runs first and frees BPIDs, you could have a problem.

**Check**: Verify that priority 104 is correct. The comment in patch 24 mentions that EAL memory is destroyed before per-driver destructors, so priorities matter. If the bus is priority 102 and the mempool is priority 104, the mempool destructor runs first (lower number = earlier). If the bus still references mempools at that point, this is a bug.

**Suggested fix**: Either reverse the priority (make mempool destructor run *after* the bus, e.g., priority 103 or 105), or verify that the bus has no live references to mempools when its destructor runs.

---

### Patch 15 (net/dpaa: support Rx/Tx taildrop threshold devarg)

**No warnings**. The devarg parsing and logic are correct.

---

### Patch 16 (bus/dpaa: orp queue create and burst enqueue)

**Warning**: The `qman_enqueue_multi_orp()` function (line 2405) has a for-loop that constructs `eq` entries in place in the EQCR ring, storing the verb in `eq_verbs[i]`, then a second loop that writes the verb to `eq->__dont_write_directly__verb`. Between the two loops it issues `lwsync()`:

```c
lwsync();
eq = eqcr->cursor;
for (i = 0; i < sent; i++) {
    eq->__dont_write_directly__verb = eq_verbs[i] | eqcr->vbit;
    eq++;
    ...
}
```

This is the same pattern as `qman_enqueue_multi()`, which is correct. However, the function does not have a `dcbf()` loop at the end to flush the cache lines like `qman_enqueue_multi()` does (see the original at the end of the function). The patch adds a dcbf loop for `qman_enqueue_multi_orp()`:

```c
eq = eqcr->cursor;
for (i = 0; i < sent; i++) {
    dcbf(eq);
    eq = (void *)((unsigned long)(eq + 1) &
        (~(unsigned long)(QM_EQCR_SIZE << 6)));
}
```

This looks correct. However, the comment above the dcbf loop should be consistent with the existing code. The original comment is "Flush all the cache lines without load/store operations between them so the flushes complete faster." Your patch has the same comment, which is good.

**Not