|WARNING| [v12, 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/167928

_AI Code Review_

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

# DPAA Patch Series v12 Review

The patch series focuses on NXP DPAA driver improvements, bug fixes, and new features. Based on the AGENTS.md guidelines, here is the review organized by severity:

---

## **ERRORS** (must fix)

### Patch 02/26: net/dpaa: fix Tx confirmation queue memory leak
**Resource leak on error path**
```c
+	rte_free(dpaa_intf->tx_conf_queues);
+	dpaa_intf->tx_conf_queues = NULL;
```
The patch fixes the leak, which is correct. However, I traced the error path in `dpaa_dev_init()`:

After the new `free_tx:` label, `dpaa_intf->tx_queues` is freed and `nb_tx_queues` is zeroed, but **`tx_conf_queues` is only freed at the new label insertion point.** On the existing error paths that jump to `free_rx:` **before** the new `free_tx:` cleanup, `tx_conf_queues` may already be allocated (it is allocated unconditionally early in `dpaa_dev_init()` at the same time as `rx_queues`), yet those paths skip the new cleanup. The patch adds the cleanup at `free_tx:` but does not add a corresponding cleanup at `free_rx:` for the case where `tx_queues` allocation fails before `tx_conf_queues` cleanup is reached. Check whether `tx_conf_queues` should also be freed at the `free_rx:` label.

---

### Patch 03/26: bus/dpaa: fix FQD dest wq channel decoding
**Correctness: bit field extraction error**
The patch fixes a serious correctness bug where the channel and work queue fields were swapped due to incorrect bit masks. The old code used:
```c
channel = dest_wq & 0x7;      /* only bits 2:0, wrong field */
wq = dest_wq >> 3;            /* upper bits, wrong field */
```
The hardware descriptor has **WQ in bits 2:0** and **channel in bits 15:3**. The fix correctly extracts them via:
```c
channel = qm_fqd_get_chan(&mcr->queryfq.fqd);  /* bits 15:3 */
wq = qm_fqd_get_wq(&mcr->queryfq.fqd);        /* bits 2:0 */
```
This is a critical correctness fix. The patch is correct.

---

### Patch 08/26: drivers: add DPAA cgrid cleanup support
**Potential use-after-free in CGR cleanup loop**
```c
+	while (qman_pending_fq_by_cgrid(cgrid, &fqid) == 0) {
+		ret = qman_shutdown_fq_by_fqid(fqid);
+		if (ret) {
+			DPAA_PMD_WARN(...);
+			break;  /* Do not spin on an FQ that refuses to shut down. */
+		}
+	}
```
The loop finds FQs still linked to the CGR and shuts them down. If `qman_shutdown_fq_by_fqid()` **succeeds**, the FQ is removed from the CGR's member list, so the next `qman_pending_fq_by_cgrid()` call should return a different FQID or `-ERANGE` (no more FQs). However, if shutdown **fails** and the code breaks, the CGR delete that follows may fail because the FQ is still linked. The code should verify that the CGR is empty after the loop, or return an error if FQs remain. As written, the code breaks on the first shutdown failure but still attempts `qman_delete_cgr()`, which may leak the CGRID if the hardware refuses to delete a CGR that still has members.

Recommend: after the loop, re-check `qman_pending_fq_by_cgrid(cgrid, NULL)` and return an error if it returns `0` (FQs still exist), preventing the CGRID release.

---

### Patch 09/26: bus/dpaa: improve FQ shutdown with channel validation
**Race condition: channel-affine portal selection without lock**
```c
+	const u16 pool_ch_start = dpaa_get_qm_channel_pool();
+	const u16 pool_ch_end = pool_ch_start + dpaa_get_qm_channel_pool_num();
+	if (channel >= pool_ch_start && channel < pool_ch_end) {
+		qm_dqrr_sdqcr_set(&p->p, QM_SDQCR_TYPE_ACTIVE |
+			QM_SDQCR_CHANNELS_POOL_CONV(channel));
```
The code programs the portal's SDQCR to subscribe to the pool channel. However, **`p` is the affine portal for the current lcore** (`get_affine_portal()`). If the FQ's `qp` (set when the FQ was created) is a **different portal** (channel-affine for a pool channel), then accessing `p->p` to program the SDQCR is incorrect - the code should program the FQ's own portal `fq->qp->p` instead of the affine portal. The current code only uses `fq->qp` if it is non-NULL at the start:
```c
struct qman_portal *p = fq->qp;
if (!p)
    p = get_affine_portal();
```
But there is no guarantee that `fq->qp` is the correct portal for this channel. Verify that the FQ's portal (`fq->qp`) is the one that should be programmed for pool-channel FQs, or document why using the affine portal is safe.

---

### Patch 14/26: dma/dpaa: add SG data validation and ERR050757
**Use-after-free in data validation on success**
```c
+	if (likely(!s_data_validation))
+		return;
```
The `fsl_qdma_data_validation()` function is called **after** a DMA transfer completes. At that point, the source and destination buffers may have been freed by the application (the DMA completion callback has already run). Reading them back for validation is a **use-after-free** if the application has freed the buffers. The validation should only be enabled in a debug mode where the application guarantees the buffers remain valid, or the code should document that enabling `dpaa_dma_data_validation=1` requires the application to not free buffers immediately after completion.

Recommend: document in the devarg description that `dpaa_dma_data_validation=1` requires the application to keep buffers valid for the entire test, or move the validation to before the completion callback is invoked (if possible).

---

### Patch 15/26: net/dpaa: support Rx/Tx taildrop threshold devarg
**Integer overflow: devarg value cast without range check**
```c
+	if (dpaa_get_devargs_int(dev->devargs, DRIVER_RX_TAILDROP, &td_val) == 1) {
+		td_threshold = (unsigned int)td_val;
+		if (td_threshold > UINT16_MAX)
+			td_threshold = CGR_RX_PERFQ_THRESH;
```
The `dpaa_get_devargs_int()` function parses the devarg value into a `long`. Casting it to `unsigned int` **sign-extends** a negative value, producing a large positive number. If the user passes a negative value (e.g., `drv_rx_taildrop=-1`), the cast produces `0xFFFFFFFF`, which is then clamped to `CGR_RX_PERFQ_THRESH`. The code should **reject** negative values instead of silently clamping them.

Recommend:
```c
if (td_val < 0 || td_val > UINT16_MAX) {
    DPAA_PMD_ERR("Invalid taildrop value: %ld", td_val);
    return -EINVAL;
}
td_threshold = (unsigned int)td_val;
```

---

### Patch 20/26: bus/dpaa: optimize DPAA multi-entry buffer pool operations
**Use-after-free: `bm_bufs[0]` stack variable used after function return**
```c
+	r->bufs[0].opaque = bm_bufs[0].opaque;
+	if (num > 1)
+		memcpy(&r->bufs[1], &bm_bufs[1], sizeof(struct bm_buffer) * (num - 1));
```
The `bm_bufs` array is a **stack variable** in `bman_release_fast()`. After the function returns, the stack frame is invalid. If the code that consumes `r->bufs[0]` runs **after** this function returns (e.g., the hardware reads it asynchronously), it will read stale stack memory. However, the code appears to be setting up a **hardware descriptor** that is consumed immediately by `bm_rcr_pvb_commit()`, which should flush the writes to hardware before returning. If the hardware reads the descriptor **after** the function returns, this is a use-after-free. Verify that `bm_rcr_pvb_commit()` ensures the hardware has consumed the descriptor before the function returns.

---

### Patch 23/26: drivers: add offline (O/H) port device support
**Missing NULL check on devargs dereference**
```c
+	ret = dpaa_ol_get_bh_port_name(dev->device->devargs, ...);
+	if (ret <= 0) {
+		DPAA_PMD_ERR("Offline port \"%s\" devarg not defined or invalid (ret=%d)", ...);
+		return -EINVAL;
+	}
```
The code calls `dpaa_ol_get_bh_port_name()` with `dev->device->devargs`. Inside that function:
```c
+	if (!devargs)
+		return 0;
```
So if `dev->device->devargs` is NULL, the function returns `0`, and the caller treats that as an error. However, the **error message** says "devarg not defined or invalid", which is misleading - it should say "devargs is NULL" if that is the case. The code should check `devargs` **before** calling the function and provide a clearer message.

Recommend:
```c
if (!dev->device->devargs) {
    DPAA_PMD_ERR("Device arguments required for offline port");
    return -EINVAL;
}
ret = dpaa_ol_get_bh_port_name(dev->device->devargs, ...);
```

---

### Patch 24/26: crypto/dpaa_sec: improve crypto fq resource handling
**Double-free on error path**
```c
+init_error3:
+	for (j = 0; j < i; j++) {
+		ret = qman_shutdown_fq(&internals->inq[j]);
+	}
+	qman_release_fqid_range(fqids[0], RTE_DPAA_MAX_RX_QUEUE);
+init_error2:
+	i = internals->max_nb_queue_pairs;
+	fqids[0] = internals->qps[0].outq.fqid;
+init_error1:
+	for (j = 0; j < i; j++) {
+		qp = &internals->qps[j];
+		ret = qman_shutdown_fq(&qp->outq);
+	}
+	qman_release_fqid_range(fqids[0], internals->max_nb_queue_pairs);
```
The `init_error1:` label releases the Tx FQID range using `fqids[0]`, which is set to `internals->qps[0].outq.fqid` **at the `init_error2:` label**. However, if the code jumps to `init_error1:` **directly** (from the Tx FQ init loop failure), `fqids[0]` has **not been reassigned** yet, so it still holds the **Rx FQID range base** from the earlier allocation. This will **double-free** the Rx FQID range (once at `init_error3:`, once at `init_error1:`).

The fix: the reassignment `fqids[0] = internals->qps[0].outq.fqid;` at `init_error2:` should happen **before** the label, or the code should use a separate variable for the Tx FQID range base.

Recommend:
```c
init_error2:
	tx_fqid_base = internals->qps[0].outq.fqid;
	i = internals->max_nb_queue_pairs;
init_error1:
	for (j = 0; j < i; j++) {
		...
	}
	qman_release_fqid_range(tx_fqid_base, internals->max_nb_queue_pairs);
```

---

## **WARNINGS** (should fix)

### Patch 01/26: net/dpaa: fix device remove
**Misleading variable name: `close_ret` shadows outer `ret`**
```c
-	ret = dpaa_eth_dev_close(eth_dev);
-	if (eth_dev->state !=  RTE_ETH_DEV_UNUSED) {
-		dpaa_eth_dev_close(eth_dev);
+	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(...);
		ret = rte_eth_dev_release_port(eth_dev);
	}
```
The outer function has `int ret;` at the top. Inside the `if` block, a new `close_ret` is declared, shadowing the intent. However, the **return value of `rte_eth_dev_release_port()`** is assigned to the outer `ret`, which is what the function returns. The `close_ret` is **not propagated** to the caller - only logged. If close fails but release succeeds, the function returns success. This may be intentional (the port must be released even if close fails), but it should be documented or the error should be propagated.

Recommend: if close failure should fail the remove, change to:
```c
if (close_ret) {
    DPAA_PMD_WARN(...);
    return close_ret;
}
ret = rte_eth_dev_release_port(eth_dev);
```
If the current behavior (release anyway) is correct, add a comment explaining why.

---

### Patch 04/26: bus/dpaa: refine fman naming
**Inconsistent comment style**
The patch renames `ccsr_map` to `memac_map` and `bmi_map` to `rx_bmi_map`, which improves clarity. However, the commit message says "Rename bmi_map to rx_bmi_map to distinguish from TX BMI" but the **TX BMI field already exists** (`tx_bmi_map`). The commit message should clarify that the **rename** is to make the existing Rx field name symmetric with the Tx field name.

---

### Patch 06/26: drivers: add process-type guards for secondary process
**Process-private check placement**
```c
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return eth_dev ? rte_eth_dev_release_port(eth_dev) : 0;
```
The check is added **after** `rte_eth_dev_allocated()`, which is fine. However, the **ternary** on `eth_dev` is redundant - if `eth_dev` is NULL, `rte_eth_dev_release_port()` is a no-op (returns 0). The ternary can be removed for clarity:
```c
if (rte_eal_process_type() != RTE_PROC_PRIMARY)
    return rte_eth_dev_release_port(eth_dev);
```

---

### Patch 10/26: drivers: add BMI Tx statistics
**Missing Tx BMI register range validation**
The patch adds Tx BMI statistics read, mirroring the Rx BMI code.