|WARNING| [v2, 2/2] doc: update 26.11 release notes for octeon_ep driver

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: CN20K SDP VF Endpoint Support

## Summary

This patch series introduces support for CN20K SDP VF endpoint devices in the `octeon_ep` driver. It adds mailbox communication, ring setup, and Rx/Tx handling for the new hardware generation.

---

## ERRORS

### 1. Resource leak on error path in `cn20k_ep_mbox_setup`

**File:** `drivers/net/octeon_ep/cn20k_ep_mbox.c`

**Lines:** 406-433, particularly 425-433

**Issue:**

When `otx_ep_cn20k_mbox_setup()` succeeds on the first call (`&mbox_info->mbox`) but then fails on the second call (`&mbox_info->mbox_up`), the function jumps to `free_mbox`, which only frees `mbox_info` and `bbuf_base`. However, the first successful setup call may have allocated internal state that is never cleaned up.

The code jumps to `free_mbox` if `otx_ep_cn20k_mbox_setup(&mbox_info->mbox_up, ...)` fails, but does not undo the allocations or state from the successful `mbox_info->mbox` setup.

**Suggested fix:**

After the `mbox_up` setup failure, clear any state initialized by the first setup call before freeing:

```c
ret = otx_ep_cn20k_mbox_setup(&mbox_info->mbox, MBOX_DIR_HOSTVF_HOSTPF);
if (ret) {
	otx_ep_err("Failed to setup downward mailbox");
	goto free_mbox;
}

ret = otx_ep_cn20k_mbox_setup(&mbox_info->mbox_up, MBOX_DIR_HOSTVF_HOSTPF_UP);
if (ret) {
	otx_ep_err("Failed to setup upward mailbox");
	/* Need to undo mbox setup here */
	goto free_mbox;
}
```

If `mbox` setup allocates anything, those resources leak when `mbox_up` setup fails. Either document that `otx_ep_cn20k_mbox_setup()` has no internal allocations requiring cleanup, or add a separate cleanup step before the jump to `free_mbox`.

---

### 2. Resource leak on queue allocation failure in `otx_ep_dev_configure`

**File:** `drivers/net/octeon_ep/otx_ep_ethdev.c`

**Lines:** 427-438

**Issue:**

When `otx_ep_cn20k_mbox_alloc_sdp_rings()` fails after the flag `otx_epvf->configured` is set to `1`, the function returns error. If the application then calls configure again, the code at line 423 frees the rings with `otx_ep_cn20k_mbox_free_sdp_rings()` only if `configured == 1`, then sets it back to `0` at line 425.

However, if allocation fails with `rc < 0`, the `configured` flag is set to `0` at line 438 **before** returning. This means any partial resources allocated on a failed configure attempt are never freed, because the next configure call will see `configured == 0` and skip the free at line 423.

**Code:**

```c
if (otx_epvf->chip_gen == OTX_EP_CN20XX) {
	if (otx_epvf->configured) {
		otx_ep_cn20k_mbox_free_sdp_rings(otx_epvf, 0, true);
		otx_epvf->configured = 0;
	}

	rc = otx_ep_cn20k_mbox_alloc_sdp_rings(otx_epvf, eth_dev->data->nb_rx_queues);
	if (rc < 0 || rc != eth_dev->data->nb_rx_queues) {
		otx_ep_err("Failed to allocate the rings");
		otx_epvf->configured = 0;   // BUG: clearing flag prevents cleanup on retry
		return rc < 0 ? rc : -EIO;
	}
}
```

**Suggested fix:**

Either:
1. Leave `configured = 1` on partial allocation so the next configure attempt will free the partial state, or
2. Call `otx_ep_cn20k_mbox_free_sdp_rings()` here before clearing the flag:

```c
if (rc < 0 || rc != eth_dev->data->nb_rx_queues) {
	otx_ep_err("Failed to allocate the rings");
	if (rc > 0)  /* Partial allocation */
		otx_ep_cn20k_mbox_free_sdp_rings(otx_epvf, 0, true);
	otx_epvf->configured = 0;
	return rc < 0 ? rc : -EIO;
}
```

---

### 3. NULL pointer dereference risk in `cnxk_ep_process_pkts_scalar_mseg`

**File:** `drivers/net/octeon_ep/cnxk_ep_rx.c`

**Lines:** 26-55, particularly 38-54

**Issue:**

On the CN20K path, when iterating through multi-segment buffers, the code checks `if (unlikely(!mbuf))` at line 38, frees `first_buf`, increments the read index by `num_bufs`, and then breaks. However, after breaking from the loop, `first_buf` has been freed but the code at line 59 checks `if (!first_buf)` and continues. The assignment at line 66 (`rx_pkts[nb_rx++] = first_buf;`) then uses the freed pointer.

**Code:**

```c
for (i = 1; i < num_bufs; i++) {
	droq->read_idx = otx_ep_incr_index(droq->read_idx, 1, nb_desc);
	mbuf = recv_buf_list[droq->read_idx];
	if (unlikely(!mbuf)) {
		otx_ep_err("NULL mbuf in multi-segment Rx (seg %u/%u)", i, num_bufs);
		rte_pktmbuf_free(first_buf);
		droq->read_idx = otx_ep_incr_index(pkt_start_idx, num_bufs, nb_desc);
		droq->refill_count += num_bufs;
		first_buf = NULL;   // Set to NULL...
		break;
	}
	...
}
if (!first_buf)
	continue;           // ...check if NULL...

last_buf->next = NULL;
...
rx_pkts[nb_rx++] = first_buf;  // ...but at line 66, first_buf is used
```

Wait, re-reading: the code sets `first_buf = NULL` at line 48, then checks `if (!first_buf)` at line 59 and does `continue`, so it skips the assignment at line 66. This is actually correct.

**Correction:** This is **not** a bug. The NULL check at line 59 prevents the use-after-free. Discard this finding.

---

### 4. Missing export annotation for new public mailbox functions

**File:** `drivers/net/octeon_ep/cn20k_ep_mbox.c`

**Lines:** Functions at 492, 526, 606, 632, 660, 690, 722

**Issue:**

The functions `otx_ep_cn20k_mbox_send_cmd`, `otx_ep_cn20k_mbox_bulk_read`, `otx_ep_cn20k_mbox_init`, `otx_ep_cn20k_mbox_uninit`, `otx_ep_cn20k_mbox_send_ready`, `otx_ep_cn20k_mbox_alloc_sdp_rings`, and `otx_ep_cn20k_mbox_free_sdp_rings` are declared in the installed header `cn20k_ep_mbox.h` and are called from other files (`otx_ep_ethdev.c`, `otx_ep_mbox.c`). These are internal API functions used between DPDK components, not exposed to applications.

None of these functions have `RTE_EXPORT_INTERNAL_SYMBOL` annotations in the `.c` file. The build system generates linker version maps from these annotations, so their absence may cause linker issues or incorrect ABI.

**Suggested fix:**

Add `RTE_EXPORT_INTERNAL_SYMBOL` before each function definition:

```c
RTE_EXPORT_INTERNAL_SYMBOL(otx_ep_cn20k_mbox_send_cmd)
int
otx_ep_cn20k_mbox_send_cmd(struct otx_ep_device *otx_ep, union otx_ep_mbox_word cmd,
			   union otx_ep_mbox_word *rsp)
{
	...
}

RTE_EXPORT_INTERNAL_SYMBOL(otx_ep_cn20k_mbox_init)
int
otx_ep_cn20k_mbox_init(struct rte_eth_dev *eth_dev)
{
	...
}
```

Repeat for all seven functions listed above.

---

## WARNINGS

### 1. Missing release notes for internal API functions

**File:** `drivers/net/octeon_ep/cn20k_ep_mbox.h` and `.c`

**Issue:**

The release notes only document the addition of CN20K device support and the `max_rings` devarg. However, the patch also introduces seven new internal API functions (`otx_ep_cn20k_mbox_*`) that are part of the driver's internal structure. These do not require release notes per the guidelines.

**No action needed** -- internal API changes are correctly excluded from release notes.

---

### 2. `max_rings` devarg not validated in `otx_ethdev_parse_devargs`

**File:** `drivers/net/octeon_ep/otx_ep_ethdev.c`

**Lines:** 45-64

**Issue:**

The `max_rings` devarg is parsed at line 60, but the parsed value is not validated against the allowed range (1 to 8). If a user passes `max_rings=0` or `max_rings=100`, the driver accepts it and later code may fail or behave incorrectly.

**Suggested fix:**

Add validation after parsing:

```c
rte_kvargs_process(kvlist, OTX_MAX_RINGS, &parse_flag, &max_rings);
rte_kvargs_free(kvlist);

null_devargs:
	if (max_rings < 1 || max_rings > CN20K_MAX_RINGS_PER_VF) {
		otx_ep_err("max_rings must be 1 to %d", CN20K_MAX_RINGS_PER_VF);
		return -EINVAL;
	}
	otx_epvf->ism_ena = !!ism_enable;
	otx_epvf->sriov_info.rings_per_vf = max_rings;
```

Note: The default value `CN20K_MAX_RINGS_PER_VF` is only valid for CN20K; for other chips this may be incorrect. Consider checking `chip_gen` before applying it, or setting the default elsewhere.

---

### 3. Scatter Rx not selected when needed on CN20K

**File:** `drivers/net/octeon_ep/cnxk_ep_rx.c`

**Lines:** 7-71, particularly the CN20K path at 26-64

**Issue:**

The CN20K path in `cnxk_ep_process_pkts_scalar_mseg` handles multi-segment packets when they arrive, but there is no code shown that **selects the scattered Rx function** when the configured MTU requires it. The CN9X/CN10X path (lines 68-105) also lacks an explicit check against MTU, but the Rx function selection may be done elsewhere.

If the MTU is large enough to require scatter Rx but the driver does not enable it or select the correct Rx function, packets will be silently dropped or truncated.

**Suggested fix (in Rx queue setup or dev_configure):**

After configuring the MTU, check if the frame size exceeds the mbuf data buffer size and select the `_mseg` Rx function accordingly. The patch does not show where this decision is made, so verify that it exists for CN20K.

---

### 4. `RTE_EXPORT_SYMBOL` missing version number for experimental symbols

**File:** `drivers/net/octeon_ep/cn20k_ep_mbox.c`

**Lines:** Functions at 492, 526, 606, 632, 660, 690, 722

**Issue:**

If any of the functions in `cn20k_ep_mbox.c` were to become experimental public API (e.g., later exposed to applications), the `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macro requires a version parameter (e.g., `26.11`). However, these functions are internal-only, so `RTE_EXPORT_INTERNAL_SYMBOL` is correct and does not require a version.

**No action needed** -- correctly using `RTE_EXPORT_INTERNAL_SYMBOL` for internal API.

---

## INFO

### 1. Pre-existing: `otx_ep_mbox_bulk_read` return type inconsistency

**File:** `drivers/net/octeon_ep/otx_ep_mbox.c`

**Lines:** Not changed by this patch

**Issue:**

The existing function `otx_ep_mbox_bulk_read` takes `int32_t *size` as an output parameter, but the new `otx_ep_cn20k_mbox_bulk_read` takes `int32_t max_size` and `int32_t *size` (separate input and output). This is an inconsistency in the internal API design, but it is not introduced by this patch.

**Suggestion for future cleanup:** Align the two functions to use the same parameter convention (in/out separation).

---

### 2. Documentation notes CN20K "uses command/data/control register interface" but does not contrast with direct access

**File:** `doc/guides/nics/octeon_ep.rst`

**Lines:** 45-68

**Issue:**

The documentation states "CN20K uses a completely different mailbox architecture... command/data/control register interface" and "VF accesses via indirect CMD registers." It would be clearer to users if the documentation briefly mentioned what CN9X/CN10X use (direct MMIO to shared memory), so the contrast is explicit.

**Suggested addition:**

```rst
On CN9X and CN10X, the VF accesses the mailbox via direct MMIO reads/writes to
a shared memory buffer. CN20K instead uses an indirect command/data/control register
interface, where the VF writes an offset and data to CSRs and the hardware performs
the access.
```

---

## FINAL SUMMARY

- **2 Errors** (resource leak on error path, missing export annotations)
- **3 Warnings** (max_rings validation, scatter Rx selection check, devarg applicability)
- **2 Info** (pre-existing API inconsistency, documentation clarity)

The most critical issues are the resource leak on mailbox setup failure and the missing export annotations for internal functions. The `max_rings` validation is also important to prevent invalid configurations.

---

Review-Result: ERROR