|WARNING| [v9, 25/25] doc: add 26.11 release notes for NXP DPAA new features

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: NXP DPAA Series v9

## Summary

Reviewing 25 patches from Hemant Agrawal for DPDK targeting NXP DPAA platform enhancements. The series adds new features, fixes bugs, and improves VSP/FQ resource handling.

---

## Correctness Issues (Errors)

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

**Use-after-free on error path**

The patch removes an unconditional call to `dpaa_eth_dev_close(eth_dev)` before the NULL check. However, the logic still has issues:

```c
eth_dev = rte_eth_dev_allocated(dpaa_dev->device.name);
if (eth_dev && eth_dev->state != RTE_ETH_DEV_UNUSED) {
    dpaa_eth_dev_close(eth_dev);
    ret = rte_eth_dev_release_port(eth_dev);
}
```

If `eth_dev` is NULL, the function returns without setting `ret`, causing the return value to be uninitialized (the original `ret` from the removed first call). The variable `ret` should be initialized to 0 at declaration.

**Suggested fix:**
```c
int ret = 0;  /* initialize */
```

---

### Patch 04/25: drivers: add process-type guards

**Missing release on secondary process remove path**

The secondary process path calls `rte_eth_dev_release_port()` but never calls `dpaa_eth_dev_close()`, which means resources (FQs, CGRs, pools) are not released on secondary process exit:

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

Secondary processes share some resources (queues, pools) that should be cleaned up symmetrically. The missing cleanup may cause resource leaks when a secondary process exits.

**Suggested fix:** Call `dpaa_eth_dev_close()` before `rte_eth_dev_release_port()` for secondary processes, or document why it's safe to skip.

---

### Patch 06/25: bus/dpaa: improve FQ shutdown

**Error return value discarded**

```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) {
    /* Pool channel - must use affine portal */
    if (p->config->channel != channel) {
        DPAA_PMD_ERR("Portal ch(0x%04x) != FQ ch(0x%04x)",
            p->config->channel, channel);
        ret = -EINVAL;
        goto out;
    }
```

The error path sets `ret = -EINVAL` and jumps to `out`, but later in the function, the drain loop may overwrite `ret` with 0 if the MR drain succeeds:

```c
do {
    qm_dqrr_drain_nomatch(&p->p);
    found_fqrn = qm_mr_drain(&p->p, FQRN);
    cpu_relax();
} while (!found_fqrn);
qm_dqrr_sdqcr_set(&p->p, p->sdqcr);
```

After this, there's no `if (ret) goto out;` to preserve the error, so the function continues and may return 0. The `ret` from the channel mismatch is lost.

**Suggested fix:** Add `if (ret) goto out;` after the drain loop or restructure to preserve the error code.

---

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

**Use-after-free in destructor: releasing rte_dpaa_bpid_info after freeing bp_info**

```c
static void
dpaa_mbuf_free_pool(struct rte_mempool *mp)
{
    struct dpaa_bp_info *bp_info = DPAA_MEMPOOL_TO_POOL_INFO(mp);
    // ...
    rte_dpaa_bpid_info[bp_info->bpid].mp = NULL;
    rte_dpaa_bpid_info[bp_info->bpid].bp = NULL;
    s_dpaa_bpid_allocated_flag[bp_info->bpid].used = false;
    rte_free(bp_info);  /* bp_info freed here */
    mp->pool_data = NULL;
    // ...
}
```

After `rte_free(bp_info)`, `bp_info` is a dangling pointer. The code then uses `bp_info->bpid` to index into `rte_dpaa_bpid_info` and `s_dpaa_bpid_allocated_flag`, which is use-after-free.

**Suggested fix:** Copy `bp_info->bpid` to a local variable before freeing:
```c
uint16_t bpid = bp_info->bpid;
rte_free(bp_info);
s_dpaa_bpid_allocated_flag[bpid].used = false;
```

---

### Patch 13/25: net/dpaa: support Rx/Tx taildrop

**Integer conversion may truncate values**

```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;
}
```

`td_val` is declared as `long`. Casting it to `unsigned int` and then checking if it's greater than `UINT16_MAX` is wrong because the cast to `unsigned int` may already truncate if `td_val` is larger than `UINT32_MAX` on a 64-bit system. The check should happen on `td_val` before the cast.

**Suggested fix:**
```c
if (td_val > UINT16_MAX || td_val < 0)
    td_threshold = CGR_RX_PERFQ_THRESH;
else
    td_threshold = (unsigned int)td_val;
```

Same issue for `td_tx_threshold`.

---

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

**Error propagation lost**

```c
ret = qman_find_fq_by_cgrid(dpaa_intf->cgr_rx[loop].cgrid, &fqid);
if (!ret) {
    /* Should be FQ not cleaned in previous program. */
    DPAA_PMD_DEBUG("FQ(fqid=0x%x) with rx cgid=%d is still alive?",
        fqid, dpaa_intf->cgr_rx[loop].cgrid);
    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 `qman_shutdown_fq_by_fqid()` fails, `ret` is set to the error code, but the next line unconditionally overwrites `ret` with the return value of `qman_delete_cgr()`. The shutdown error is lost.

**Suggested fix:** Accumulate errors or log the first failure and continue:
```c
int err = qman_shutdown_fq_by_fqid(fqid);
if (err) {
    DPAA_PMD_WARN("Failed(%d) to shutdown fq(fqid=0x%x)", err, fqid);
    if (!ret) ret = err;  /* preserve first error */
}
```

Same pattern applies to the Tx CGR loop.

---

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

**Missing validation of bh_port_name**

```c
ret = dpaa_ol_get_bh_port_name(dev->device->devargs,
                   (char *)fq_info.bh_port_name,
                   MAX_BH_PORT_NAME_LEN);
if (ret <= 0) {
    DPAA_PMD_ERR("Offline port \"%s\" devarg not defined or invalid",
             DRIVER_OL_BH_PORT);
    return -1;
}
```

If `dpaa_ol_get_bh_port_name()` returns `< 0`, it's an error (e.g. from `rte_strscpy()` buffer overflow). The check `ret <= 0` treats both 0 (key absent) and negative (error) the same. A negative return should be handled as an error and propagated properly, not hardcoded to `-1`.

**Suggested fix:**
```c
if (ret < 0)
    return ret;  /* propagate rte_strscpy error */
if (ret == 0) {
    DPAA_PMD_ERR("Offline port \"%s\" devarg not defined", DRIVER_OL_BH_PORT);
    return -EINVAL;
}
```

---

### Patch 23/25: drivers: improve crypto fq resource

**Resource leak on init_error3 path**

```c
init_error3:
    for (j = 0; j < i; j++) {
        ret = qman_shutdown_fq(&internals->inq[j]);
        if (ret) {
            DPAA_PMD_ERR("%s: Failed(%d) to shutdown RXQ(0x%08x)",
                cryptodev->data->name, ret, internals->inq[j].fqid);
        }
    }
    qman_release_fqid_range(fqids[0], RTE_DPAA_MAX_RX_QUEUE);
```

The loop shuts down only the first `i` RXQs (those created before the error), but `qman_release_fqid_range()` releases all `RTE_DPAA_MAX_RX_QUEUE` FQIDs. The FQIDs for `i` to `RTE_DPAA_MAX_RX_QUEUE-1` were allocated but never created, so they should still be released, but the mismatch is confusing. The code is correct but could be clearer by documenting that the release covers all allocated FQIDs, not just those successfully created.

**Suggested fix:** Add a comment:
```c
/* Release all allocated FQIDs; uncreated ones were never initialized */
qman_release_fqid_range(fqids[0], RTE_DPAA_MAX_RX_QUEUE);
```

---

## Style / Process Issues (Warnings)

### Patch 02/25: bus/dpaa: refine fman naming

**Inconsistent log macros**

The code uses both `FMAN_ERR()` and `DPAA_BUS_ERR()` in the same file. Use consistent log macros within a module. The patch itself changes some `FMAN_ERR` to `DPAA_BUS_ERR`, which is good, but the file should be fully consistent.

**Suggested fix:** Convert remaining `FMAN_ERR` calls to `DPAA_BUS_ERR` in a follow-up cleanup patch.

---

### Patch 08/25: drivers: add BMI Tx statistics

**Hardcoded DPAA_BMI_XSTATS_COUNT**

```c
#define DPAA_BMI_XSTATS_COUNT 12
```

The comment says "Must equal RTE_DIM(dpaa_xstats_strings) - number_of_non_bmi_entries" but the array `dpaa_xstats_strings` gains 4 new BMI Tx entries, bringing the total to 12 BMI entries. The array has 8 Rx + 4 Tx = 12 BMI entries. The comment and define are correct, but if the array changes (e.g., a new stat is added) and the define is not updated, the code will break silently.

**Suggested fix:** Use a static_assert or runtime check to verify the count matches the array size.

---

### Patch 10/25: net/dpaa: optimize FMC MAC

**Magic number in port name parsing**

```c
idx = atoi(&found[idx_str_start]);
```

`atoi()` does not check for conversion errors. If the port name is malformed (e.g., `MAC/abc`), `atoi()` returns 0, which is treated as a valid index 0.

**Suggested fix:** Use `strtol()` with error checking:
```c
char *end;
long tmp = strtol(&found[idx_str_start], &end, 10);
if (*end != '\0' || tmp < 0 || tmp > INT_MAX) {
    DPAA_PMD_ERR("Invalid port index in name: %s", name);
    return -EINVAL;
}
idx = (int)tmp;
```

---

### Patch 17/25: net/dpaa: support non fmX-macY

**Missing bounds check on sh_if_name**

```c
char sh_if_name[IFNAMSIZ];
// ...
ret = dpaa_get_devargs_str(dpaa_dev->device.devargs,
               DRIVER_SH_IF_NAME, sh_if_name,
               sizeof(sh_if_name));
```

`dpaa_get_devargs_str()` uses `rte_strscpy()` which NUL-terminates and checks buffer size, so this is safe. However, the function returns `< 0` on truncation, and the code treats it as 0 (key absent). If the devarg value is too long, the truncated name is silently used.

**Suggested fix:**
```c
if (ret < 0) {
    DPAA_PMD_ERR("Shared interface name too long (max %zu)", sizeof(sh_if_name) - 1);
    return ret;
}
```

---

### Patch 22/25: drivers: add offline port

**Unchecked return from rte_kvargs_parse**

```c
kvlist = rte_kvargs_parse(devargs->bus_str, NULL);
if (kvlist == NULL)
    continue;
```

If parsing fails, `kvlist` is NULL and the code skips that devarg. This silently ignores malformed devargs. Should log a warning if parsing fails.

**Suggested fix:**
```c
if (kvlist == NULL) {
    DPAA_BUS_WARN("Failed to parse bus devargs for device %s", devargs->name);
    continue;
}
```

---

### Patch 24/25: common/dpaax: add destructor guard

**kfree macro is verbose**

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

The `pr_debug` message is printed every time `kfree()` is called during destructor context, which may be hundreds of times. This will spam the debug log.

**Suggested fix:** Remove the `pr_debug` or make it conditional on a verbose debug flag.

---

## Info (Consider)

### Patch 03/25: bus/dpaa: scan max BPID

**DTS parsing could fail silently**

If the `fsl,bpid-range` property is missing, `start + count` is 0 and the code warns but continues. The warning is good, but the code should consider whether continuing with default `bman_pool_max` is safe or if initialization should fail.

**Suggested consideration:** Document in a comment why the default is acceptable.

---

### Patch 12/25: dma/dpaa: add SG data validation

**Data validation is a debug feature**

The `s_data_validation` flag and associated code (`fsl_qdma_data_validation()`) are only enabled via