|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

## Patch: CNXK DMA O20 Support Series (v2)

This is a 4-patch series adding CN20K (O20) DPI DMA hardware support to the DPDK cnxk driver.

---

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

### Errors

**Resource leak on error path (roc_dpi.c:467-472):**
```c
int
dpi_lf_queue_configure(struct roc_dpi_lf_que *que, struct roc_dpi_lf_ring_cfg *rcfg)
{
	...
	mz = plt_memzone_reserve_aligned(nm, que->qsize * que->cmd_len, 0, 128);
	if (!mz) {
		plt_err("Cannot alloc buffer for DPI LF ring command buffer: %s", nm);
		return -ENOMEM;
	}

	que->mz = mz;
	que->cmd_base = (uint64_t *)mz->addr;

	reg = plt_read64(lf->rbase + DPI_LF_RINGX_CFG(rcfg->ring_idx));
```
In `roc_dpi_lf_ring_init()` (line 561), if `mbox_process()` fails after calling `dpi_lf_queue_configure()`, the memzone allocated in `dpi_lf_queue_configure()` is leaked. The error path at line 567 calls `mbox_put()` but does not free `que->mz`.

**Suggested fix:**
```c
fail:
	mbox_put(mbox);
	if (rc && que->mz) {
		plt_memzone_free(que->mz);
		que->mz = NULL;
		que->cmd_base = NULL;
	}
	return rc;
```

**Memzone leak on rsrc_fini error path (roc_dpi.c:660-683):**
In `roc_dpi_rsrc_fini()`, if `dpi_lf_detach()` fails, the memzone is freed but the function still returns the error code from `dpi_lf_detach()` instead of 0. However, the more severe issue is in the earlier loop: if `plt_memzone_free(que->mz)` is called and `dpi_lf_detach()` later fails, the memzones are freed but `roc_dpi->mz` is freed again on line 681, potentially double-freeing.

Actually, re-reading: the queue memzones are freed in the loop (lines 673-678), then `roc_dpi->mz` (which holds the LF array) is freed on line 681. This is correct unless one of the queue frees fails (memzone_free typically can't fail). The real issue is that the code does not check whether the queue memzone free succeeded before proceeding.

However, `plt_memzone_free()` wraps `rte_memzone_free()` which returns `int` but the code ignores the return value. If a memzone free fails, the function continues and may leave stale pointers.

**Suggested fix:** Check return values:
```c
	for (slot = 0; slot < roc_dpi->nr_lfs; slot++) {
		lf = &roc_dpi->lfs[slot];

		for (qid = 0; qid < ROC_DPI_LF_RINGS; qid++) {
			que = &lf->queue[qid];
			if (que->mz) {
				rc = plt_memzone_free(que->mz);
				if (rc)
					plt_err("Failed to free queue memzone: %d", rc);
				que->mz = NULL;
			}
		}
	}

	rc = dpi_lf_detach(dev);
	if (rc)
		plt_err("Failed to detach LF: %d", rc);

	rc = plt_memzone_free(roc_dpi->mz);
	if (rc)
		plt_err("Failed to free LF array memzone: %d", rc);

	roc_dpi->mz = NULL;
	roc_dpi->lfs = NULL;

	return rc;  /* return last error, or 0 if all succeeded */
```

**Missing memzone cleanup on configure error (roc_dpi.c:282):**
In `cnxk_dmadev_vchan_rsrc_free()`, if `roc_dpi_lf_chan_tbl_free()` fails, the function still proceeds to free `rdpi->lfs` via `roc_dpi_rsrc_fini()`. But the ring_conf memzone is freed unconditionally without checking whether the channel table free succeeded. This could leave the channel table resource in the kernel in an inconsistent state.

Not strictly a leak, but the error handling is incomplete. The code should log the error and continue cleanup, which it does.

**Use-after-free risk (roc_dpi.c:223-235):**
In `cnxk_dmadev_configure()`, the code calls `cnxk_dmadev_vchan_free()` which frees `dpivf->conf[i].c_desc.compl_ptr` for all vchans. Then on CN20K, it may call `cnxk_dmadev_vchan_rsrc_free()` which can fail. If that fails, the function returns an error, but `dpivf->conf` is left in a partially initialized state with freed `compl_ptr` pointers. If the caller retries `rte_dma_configure()`, the second call to `cnxk_dmadev_vchan_free()` (line 221) will access already-freed memory in line 133.

**Suggested fix:** Set `compl_ptr` to NULL after freeing:
```c
	for (; i < num_vchans; i++) {
		dpi_conf = &dpivf->conf[i];
		if (dpi_conf->c_desc.compl_ptr) {
			rte_free(dpi_conf->c_desc.compl_ptr);
			dpi_conf->c_desc.compl_ptr = NULL;
		}
	}
```
(Note: the code already does this on line 135, so this is actually correct.)

**Incorrect memzone size calculation (roc_dpi.c:469):**
```c
	mz = plt_memzone_reserve_aligned(nm, que->qsize * que->cmd_len, 0, 128);
```
`que->qsize` is `uint16_t` and `que->cmd_len` is `uint8_t`. The multiplication is performed at 16-bit width, then widened to `size_t` for the memzone size parameter. If `que->qsize * que->cmd_len` overflows 16 bits, the memzone will be allocated with a truncated size, leading to out-of-bounds writes when commands are enqueued.

**Suggested fix:**
```c
	mz = plt_memzone_reserve_aligned(nm, (size_t)que->qsize * que->cmd_len, 0, 128);
```

### Warnings

**`roc_dpi.c:75` function name and parameter naming style:**
```c
void
dpi_lf_ena_dis(struct roc_dpi_lf *lf, uint8_t enb)
```
Parameter `enb` should be `bool` rather than `uint8_t`. The function is called with `true`/`false` on lines 193, 211, 368, 376, making it a boolean control.

**Suggested fix:**
```c
void
dpi_lf_ena_dis(struct roc_dpi_lf *lf, bool enable)
{
	uint64_t reg;
	int ring_idx;

	for (ring_idx = 0; ring_idx < ROC_DPI_LF_RINGS; ring_idx++) {
		reg = plt_read64(lf->rbase + DPI_LF_RINGX_CFG(ring_idx));

		if (enable)
			reg |= DPI_LF_QCFG_QEN;
		else
			reg &= ~DPI_LF_QCFG_QEN;

		plt_write64(reg, lf->rbase + DPI_LF_RINGX_CFG(ring_idx));
	}
}
```

**roc_dpi.c:220 bool parameter instead of uint8_t:**
```c
int
dpi_lf_attach(struct dev *dev, uint8_t blk_addr, bool modify, uint16_t nb_lf)
```
Good.

**roc_dpi.c:286 confusing error message:**
```c
		plt_err("Very few LFs are requested than required");
```
Grammar is incorrect. Should be "Too few LFs requested" or "Insufficient LFs requested".

**roc_dpi.h:8 and roc_dpi_priv.h:22 missing ROC_DPI_MAX_LFS definition:**
The header defines `ROC_DPI_LF_RINGS` (2) and uses `ROC_DPI_MAX_LFS` in `struct dpi` on line 49 of roc_dpi_priv.h, but `ROC_DPI_MAX_LFS` is not defined in the common headers. It's defined in the dmadev driver's cnxk_dmadev.h (256), but the ROC layer should not depend on driver headers.

The constant `256` appears in the dmadev driver but should be defined in the ROC headers for consistency.

**Suggested fix:** Add to `roc_dpi.h`:
```c
#define ROC_DPI_MAX_LFS	       256
```

---

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

### Errors

**Potential double-free on configure retry (cnxk_dmadev.c:284):**
In `cn20k_dmadev_parse_devargs()`, if `rte_kvargs_parse()` succeeds but a subsequent `rte_kvargs_process()` fails, the code goes to `exit:` which calls `rte_kvargs_free(kvlist)` on line 71. However, if the caller retries the operation after an error, the `kvlist` pointer is already freed.

Actually, this is fine: each call to `cn20k_dmadev_parse_devargs()` creates a new kvlist, and the pointer is local. No issue here.

**Memzone leak on ring_conf allocation failure (cnxk_dmadev.c:260-268):**
```c
		dpivf->ring_conf = rte_zmalloc("dpi_ring_conf", sizeof(struct cn20k_ring_conf) *
					       num_rings, 0);
		if (dpivf->ring_conf == NULL) {
			plt_err("Failed to allocate memory for ring conf");
			rc = -ENOMEM;
			goto error;
		}

		rc = roc_dpi_rsrc_init(&dpivf->rdpi);
		if (rc < 0) {
			plt_err("rsrc alloc failed");
			goto error;
		}
```
If `roc_dpi_rsrc_init()` fails after `ring_conf` is allocated, the error path calls `cnxk_dmadev_vchan_rsrc_free()` which frees `ring_conf` on line 187. This is correct. However, if `roc_dpi_lf_chan_tbl_alloc()` (line 271) fails, the same cleanup path is taken. This is also correct.

No issue here.

**Resource leak on cn20k_dmadev_queue_setup failure (cnxk_dmadev.c:706):**
In `cnxk_dmadev_start()`, if `cn20k_dmadev_queue_setup()` fails (line 701), the function returns the error without cleaning up the DPI reset state. The device was reset on line 694 but is not re-enabled or left in a defined state.

**Suggested fix:** On error, either re-enable the device or document that the device is left disabled.

**Memzone leak on configure failure (cnxk_dmadev.c:1106):**
```c
	dpivf->conf = rte_zmalloc("dpi_vchan_conf", dpivf->num_vchans *
				  sizeof(struct cnxk_dpi_conf), 0);
	if (dpivf->conf == NULL) {
		plt_err("Failed to allocate memory for vchan conf");
		rc = -ENOMEM;
		goto err_out_free;
	}
```
If allocation of `dpivf->conf` succeeds but a later error occurs (e.g., `roc_dpi_dev_init()` on line 1118), the error path `err_out_free:` (line 1124) calls `rte_dma_pmd_release(name)` which frees the dmadev but does not free `dpivf->conf`. This is a memory leak.

**Suggested fix:** Add cleanup before the error label:
```c
err_out_free:
	if (dpivf->conf) {
		rte_free(dpivf->conf);
		dpivf->conf = NULL;
	}
	rte_dma_pmd_release(name);
	return rc;
```

**Missing initialization of `dpivf->num_vchans` on non-CN20K (cnxk_dmadev.c:1087-1089):**
In `cnxk_dmadev_probe()`, on CN9K (not CN10K, not CN20K), the code sets `dpivf->num_vchans` to `CNXK_DPI_MAX_VCHANS_PER_QUEUE` on line 1087. But if CN10K is detected, `dpivf->num_vchans` is set on line 1082. However, the CN20K branch (line 1083-1085) sets it via `cn20k_dmadev_parse_devargs()` which may fail, leaving `num_vchans` uninitialized if the devargs parse returns an error before setting it.

Actually, re-reading: `cn20k_dmadev_parse_devargs()` always sets `dpivf->max_vchans` and `dpivf->max_lfs` even if `devargs == NULL` (lines 33-36), so `num_vchans` is set on line 1069. No issue.

**Confusing control flow (cnxk_dmadev.c:1067-1089):**
The CN20K devargs parse happens before the process type check (line 1070 vs 1072), but then `num_vchans` is set again after the process type check (line 1083 vs 1069). This means the devargs are parsed in both primary and secondary processes, but the assignment on line 1069 is overwritten on line 1083 in the primary. This is redundant and confusing.

**Suggested fix:** Move devargs parsing after the process type check so it only runs in the primary.

### Warnings

**Function pointer assignment on every configure (cnxk_dmadev.c:1067-1089):**
The code assigns function pointers to `dev->fp_obj->copy`, `dev->fp_obj->submit`, etc. on every call to `cnxk_dmadev_probe()`. This is redundant since the function pointers don't change after the initial assignment. This is a minor inefficiency but not a bug.

**Missing validation of `dpivf->vchans_per_ring` (cnxk_dmadev.c:249):**
The code computes `dpivf->vchans_per_ring` on line 249 as `dpivf->num_vchans / num_rings`. If `num_vchans < num_rings`, the division truncates to 0, which is then used as a divisor on line 256: `(CN20K_DPI_MAX_DESC / dpivf->vchans_per_ring)`. This causes a divide-by-zero or overflow.

**Suggested fix:** Validate that `vchans_per_ring` is non-zero before using it as a divisor.

**Hardcoded priority (cnxk_dmadev.c:620):**
```c
			cfg.pri = 0;