|WARNING| [v2,4/4] doc: update 26.11 release notes for cnxk dmadev

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-20

# DPDK Patch Review

## Summary

This patch series adds O20 (CN20K) DPI DMA hardware support to the cnxk driver.
The series consists of 4 patches:
1. Common/cnxk: ROC support for CN20K DPI
2. Dma/cnxk: DMA driver support for CN20K
3. Dma/cnxk: Inter-process domain DMA support
4. Doc: Release notes update

---

## Patch 1/4: common/cnxk: add O20 DPI DMA support

### Errors

**Resource leak on error path in `roc_dpi_rsrc_init()`**

In `roc_dpi_rsrc_init()` (roc_dpi.c:680):
- `plt_memzone_reserve_cache_align()` allocates `mz`
- `dpi_lf_attach()` may fail after `mz` is stored in `roc_dpi->mz`
- On failure, the function frees `mz` and sets pointers to NULL
- **However**, if `dpi_lf_attach()` succeeds but `dpi_lf_init()` loop fails partway through, the function returns the error without cleanup
- The memzone and any partially initialized LFs are leaked

```c
rc = dpi_lf_attach(dev, blk_addr, true, roc_dpi->nr_lfs);
if (rc) {
    plt_err("Could not attach LFs");
    plt_memzone_free(mz);
    roc_dpi->mz = NULL;
    roc_dpi->lfs = NULL;
    return rc;  /* Good: cleaned up */
}

for (slot = 0; slot < roc_dpi->nr_lfs; slot++)
    dpi_lf_init(&(roc_dpi->lfs[slot]), dev, slot);

return rc;  /* BAD: if dpi_lf_init failed (returns non-zero), mz is leaked */
```

**Fix**: Check `rc` after the `dpi_lf_init()` loop and clean up on error:
```c
for (slot = 0; slot < roc_dpi->nr_lfs; slot++) {
    rc = dpi_lf_init(&(roc_dpi->lfs[slot]), dev, slot);
    if (rc) {
        dpi_lf_detach(dev);
        plt_memzone_free(mz);
        roc_dpi->mz = NULL;
        roc_dpi->lfs = NULL;
        return rc;
    }
}
```

---

**Missing error check on `roc_dpi_lf_chan_tbl_alloc()` in `cn20k_dmadev_queue_setup()`**

In `cn20k_dmadev_queue_setup()` (cnxk_dmadev.c:568-576 in patch 2/4, but originates from ROC code added in patch 1):
```c
rc = roc_dpi_lf_ring_init(que, &cfg);
if (rc)
    return rc;  /* checked */

/* ... assignment to dpi_conf ... */
```

Later in the calling function `cnxk_dmadev_start()`:
```c
rc = cn20k_dmadev_queue_setup(dpivf);
if (rc)
    goto error;

rc = cn20k_dmadev_chan_tbl_setup(dpivf);
/* chan_tbl_setup depends on chan_tbl_alloc having succeeded */
```

The `roc_dpi_lf_chan_tbl_alloc()` call in the configure path stores the channel table in `lf->chan_tbl`.
If it fails, `lf->chan_tbl` is uninitialized, but the code proceeds to use it in `cn20k_dmadev_chan_tbl_setup()`.

**Fix**: `roc_dpi_lf_chan_tbl_alloc()` returns negative on error; verify and propagate it:
```c
rc = roc_dpi_lf_chan_tbl_alloc(lf, dpivf->num_vchans);
if (rc < 0) {
    plt_err("Failed to allocate chan tbl");
    return rc;  /* instead of storing rc and ignoring */
}
```

---

**Incorrect mempool size calculation potentially exceeds hardware limit**

In `roc_dpi.c`, `roc_dpi_configure_v2()`:
```c
#define DPI_POOL_SZ(d) ((1ULL << (d)) / 8)  /* d is block_size */

/* ... */
queue_buf_sz = CNXK_DPI_QUEUE_BUF_SIZE_V2;  /* 130944 from patch 2 */
block_size = rte_log2_u32(queue_buf_sz);
if (!rte_is_power_of_2(queue_buf_sz))
    block_size += 1;
nb_chunks = CNXK_DPI_CHUNKS_FROM_DESC(queue_buf_sz, nb_desc);
/* ... */
pool_sz = DPI_POOL_SZ(block_size);  /* 2^block_size / 8 */
```

The maximum pool size supported by the hardware is `128 * 1024` (per comments in patch 2).
With `CNXK_DPI_QUEUE_BUF_SIZE_V2 = 130944`:
- `block_size = rte_log2_u32(130944) + 1 = 17 + 1 = 18`
- `pool_sz = (1ULL << 18) / 8 = 262144 / 8 = 32768` pool entries
- Each entry is `queue_buf_sz` bytes, so total pool memory = `32768 * 130944 = 4,292,870,144` bytes (~4GB)

**This exceeds the 128KB device limit** and will fail when the mempool is created.

Patch 2 changes `CNXK_DPI_QUEUE_BUF_SIZE_V2` from `130944` to `130048` and adds a comment explaining the 127KB limit.
However, the calculation here uses the old larger value, and the mempool allocation does not verify the computed size against the hardware maximum before calling `rte_mempool_create()`.

**Fix**:
1. Verify that `queue_buf_sz` does not exceed `DPI_MAX_POOL_SIZE` (127KB).
2. Fail early with a clear error if the requested buffer size would exceed the hardware limit.
3. Ensure the mempool size respects the per-object and total pool size constraints.

---

**Incorrect symbol export name in `roc_platform_base_symbols.c`**

```c
RTE_EXPORT_INTERNAL_SYMBOL(roc_dpi_access_pair_group_handler_get)
```

The function is named `roc_dpi_access_pair_group_handler_get()`, but the English word is "handle" not "handler".
The function returns a "handle" (an identifier), not a "handler" (a callback or service function).

**Fix**: Rename to:
```c
int roc_dpi_access_pair_group_handle_get(...);
RTE_EXPORT_INTERNAL_SYMBOL(roc_dpi_access_pair_group_handle_get)
```

And update the function name in `roc_dpi.h` and `roc_dpi.c` to match.

---

### Warnings

**Hardcoded memzone name length in `dpi_lf_queue_configure()`**

```c
char nm[ROC_DPI_DEV_NAME_LEN] = {'\0'};
/* ... */
snprintf(nm, sizeof(nm), "%s_%u_%u_%x", "dpi_lf_q", lf->slot, rcfg->ring_idx,
         lf->dev->pf_func);
```

`ROC_DPI_DEV_NAME_LEN` is defined as `sizeof(ROC_DPI_DEV_NAME) + PCI_PRI_STR_SIZE`.
The format string includes a prefix, two `%u` (up to 5 digits each), one `%x` (up to 4 hex digits), and underscores.
If `slot`, `ring_idx`, or `pf_func` are large, the string could exceed the buffer.

**Suggestion**: Use a larger buffer or verify that `ROC_DPI_DEV_NAME_LEN` accounts for the maximum possible string length.

---

**Inconsistent devargs parsing error handling**

In `cn20k_dmadev_parse_devargs()` (patch 2):
```c
kvlist = rte_kvargs_parse(devargs->args, NULL);
if (kvlist == NULL)
    goto exit;
/* ... */
exit:
    rte_kvargs_free(kvlist);
    return -EINVAL;
```

If `rte_kvargs_parse()` returns NULL, the code jumps to `exit` which calls `rte_kvargs_free(NULL)`.
While `rte_kvargs_free()` tolerates NULL, the pattern is inconsistent with typical DPDK error paths
where a NULL allocation is handled immediately without cleanup.

**Suggestion**: Return directly on NULL `kvlist`:
```c
if (kvlist == NULL)
    return -EINVAL;
```

---

**Shadowed variable name `rc` in nested scope**

In `cnxk_dmadev_configure()` (patch 2), the function declares `int rc = 0;` at the top,
then in a CN20K-specific block:
```c
if (roc_model_is_cn20k()) {
    rc = cnxk_dmadev_vchan_rsrc_free(dpivf);
    if (rc < 0)
        goto error;
    /* ... more uses of rc ... */
}
```

The `rc` variable is reused for multiple calls, which is acceptable but can obscure errors
if an early failure is overwritten by a later success.

**Suggestion**: Initialize `rc` to a sentinel value (e.g., `-1`) and verify that all paths set it explicitly.

---

## Patch 2/4: dma/cnxk: add O20 DPI DMA support

### Errors

**Use-after-free potential in `cnxk_dmadev_vchan_rsrc_free()`**

```c
if (rdpi->lfs == NULL)
    return 0;

rc = roc_dpi_lf_chan_tbl_free(&(rdpi->lfs[0]));
/* ... */
rc = roc_dpi_rsrc_fini(rdpi);
/* roc_dpi_rsrc_fini frees rdpi->mz, which contains rdpi->lfs */

dpivf->is_ring_conf_done = false;
return rc;
```

After `roc_dpi_rsrc_fini()` frees `rdpi->mz`, the pointer `rdpi->lfs` becomes stale but is not set to NULL.
A subsequent call to `cnxk_dmadev_vchan_rsrc_free()` will pass the stale pointer check `if (rdpi->lfs == NULL)`
and attempt to dereference `rdpi->lfs[0]`, causing a use-after-free.

**Fix**: Set `rdpi->lfs = NULL` after `roc_dpi_rsrc_fini()`:
```c
rc = roc_dpi_rsrc_fini(rdpi);
if (rc < 0)
    plt_err("Failed to free dpi lfs");
/* roc_dpi_rsrc_fini already sets rdpi->mz and rdpi->lfs to NULL on success,
   but ensure it's done even on failure */
rdpi->lfs = NULL;
```

Verify that `roc_dpi_rsrc_fini()` in patch 1 sets both pointers to NULL on all exit paths.

---

**Double-free of `dpivf->ring_conf` in error path**

In `cnxk_dmadev_configure()`:
```c
if (roc_model_is_cn20k()) {
    rc = cnxk_dmadev_vchan_rsrc_free(dpivf);
    if (rc < 0)
        goto error;
    /* ... */
    dpivf->ring_conf = rte_zmalloc(...);
    if (dpivf->ring_conf == NULL) {
        rc = -ENOMEM;
        goto error;
    }
    /* ... more allocations that can fail ... */
}
error:
    if (roc_model_is_cn20k() && rc < 0)
        cnxk_dmadev_vchan_rsrc_free(dpivf);
```

If the `rte_zmalloc()` for `ring_conf` succeeds, but a later allocation fails (e.g., `roc_dpi_rsrc_init()`),
the `goto error` calls `cnxk_dmadev_vchan_rsrc_free()` again, which frees `ring_conf` a second time:
```c
if (dpivf->ring_conf) {
    rte_free(dpivf->ring_conf);
    dpivf->ring_conf = NULL;
}
```

This is a double-free because `ring_conf` was already freed in the first call to `cnxk_dmadev_vchan_rsrc_free()`.

**Fix**: Set `dpivf->ring_conf = NULL` immediately after freeing:
```c
if (dpivf->ring_conf) {
    rte_free(dpivf->ring_conf);
    dpivf->ring_conf = NULL;
}
```

Alternatively, check whether the allocation that failed was *after* `ring_conf` was set,
and only call cleanup if needed.

---

**Missing `rc` initialization in `cnxk_dmadev_start()`**

```c
if (roc_model_is_cn20k()) {
    for (i = 0; i < (dpivf->rdpi.nr_lfs << 1); i++)
        dpivf->ring_conf[i].pending = 0;
    rc = roc_dpi_reset(&dpivf->rdpi);
    if (rc < 0) {
        plt_err("Failed to reset DPI LFs");
        goto error;
    }
    /* ... */
    goto enable_dpi;
}
/* Non-CN20K path */
queue_buf_sz = CNXK_DPI_QUEUE_BUF_SIZE_V2;
/* ... */
enable_dpi:
    roc_dpi_enable(&dpivf->rdpi);
error:
    return rc;
```

If the CN20K path is taken and succeeds, `rc` is set to `0` by `roc_dpi_reset()`.
However, if the non-CN20K path is taken, `rc` is only set if an error occurs.
If the non-CN20K setup succeeds, `rc` is undefined at the `enable_dpi` label,
and the function returns garbage.

**Fix**: Initialize `rc = 0;` at the top of `cnxk_dmadev_start()`.

---

**Variable assigned then overwritten before read in `cn20k_dmadev_parse_devargs()`**

```c
uint16_t num_lfs = num_vchans >> 1;
/* ... */
if (rte_kvargs_process(kvlist, CN20K_DPI_NUM_LFS, &parse_val_u16, &num_lfs) < 0)
    goto exit;
```

The initial assignment `num_lfs = num_vchans >> 1` is overwritten by `rte_kvargs_process()` if the key is present.
If the key is absent, `parse_val_u16()` is not called and `num_lfs` retains its default.

This is correct behavior (default value if key absent, parsed value if present),
but the initial assignment is **always overwritten** if the key exists, making it a dead store.

**Not an error**, just an observation. The code is correct as-is.

---

### Warnings

**Potential memzone name collision with concurrent calls**

In `dpi_lf_queue_configure()`:
```c
snprintf(nm, sizeof