|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 support for Marvell CN20K DPI (Data Plane Interface) DMA hardware to the DPDK cnxk driver. The patches add new ROC (Resource Operating Code) layer APIs and extend the dmadev PMD to support CN20K PF/VF devices, multiple virtual channels, and inter-process domain DMA transfers.

---

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

### Errors

**Missing error checks and error path resource leaks:**

1. **`roc_dpi_lf_chan_tbl_alloc` resource leak on error** (line 577, roc_dpi.c):
   ```c
   rc = dpi_chan_tbl_alloc(lf->dev, lf->blk_addr, tbl_sz);
   if (rc < 0)
       return rc;
   
   lf->chan_tbl_sz = tbl_sz;
   lf->chan_tbl = rc;
   return 0;
   ```
   If `dpi_chan_tbl_alloc` returns a valid table number (non-negative) but the function later fails (currently it doesn't, but future changes could add failure paths), the allocated channel table is never freed. However, as written this is not a leak since there are no error paths after the assignment. **Not an error in current code** - omit this.

2. **`roc_dpi_rsrc_init` multiple resource leak on error** (line 646-670, roc_dpi.c):
   ```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;
   }
   
   for (slot = 0; slot < roc_dpi->nr_lfs; slot++)
       dpi_lf_init(&(roc_dpi->lfs[slot]), dev, slot);
   
   return rc;
   ```
   If `dpi_lf_attach` succeeds but a later operation in `roc_dpi_rsrc_init` were to fail (currently none do, but this is fragile), the attached LFs are never detached. However, the function as written has no failure path after `dpi_lf_attach`, so **this is not a current error** - omit.

3. **`cn20k_dmadev_parse_devargs` kvlist not freed on all error paths** (line 27, cnxk_dmadev.c):
   ```c
   if (devargs == NULL) {
       dpivf->max_vchans = num_vchans;
       dpivf->max_lfs = num_lfs;
       return 0;  /* OK - no kvlist allocated */
   }
   
   kvlist = rte_kvargs_parse(devargs->args, NULL);
   if (kvlist == NULL)
       goto exit;  /* OK - allocation failed, nothing to free */
   
   if (rte_kvargs_process(kvlist, CN20K_DPI_NUM_VCHANS, &parse_val_u16, &num_vchans) < 0)
       goto exit;
   if (rte_kvargs_process(kvlist, CN20K_DPI_NUM_LFS, &parse_val_u16, &num_lfs) < 0)
       goto exit;
   
   if (!num_vchans || !num_lfs || num_vchans > CN20K_DPI_MAX_VCHANS ||
       num_lfs > CN20K_DPI_MAX_LFS)
       goto exit;
   
   if (!rte_is_power_of_2(num_vchans) || !rte_is_power_of_2(num_lfs)) {
       plt_err("num_vchans or num_lfs is not a power of 2");
       goto exit;
   }
   
   dpivf->max_vchans = num_vchans;
   dpivf->max_lfs = num_lfs;
   
   rte_kvargs_free(kvlist);
   return 0;
   exit:
   rte_kvargs_free(kvlist);
   return -EINVAL;
   ```
   All error paths correctly free `kvlist` via the `exit` label. **No error.**

4. **`roc_dpi_rsrc_fini` - queue memzone leaks if not freed by caller** (line 683-697, roc_dpi.c):
   The function calls `plt_memzone_free(que->mz)` inside the loop, but only if `que->mz` is non-NULL. The `que->mz` is set to NULL after freeing. This is correct. **No error.**

5. **`roc_dpi_dev_init` and `roc_dpi_dev_fini` - missing `dev_fini` call on error in `dev_init`** (line 726-745, roc_dpi.c):
   ```c
   if (roc_model_is_cn20k()) {
       rc = dev_init(dev, pci_dev);
       if (rc)
           plt_err("Failed to init dpi roc device");
   } else {
       vfid = ((pci_dev->addr.devid & 0x1F) << 3) | (pci_dev->addr.function & 0x7);
       vfid -= 1;
       roc_dpi->vfid = vfid;
       idev_dma_cs_offset_set(offset);
   }
   
   return rc;
   ```
   If `dev_init` fails, the error is logged and returned, but no cleanup is performed. However, `dev_init` itself should clean up on failure. Looking at the definition of `dev_init` (in roc_dev.c, not shown in this patch), it is part of the common device init infrastructure and is expected to clean up on error. **Not a leak in this patch**, but the caller should verify `dev_init` contract.

**No errors found.** The code correctly handles resource cleanup on error paths where applicable.

---

### Warnings

1. **Inconsistent use of `rte_zmalloc` vs `malloc` for descriptors** (line 553, roc_dpi_priv.h and line 670, roc_dpi.c):
   ```c
   struct roc_dpi_lf_que {
       const struct plt_memzone *mz;
       uint64_t *cmd_base;
       ...
   } __plt_cache_aligned;
   ```
   The queue command buffers are allocated via `plt_memzone_reserve_aligned` (which uses hugepage memory), not `rte_zmalloc_socket`. Per guidelines, queue-related buffers (descriptor rings, Rx/Tx queue control structures) should use `rte_zmalloc_socket` for zero-initialization and NUMA-local allocation. However, the code uses `plt_memzone_reserve_aligned`, which is also hugepage-backed and NUMA-aware. The guidelines state "Queue-related buffers... should use `rte_zmalloc_socket`", but `plt_memzone_reserve_aligned` is acceptable for large, aligned allocations. **No issue** - memzone is appropriate here.

2. **Missing release notes update in patch 1** (line 1-10, series.patch):
   Patch 1 adds new API functions (`roc_dpi_rsrc_init`, `roc_dpi_rsrc_fini`, `roc_dpi_access_pair_group_*`, etc.) that are internal symbols exported via `RTE_EXPORT_INTERNAL_SYMBOL`. Internal API changes do not require release notes per the guidelines. **No warning.**

3. **Potential integer overflow in memzone size calculation** (line 638, roc_dpi.c):
   ```c
   mz = plt_memzone_reserve_cache_align(plt_pci_dev_name(name, ROC_DPI_DEV_NAME, pci_dev),
                                        roc_dpi->nr_lfs * sizeof(struct roc_dpi_lf));
   ```
   If `roc_dpi->nr_lfs` is large, `roc_dpi->nr_lfs * sizeof(struct roc_dpi_lf)` could overflow. However, `nr_lfs` is bounded by `CN20K_DPI_MAX_LFS` (256) and `sizeof(struct roc_dpi_lf)` is likely a few hundred bytes at most. The maximum allocation is `256 * ~500 = ~128KB`, well below any overflow risk. **No warning.**

4. **Unbounded descriptor chain traversal in ring init** (line 619-625, roc_dpi.c):
   ```c
   for (slot = 0; slot < roc_dpi->nr_lfs; slot++)
       dpi_lf_init(&(roc_dpi->lfs[slot]), dev, slot);
   ```
   This is a simple loop over LFs, not a descriptor chain traversal. The loop is bounded by `roc_dpi->nr_lfs`, which is validated during devargs parsing. **No issue.**

**No warnings.**

---

### Info

1. **devargs parsing could use `rte_kvargs_parse` with a key list** (line 38, cnxk_dmadev.c):
   Currently the code passes `NULL` as the `valid_keys` argument to `rte_kvargs_parse`, then manually checks for known keys. Using a static key list would allow the parser to reject unknown keys:
   ```c
   static const char * const valid_keys[] = {
       CN20K_DPI_NUM_VCHANS,
       CN20K_DPI_NUM_LFS,
       NULL,
   };
   kvlist = rte_kvargs_parse(devargs->args, valid_keys);
   ```
   This is a minor improvement for better error messages. **Info suggestion.**

2. **Magic numbers in shift operations** (line 565, roc_dpi.c):
   ```c
   lf->rbase = dev->bar2 + (RVU_BLOCK_ADDR_DPI0 << 20 | slot << 12);
   ```
   The constants `20` and `12` are hardware-defined register offsets. Consider defining them:
   ```c
   #define DPI_LF_BLKADDR_SHIFT 20
   #define DPI_LF_SLOT_SHIFT    12
   lf->rbase = dev->bar2 + (RVU_BLOCK_ADDR_DPI0 << DPI_LF_BLKADDR_SHIFT |
                            slot << DPI_LF_SLOT_SHIFT);
   ```
   **Info suggestion.**

---

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

### Errors

**Missing error checks and error path resource leaks:**

1. **`cnxk_dmadev_configure` - ring_conf not freed on error after `roc_dpi_rsrc_init`** (line 238-251, cnxk_dmadev.c):
   ```c
   rc = roc_dpi_rsrc_init(&dpivf->rdpi);
   if (rc < 0) {
       plt_err("rsrc alloc failed");
       goto error;
   }
   
   lf = &(dpivf->rdpi.lfs[0]);
   rc = roc_dpi_lf_chan_tbl_alloc(lf, dpivf->num_vchans);
   if (rc < 0) {
       plt_err("Failed to allocate chan tbl");
       goto error;
   }
   
   /* ... */
   error:
   if (roc_model_is_cn20k() && rc < 0)
       cnxk_dmadev_vchan_rsrc_free(dpivf);
   
   return rc;
   ```
   If `roc_dpi_rsrc_init` succeeds but `roc_dpi_lf_chan_tbl_alloc` fails, the error path calls `cnxk_dmadev_vchan_rsrc_free`, which frees `ring_conf` and calls `roc_dpi_rsrc_fini`. Looking at `roc_dpi_rsrc_fini` (patch 1, line 683):
   ```c
   int roc_dpi_rsrc_fini(struct roc_dpi *roc_dpi) {
       /* ... frees queue memzones ... */
       rc = dpi_lf_detach(dev);
       plt_memzone_free(roc_dpi->mz);
       roc_dpi->mz = NULL;
       roc_dpi->lfs = NULL;
       return rc;
   }
   ```
   And `cnxk_dmadev_vchan_rsrc_free` (line 181):
   ```c
   static int cnxk_dmadev_vchan_rsrc_free(struct cnxk_dpi_vf_s *dpivf) {
       if (dpivf->ring_conf) {
           rte_free(dpivf->ring_conf);
           dpivf->ring_conf = NULL;
       }
       if (rdpi->lfs == NULL)
           return 0;
       rc = roc_dpi_lf_chan_tbl_free(&(rdpi->lfs[0]));
       rc = roc_dpi_rsrc_fini(rdpi);
       dpivf->is_ring_conf_done = false;
       return rc;
   }
   ```
   The error path correctly calls `cnxk_dmadev_vchan_rsrc_free`, which frees all allocated resources. **No error.**

2. **`cn20k_dmadev_queue_setup` - no cleanup on error after `roc_dpi_lf_ring_init`** (line 573, cnxk_dmadev.c):
   ```c
   rc = roc_dpi_lf_ring_init(que, &cfg);
   if (rc)
       return rc;
   ```
   If `roc_dpi_lf_ring_init` fails, the function returns without cleaning up previously initialized rings. However, looking at the structure, `roc_dpi_lf_ring_init` allocates the queue memzone, and on error the caller (`cnxk_dmadev_start`) should handle cleanup. Tracing the call chain: `cnxk_dmadev_start` -> `cn20k_dmadev_queue_setup` -> `roc_dpi_lf_ring_init`. If `cn20k_dmadev_queue_setup` fails, `cnxk_dmadev_start` returns the error (line 733):
   ```c
   rc = cn20k_dmadev_queue_setup(dpivf);
   if (rc)
       goto error;
   ```
   But the `error` label at line 753 only calls `roc_dpi_enable`, it doesn't clean up the partially initialized rings. Let's check `cnxk_dmadev_stop` (line 759):
   ```c
   static int cnxk_dmadev_stop(struct rte_dma_dev *dev) {
       /* ... */
       roc_dpi_disable(&roc_dpi->rdpi);
       if (!roc_model_is_cn20k()) {
           rte_mempool_free(dpivf->chunk_pool);
           /* ... */
       }
       return 0;
   }
   ```
   And `cnxk_dmadev_close` (line 782):
   ```c
   static int cnxk_dmadev_close(struct rte_dma_dev *dev) {
       /* ... */
       cnxk_dmadev_vchan_free(dpivf, RTE_DMA_ALL_VCHAN);
       if (roc_model_is_cn20k())
           cnxk_dmadev_vchan_rsrc_free(dpivf);
       /* ... */
   }
   ```
   So `cnxk_dmadev_vchan_rsrc_free` is only