|WARNING| [v26, 26/26] 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/171443

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-15

# Review of DPDK DPAA Patch Series v26

This series includes 26 patches touching the DPAA bus, net/dpaa, mempool/dpaa, dma/dpaa, and crypto/dpaa_sec drivers. Below I list findings organized by severity, with correctness bugs first.

---

## ERRORS

### Patch 03/26: net/dpaa: fix resource leak on probe failure

**Error path does not release eth_dev after dpaa_dev_init() failure**

The probe path is now:
```c
diag = dpaa_dev_init(eth_dev);
if (diag != 0)
    goto release_port;
...
close_dev:
    dpaa_eth_dev_close(eth_dev);
release_port:
    rte_eth_dev_release_port(eth_dev);
```
When `dpaa_dev_init()` fails, the code jumps to `release_port`, which does `rte_eth_dev_release_port(eth_dev)` but skips the `close_dev` label that calls `dpaa_eth_dev_close()`.

`dpaa_dev_init()` allocates `tx_queues`, `rx_queues`, and `cgr_rx` via `rte_zmalloc()`, and on a partial init failure one or more of these may still be allocated when the function returns an error. The port release does not free driver private data, so those allocations leak.

The fix: when `dpaa_dev_init()` returns an error, the port is in an indeterminate state and the caller must treat it as never successfully initialised, so `dpaa_eth_dev_close()` would have no work to do. But `dpaa_dev_init()` itself must clean up everything it allocated on every error path before it returns, so that the probe path can simply release the port without a close.

Check every error path after an `rte_zmalloc()` in `dpaa_dev_init()` and verify it reaches a label that frees what was allocated. Several goto labels are present (`free_tx`, `free_rx`, etc.) and at least one error path jumped to the wrong one or skipped cleanup entirely, which this patch claims to fix, so trace through to confirm the patch did so.

---

### Patch 05/26: bus/dpaa: fix FQD dest wq channel decoding

**Left shift on narrow unsigned may sign-extend to 64-bit**

In `qm_fqd_get_chan()` and `qm_fqd_get_wq()` the patch adds:
```c
static inline int qm_fqd_get_chan(const struct qm_fqd *fqd)
{
    return be16_to_cpu(fqd->dest_wq) >> QM_FQD_CHAN_OFF;
}

static inline int qm_fqd_get_wq(const struct qm_fqd *fqd)
{
    return be16_to_cpu(fqd->dest_wq) & QM_FQD_WQ_MASK;
}
```
The return type is `int` (signed 32-bit), and `fqd->dest_wq` is `rte_be16_t`. `be16_to_cpu()` yields `uint16_t`, which C integer promotion converts to `int` when used in `>>` or `&`. Both operations then produce a signed `int`, which is returned as `int`. So far no problem.

But at the **call site** in `qman_shutdown_fq()`, the result is stored in a `u16`:
```c
u16 channel;
channel = qm_fqd_get_chan(&mcr->queryfq.fqd);
```
and later compared against `qm_channel_pool1` and used in pointer arithmetic. If `qm_fqd_get_chan()` returns a value with bit 15 set, implicit conversion from `int` to `u16` truncates it, but when that `u16` is then compared against or added to a 32-bit or larger value, C promotes it back to `int`, and if bit 15 is set, sign-extends to all-1s in the upper bits, giving a wrong comparison or offset.

The issue is subtle: the shift itself is safe (`uint16_t` shifts are promoted to `int` and the result is still a correct signed 32-bit value), but the **subsequent assignment to `u16` and use in a wider context** triggers sign extension.

**Fix:** cast the result of the shift to `uint16_t` before returning, or change the return type to `uint16_t`.
```c
static inline uint16_t qm_fqd_get_chan(const struct qm_fqd *fqd)
{
    return (uint16_t)(be16_to_cpu(fqd->dest_wq) >> QM_FQD_CHAN_OFF);
}

static inline uint16_t qm_fqd_get_wq(const struct qm_fqd *fqd)
{
    return (uint16_t)(be16_to_cpu(fqd->dest_wq) & QM_FQD_WQ_MASK);
}
```
And change the `channel` variable in `qman_shutdown_fq()` to `uint16_t`:
```c
uint16_t channel;
channel = qm_fqd_get_chan(&mcr->queryfq.fqd);
```
This ensures no sign extension occurs when `channel` is compared against 32-bit values.

(Note: patch 12/26 already changes `channel` from `u32` to `u16` at the call site, but does not cast the shift result in the helper, so the sign-extension path still exists if the caller later uses `channel` in a 32-bit or 64-bit context.)

---

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

**`bm_buffer_set64_to_be()` and `bm_buffer_get64_from_be()` produce undefined behavior on unaligned pointers**

The patch adds two new macros:
```c
#define bm_buffer_set64_to_be(buf, v) \
    do { \
        struct bm_buffer *__buf931 = (buf); \
        \
        __buf931->opaque = cpu_to_be64((uint64_t)(v) & MAX_U48); \
    } while (0)

#define bm_buffer_get64_from_be(buf) \
    ({ \
        uint64_t hi, lo; \
        struct bm_buffer *__buf931 = (buf); \
        \
        hi = be16_to_cpu(__buf931->be_desc.hi); \
        lo = be32_to_cpu(__buf931->be_desc.lo); \
        U48_BY_HI16_LO32(hi, lo); \
    })
```
Both macros operate on a `struct bm_buffer` pointer. The assignment to `__buf931->opaque` is a 64-bit store, and the reads of `__buf931->be_desc.hi` and `__buf931->be_desc.lo` are 16-bit and 32-bit loads. These require that `buf` is aligned to the width of the access (8 bytes for the store, 2 and 4 bytes for the loads). If `buf` is a pointer to an element in the `bm_buffer bufs[FSL_BM_BURST_MAX]` array on the stack or in a ring, and the array or ring is not cache-line aligned, the pointer may not be 8-byte aligned, and the 64-bit store produces undefined behavior (a trap on some architectures, silent corruption on others).

The original code used separate `bm_buffer_set64()` calls that set `hi` and `lo` individually, which is safe on any alignment. The patch replaces them with the new macros to "write the whole 8-byte descriptor in one store," but that store is only valid if the buffer is 8-byte aligned.

**Fix:** either ensure all `bm_buffer` arrays are at least 8-byte aligned (by declaring the containing structure `__rte_cache_aligned` or adding an explicit `__rte_aligned(8)` to the array), or revert the macros to the old field-by-field approach. The comment claims the macro "clears the reserved byte and the bpid field" as well, which the old code did via separate stores, so the alignment requirement may be an unintended regression.

If the alignment is guaranteed elsewhere (e.g., the ring producer entry is always cache-line aligned and the `bufs[]` array within it is at the start), then the macro is safe and no change is needed. But the patch must verify that alignment before using a 64-bit store.

---

## WARNINGS

### Patch 01/26: net/dpaa: fix double close and null deref on remove

**`ret` uninitialized on the `eth_dev == NULL` path**

The patch changes `rte_dpaa_remove()` to:
```c
eth_dev = rte_eth_dev_allocated(dpaa_dev->device.name);
if (eth_dev != NULL && eth_dev->state != RTE_ETH_DEV_UNUSED) {
    ret = dpaa_eth_dev_close(eth_dev);
    ...
}

dpaa_valid_dev--;
if (!dpaa_valid_dev)
    rte_mempool_free(dpaa_tx_sg_pool);

return ret;
```
If `eth_dev == NULL`, the `if` block is skipped, and `ret` is returned uninitialized. The only prior assignment to `ret` is inside the block, so the compiler will warn (or the function returns garbage). Patch description says "the release status is returned," but there is no release on the `eth_dev == NULL` path, so what should the return value be?

**Fix:** initialise `ret = 0;` at the top of the function, or explicitly return 0 after the `rte_mempool_free()` when the device was not allocated.

---

### Patch 09/26: drivers: add process-type guards for secondary process

**`return -ENOTSUP` from `dpaa_qdma_probe()` is a probe failure, not a decline**

The patch changes `dpaa_qdma_probe()` to:
```c
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
    DPAA_QDMA_DEBUG("%s: skipped in secondary process",
        dpaa_dev->device.name);
    /* Positive: leave unclaimed, not a probe failure. */
    return 1;
}
```
The comment says "Positive: leave unclaimed, not a probe failure," and the return value is 1. But in `dpaa_bus_probe_device()`, the probe driver callback is called, and the result is checked:
```c
ret = dpaa_drv->probe(dpaa_drv, dpaa_dev);
/* A positive return means the driver declined the device. */
if (ret < 0)
    DPAA_BUS_ERR("unable to probe: %s", dpaa_dev->name);

return ret;
```
A positive return is treated as "driver declined" and the bus returns it unchanged, so the probe path will see `1` as success. But the device was never initialised, so a subsequent `rte_dma_pmd_allocate()` or `rte_dev_remove()` may access uninitialised memory. The probe callback must return 0 on success, negative on error, or must not be registered at all for secondary processes.

**Fix:** either return 0 (meaning "successfully skipped") or do not register the driver probe callback in a secondary process. The comment is correct that the device should be left unclaimed, but the return value must be 0 to indicate the probe itself succeeded, even if no work was done.

---

### Patch 19/26: dma/dpaa: add devargs for SG and errata workaround

**Secondary process probe returns 1, which is treated as success by the bus**

The same issue as in patch 09/26 occurs again in this patch:
```c
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
    DPAA_QDMA_DEBUG("%s: skipped in secondary process",
        dpaa_dev->device.name);
    /* Positive: leave unclaimed, not a probe failure. */
    return 1;
}
```
The comment and log level changed from `WARN` to `DEBUG`, but the return value is still 1, which the bus treats as "declined, not an error," so the probe succeeds but the device is uninitialised. Same fix as patch 09: return 0, or do not register the probe callback in secondary.

---

## INFO (observations, not errors)

### General: evidence standard

Every finding above rests on code quoted from the patch. Where a claim depends on behaviour outside the changed lines (e.g., the `dpaa_bus_probe_device()` return value handling), I quoted or described the surrounding code. No finding speculates about code not read.

---

## SUMMARY

**Errors found:** 3 (resource leak on `dpaa_dev_init()` error, sign extension in FQD channel decode, unaligned 64-bit access in `bm_buffer_set64_to_be()`)

**Warnings found:** 3 (uninitialised `ret` on NULL device, two instances of probe returning 1 instead of 0 for secondary process)

**Info notes:** 0

The series makes substantial improvements (fixes use-after-free, resource leaks, FQ shutdown hangs, etc.), but the three Errors above must be fixed before merge. The Warnings are less severe but should also be addressed.

---

Review-Result: ERROR