|WARNING| [v22, 27/27] 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/170746

_AI Code Review_

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

# DPAA Patch Series Review

## Overview

This is a 27-patch series for DPAA drivers containing correctness fixes, style improvements, new features, and documentation updates. The series spans net/dpaa, bus/dpaa, mempool/dpaa, crypto/dpaa_sec, and dma/dpaa components.

---

## Pass 1: Correctness Bugs (Highest Priority)

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

**Error** - Use-after-free / NULL pointer dereference:
- `rte_dpaa_remove()` called `dpaa_eth_dev_close(eth_dev)` unconditionally, then checked if the device was still in use and called `dpaa_eth_dev_close(eth_dev)` again. The first call ran on a potentially NULL `eth_dev` (if `rte_eth_dev_allocated()` returned NULL), causing a NULL dereference. The second call operated on an already-closed device, which is undefined behavior.
- **Fix is correct**: Now checks `eth_dev != NULL` and `eth_dev->state != RTE_ETH_DEV_UNUSED` before calling close once, and logs the close result before releasing the port.

---

### **Patch 02/27: net/dpaa: fix probe success when FMC init fails**

**Error** - Missing error propagation:
- `dpaa_port_fmc_init()` returned a negative error code in `num_rx_fqs`, but the caller did not assign it to `ret` before jumping to the error label. This left `ret = 0` from its initializer, so the probe succeeded with zero Rx queues.
- **Fix is correct**: `ret = num_rx_fqs;` before `goto free_rx;` propagates the error.

---

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

**Error** - Resource leak on error path:
- `rte_dpaa_probe()` created the Tx SG mempool after `dpaa_dev_init()` succeeded. If the mempool creation failed, the function returned `-ENOMEM` directly without closing the device or releasing the port. This abandoned all resources allocated by `dpaa_dev_init()`, including frame queues, congestion groups, and driver-private memory.
- **Fix is correct**: Adds `close_dev:` and `release_port:` labels so the mempool-creation failure path calls `dpaa_eth_dev_close()` and `rte_eth_dev_release_port()`, matching the existing `dpaa_dev_init()` failure path.

---

### **Patch 04/27: net/dpaa: fix free port resources on close**

**Error** - Memory leak:
- `dpaa_intf->tx_conf_queues` is allocated unconditionally in `dpaa_dev_init()` with `rte_zmalloc()`, but it is never freed. It is driver-private memory, so `rte_eth_dev_release_port()` does not free it either. This leaks the allocation on every device close and on every probe failure that occurs after the allocation.
- **Fix is correct**: Adds `rte_free(dpaa_intf->tx_conf_queues)` and sets it to NULL in both `dpaa_eth_dev_close()` and on the `free_tx:` error path in `dpaa_dev_init()`.

**Error** - Early return skipping cleanup:
- `dpaa_eth_dev_close()` returned early for offline (O/H) and ONIC ports before reaching the code that frees `fc_conf`, `cgr_rx`, `cgr_tx`, `rx_queues`, `tx_queues`, and `tx_conf_queues`. This leaked all of those allocations for those port types.
- **Fix is correct**: The early return now jumps to a `clean_1:` label where the common cleanup happens, so offline/ONIC ports also release those resources.

---

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

**Error** - Incorrect bit field extraction:
- `qman_shutdown_fq()` decoded the FQD `dest_wq` field as `channel = dest_wq & 0x7; wq = dest_wq >> 3;`. In the hardware descriptor, the work queue is bits 2:0 and the channel is bits 15:3. The old code extracted a 0-7 value as the channel and the real channel as the work queue. This caused pool-channel FQs to never match the pool-channel range check, so they were not drained correctly on shutdown.
- **Fix is correct**: Adds `qm_fqd_get_chan()` and `qm_fqd_get_wq()` helpers that decode the fields correctly: `channel = be16_to_cpu(fqd->dest_wq) >> QM_FQD_CHAN_OFF;` (where `QM_FQD_CHAN_OFF = 3`) and `wq = be16_to_cpu(fqd->dest_wq) & QM_FQD_WQ_MASK;` (where `QM_FQD_WQ_MASK = GENMASK(2, 0) = 0x7`).

---

### **Patch 10/27: drivers: shutdown DPAA FQ by fq descriptor**

**Info** - API change is correct:
- `qman_shutdown_fq()` now takes `struct qman_fq *fq` instead of `u32 fqid`. This allows the caller to pass the portal the FQ is bound to via `fq->qp`, so the FQ is drained on the correct portal. The old API always used `get_affine_portal()`, which cannot drain an FQ scheduled to a different portal. This is a correctness improvement, not a bug in the old code, because the callers were using it correctly (draining FQs on their own portals). The new API is safer and more flexible.

---

### **Patch 11/27: drivers: add DPAA cgrid cleanup support**

**Error** - CGRID leaked:
- A congestion group allocated by `qman_alloc_cgrid_range()` and created by `qman_create_cgr()` is never released. The CGRID stays reserved until the board is rebooted. `qman_shutdown_fq()` shuts down FQs but does not release the CGRIDs they reference.
- **Fix is correct**: Adds `dpaa_cgr_stale_fq_cleanup()` to find and shut down any FQs still attached to a CGRID range before the CGR is deleted, and calls `qman_release_cgrid_range()` after all CGRs in the range are deleted. This happens in `dpaa_eth_dev_close()` for both the Rx and Tx CGRID ranges.

**Error** - CGRID with member FQ cannot be deleted:
- QMan requires that a CGR have no member FQs before it is deleted. The old code did not ensure this, so deleting a CGR that still had FQs scheduled with CGE set would fail.
- **Fix is correct**: Scans the FQID space for any FQ still scheduled to the CGRID range, shuts them down, and only then deletes the CGR. The scan is O(FQID_space) but only runs once per port close (all CGRIDs in a range are scanned together), and only for ports that use tail drop, so the cost is acceptable.

**Error** - Changing `qman_create_cgr()` failure from warning to error is correct:
- The old code warned when `qman_create_cgr()` failed and continued without tail drop on that queue. However, the caller counted one CGR per queue it initialized and deleted `cgr_rx[0..n)` on its error path. If a queue was left without a CGR, the caller would later delete an object that was never added to the portal list, whose `node` is still zeroed from `rte_zmalloc()`. This is a use-after-free (or use-of-uninit) bug.
- **Fix is correct**: Fail the probe with the error from `qman_create_cgr()` so that the caller's CGR count matches reality.

---

### **Patch 12/27: bus/dpaa: improve FQ shutdown with channel validation**

**Error** - Unbounded wait for FQRN:
- The old `qman_shutdown_fq()` had a `do { drain_dqrr(); found_fqrn = drain_mr(); } while (!found_fqrn);` loop. If the FQ never retired (e.g., a push-mode Rx queue scheduled on another portal's dedicated channel), the loop would spin forever, hanging the caller.
- **Fix is correct**: Adds `QMAN_FQRN_WAIT_MAX = 10000000u` loop bound and returns `-EBUSY` if the FQRN does not arrive within that many iterations. This prevents the infinite loop.

**Error** - Draining an FQ on the wrong portal:
- The old code tried to drain an FQ scheduled to a dedicated channel by setting `QM_SDQCR_CHANNELS_DEDICATED` on the calling portal. But `QM_SDQCR_CHANNELS_DEDICATED` only dequeues the calling portal's own channel. If the FQ is scheduled to another portal's dedicated channel (e.g., a push-mode Rx queue), the FQRN would never arrive, and the loop would spin forever (now caught by the bound above, but still wrong).
- **Fix is correct**: Checks that the FQ's channel matches `p->config->channel` before trying to drain a dedicated-channel FQ. If it does not match, returns `-EBUSY` immediately rather than waiting for an FQRN that will never come.

**Error** - Draining a DCP portal channel FQ:
- The old code did not check whether the FQ's channel was in the DCP portal range (e.g., FM0). A DCP FQ cannot be drained by a user-space portal.
- **Fix is correct**: Returns `-EBUSY` for any channel in the DCP range (>= pool_ch_end) rather than attempting a drain that will never succeed.

---

### **Patch 16/27: mempool/dpaa: fix write after free on pool free**

**Error** - Use-after-free:
- `dpaa_mbuf_free_pool()` freed `mp->pool_data` with `rte_free(mp->pool_data);` and then wrote to it: `bp_info->bp = NULL;`. `bp_info` is `DPAA_MEMPOOL_TO_POOL_INFO(mp)`, which is `mp->pool_data`, so both refer to the same allocation. Writing `bp_info->bp` after the `rte_free()` is a use-after-free.
- **Fix is correct**: Clears `bp_info->bp` before freeing the allocation, and frees `bp_info` directly rather than the alias `mp->pool_data`.

---

### **Patch 17/27: drivers: release DPAA bpid on driver destructor**

**Error** - BPID leaked:
- A BPID allocated by `dpaa_mbuf_create_pool()` is only returned to the kernel allocator from `dpaa_mbuf_free_pool()`. An application that exits without calling `rte_mempool_free()` leaks the BPID, and the ID stays reserved until the board is rebooted.
- **Fix is correct**: Adds a static per-BPID table `s_dpaa_bpid_allocated_flag[]` to track which BPIDs were allocated by this process. Adds a driver destructor `dpaa_mpool_finish()` that walks the table and releases any BPID still marked in use at process exit via the new `bman_free_bpid()` helper. The destructor cannot touch the `bman_pool` object because it lives in EAL memory that `rte_eal_cleanup()` may already have detached, so `bman_free_bpid()` releases the ID from the flags alone.

---

### **Patch 26/27: crypto/dpaa_sec: improve crypto fq resource handling**

**Error** - FQID leaked on error path:
- When `dpaa_sec_init_tx()` or `qman_create_fq()` failed during probe, the function jumped to `init_error:` without shutting down the already-created FQs or releasing their FQIDs. The FQIDs stay reserved until the board is rebooted.
- **Fix is correct**: On the error paths, walks the FQs created so far, calls `qman_shutdown_fq()` for each, and then releases the allocated FQID range with `qman_release_fqid_range()`. Adds `init_error1:`, `init_error2:`, and `init_error3:` labels to handle cleanup at different stages.

**Error** - No cleanup in `dpaa_sec_uninit()`:
- `dpaa_sec_uninit()` returned immediately for a NULL device, but it did not shut down the FQs or release the FQIDs on a normal uninit path. This leaks the FQIDs when the cryptodev is removed or when the process exits.
- **Fix is correct**: Adds code in `dpaa_sec_uninit()` to walk both the Tx and Rx FQs, call `qman_shutdown_fq()` for each, and release the FQIDs. Only runs in the primary process (a secondary does not own the FQs).

---

## Pass 2: Remove Style/Process False Positives

### Items to Keep

All items flagged as **Error** above are correctness bugs and should remain in the review.

### Items to Remove

Reviewing each style/process item from earlier pass:

None of the style or process items in this series concluded with "is actually correct" or "no issue here". All style items identified actual issues. Therefore, no style/process items need to be removed.

---

## Warnings

### **Patch 08/27: bus/dpaa: scan max BPID from DTS**

**Warning** - `const rte_be32_t *range;` not `const u32 *`:
- The device tree property `fsl,bpid-range` is parsed with `of_get_property()`, which returns big-endian 32-bit values. The code declares `const rte_be32_t *range;` and uses `rte_be_to_cpu_32(range[INDEX])` to convert each value. This is correct endianness handling. However, the device tree parsing code in the same file (`bman_init_ccsr()`, `bman_global_init()`) uses `const u32 *` for similar properties and does not byte-swap them. This inconsistency is a **potential bug** in the existing code, not in this patch. The patch is more correct than the surrounding code.
- **No issue in this patch.** Do not flag.

---

### **Patch 13/27: drivers: add BMI Tx statistics**

**Info** - Conditional reporting of zero is correct:
- `fman_if_bmi_stats_get_all()` now reports zero for register blocks that are not mapped for the current port type (e.g., Tx BMI stats for an Rx-only port). This is the correct behavior: the caller expects a fixed number of values in a fixed order, so all counters must be present. Reporting zero for unmapped registers is safer than leaving the buffer uninitialized or changing the output array size.
- **No issue.** Do not flag.

---

### **Patch 18/27: dma/dpaa: add devargs for SG and errata workaround**

**Info** - Gating existing features with devargs:
- This patch does not add new features; it makes existing always-on features (SG batching, data validation, ERR050757 programming) configurable at runtime through devargs. The default behavior is unchanged: SG batching stays on, data validation stays off (except when explicitly enabled), and ERR050757 programming stays on (unless disabled). This is a usability improvement with no correctness risk.
- **No issue.** Do not flag.

---

### **Patch 19/27: net/dpaa: support Rx/Tx taildrop threshold devarg**

**Info** - `dpaa_get_devargs_int()` bounds checking is correct:
- The helper parses a `long` from the devarg string, then range-checks it before narrowing to `unsigned int`. The code explicitly checks `td_val < 0 || td_val > UINT16_MAX` and clamps or warns as appropriate. This prevents truncation bugs and is a good pattern.
- **No issue.** Do not flag.