|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

# Review of DPDK Patch Series: CN20K SDP VF Endpoint Support

## ERRORS

### 1. `cn20k_ep_mbox.c`: Mailbox Write Error Path Resource Leak

**Location**: `drivers/net/octeon_ep/cn20k_ep_mbox.c:97-108`

```c
if (ctl & CN20K_MBOX_WR_CMD_ERR) {
    otx_ep_err("Mbox write error at offset %u", offset + (uint16_t)(i * 8));
    oct_ep_write64(CN20K_MBOX_WR_CMD_ERR,
                   otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_WR_CMD_CTL);
    return -EIO;
}

if (ctl & CN20K_MBOX_WR_DROP) {
    otx_ep_err("Mbox write dropped at offset %u", offset + (uint16_t)(i * 8));
    oct_ep_write64(CN20K_MBOX_WR_DROP,
                   otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_WR_CMD_CTL);
    return -EIO;
}
```

**Error**: Both error paths exit the loop without completing all `num_words` writes. If an error occurs on iteration `i`, the function returns immediately, leaving the mailbox in a partial-write state. The caller has no way to know how many words were successfully written, and subsequent operations may read stale or incomplete data from the mailbox.

The loop attempts `num_words` writes but can fail partway through. The error paths clear the hardware error flags but do not reset the write offset or invalidate the partial data. This can cause the next mailbox operation to read or process incomplete data.

**Suggested Fix**:
Track the number of successfully written words and either:
1. Return an error code that includes the partial byte count, allowing the caller to handle cleanup, or
2. Invalidate the partial write by resetting the mailbox state (e.g., by writing a zero header or incrementing a sequence number).

At minimum, the function should document that an early return leaves the mailbox in an inconsistent state, and the caller must reinitialize before the next operation.

---

### 2. `cn20k_ep_mbox.c`: Mailbox Read Error Path Partial Data Return

**Location**: `drivers/net/octeon_ep/cn20k_ep_mbox.c:169-177`

```c
if (ctl & CN20K_MBOX_RD_CMD_ERR) {
    otx_ep_err("Mbox read error at offset %u", offset + (uint16_t)(i * 8));
    oct_ep_write64(CN20K_MBOX_RD_CMD_ERR,
                   otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_RD_CMD_CTL);
    data[i] = 0;
    return -EIO;
}
```

**Error**: When a read error occurs at iteration `i`, the function zeros `data[i]` and returns `-EIO`. However, `data[0]` through `data[i-1]` have already been populated with values read from the mailbox. The caller receives `-EIO` but has no indication that `buf` contains partial data. If the caller checks the return value but does not zero the buffer on error, it may use the partially-read data, leading to undefined behavior.

This is distinct from the write-error case (Error #1) because here the partial data is already in the caller's buffer, not just in the hardware.

**Suggested Fix**:
On error, zero the entire `buf` before returning:
```c
if (ctl & CN20K_MBOX_RD_CMD_ERR) {
    otx_ep_err("Mbox read error at offset %u", offset + (uint16_t)(i * 8));
    oct_ep_write64(CN20K_MBOX_RD_CMD_ERR,
                   otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_RD_CMD_CTL);
    memset(buf, 0, len);
    return -EIO;
}
```

---

### 3. `cn20k_ep_mbox.c`: Bulk Read Buffer Overflow Check Insufficient

**Location**: `drivers/net/octeon_ep/cn20k_ep_mbox.c:529-535`

```c
if (otx_ep->mbox_data_index + read_cnt > MBOX_MAX_DATA_BUF_SIZE) {
    otx_ep_err("CN20K mbox bulk read buffer overflow");
    otx_ep->mbox_data_index = 0;
    memset(otx_ep->mbox_data_buf, 0, MBOX_MAX_DATA_BUF_SIZE);
    ret = -EINVAL;
    goto unlock;
}
```

**Error**: This check occurs *after* the loop has already read `read_cnt` bytes into `rsp.s_data.data[i]` (lines 536-540). If `otx_ep->mbox_data_index + read_cnt > MBOX_MAX_DATA_BUF_SIZE`, the subsequent `for` loop will write past the end of `otx_ep->mbox_data_buf`, causing a buffer overflow.

The check must happen *before* the data is copied into the buffer, not after it has been read from the mailbox but before it is stored locally.

**Suggested Fix**:
Move the overflow check above the mailbox read operation:
```c
if (otx_ep->mbox_data_index + read_cnt > MBOX_MAX_DATA_BUF_SIZE) {
    otx_ep_err("CN20K mbox bulk read buffer overflow");
    otx_ep->mbox_data_index = 0;
    memset(otx_ep->mbox_data_buf, 0, MBOX_MAX_DATA_BUF_SIZE);
    ret = -EINVAL;
    goto unlock;
}

ret = otx_ep_cn20k_mbox_send_cmd(otx_ep, cmd, &rsp);
if (ret) {
    otx_ep_err("CN20K mbox bulk read fragment failed");
    otx_ep->mbox_data_index = 0;
    memset(otx_ep->mbox_data_buf, 0, MBOX_MAX_DATA_BUF_SIZE);
    goto unlock;
}

for (i = 0; i < read_cnt; i++) {
    otx_ep->mbox_data_buf[otx_ep->mbox_data_index] = rsp.s_data.data[i];
    otx_ep->mbox_data_index++;
}
```

---

### 4. `cn20k_ep_mbox.c`: Bounce Buffer Leak on Setup Failure

**Location**: `drivers/net/octeon_ep/cn20k_ep_mbox.c:403-425`

```c
ret = otx_ep_cn20k_mbox_bbuf_init(mbox_info);
if (ret) {
    otx_ep_err("Failed to init bounce buffer");
    goto free_mbox;
}

mbox_info->mbox.hwbase = NULL;
mbox_info->mbox_up.hwbase = NULL;

otx_ep_dbg("CN20K VF mailbox initialized (CMD register based)");

return 0;

free_mbox:
    if (mbox_info->bbuf_base)
        rte_free(mbox_info->bbuf_base);
    rte_free(mbox_info);
    otx_ep->mbox_info = NULL;
    return ret;
```

**Error**: If `otx_ep_cn20k_mbox_bbuf_init()` succeeds but the function later returns via the `free_mbox` label (e.g., from an error path added in a future patch or from the `goto free_mbox` on line 399 or 405), the code frees `bbuf_base` correctly. However, if the function returns successfully (`return 0` at line 419) and a later operation in the call chain fails, nothing frees the bounce buffer. The only cleanup path is `otx_ep_cn20k_delete_mbox()`, which is called from `otx_ep_cn20k_mbox_uninit()`. If the caller does not call `uninit` on a failed `init`, the buffer leaks.

More precisely: if `otx_ep_cn20k_mbox_init()` calls `otx_ep_cn20k_setup_mbox()` and that succeeds, but `rte_intr_callback_register()` or `rte_intr_enable()` fails afterward (line 579-589 in `cn20k_ep_mbox.c`), the function returns an error but does not call `otx_ep_cn20k_delete_mbox()`. The bounce buffer remains allocated and unreachable.

**Suggested Fix**:
In `otx_ep_cn20k_mbox_init()`, if any operation after `otx_ep_cn20k_setup_mbox()` fails, call `otx_ep_cn20k_delete_mbox()` before returning:
```c
rc = otx_ep_cn20k_setup_mbox(otx_ep);
if (rc) {
    otx_ep_err("Failed to setup CN20K PF-VF mailbox");
    return rc;
}

rte_intr_callback_register(pdev->intr_handle, otx_ep_cn20k_mbox_intr_handler,
                           (void *)eth_dev);

rc = rte_intr_enable(pdev->intr_handle);
if (rc != 0) {
    otx_ep_err("rte_intr_enable failed: %d", rc);
    rte_intr_callback_unregister(pdev->intr_handle, otx_ep_cn20k_mbox_intr_handler,
                                 (void *)eth_dev);
    otx_ep_cn20k_delete_mbox(otx_ep);  /* ADD THIS */
    return rc;
}
```

---

### 5. `cn20k_ep_vf.c`: IQ Setup Returns Error but Device State Partially Initialized

**Location**: `drivers/net/octeon_ep/cn20k_ep_vf.c:95-107` and `154-156`

```c
if (!(reg_val & CNXK_EP_R_IN_CTL_IDLE)) {
    do {
        reg_val = oct_ep_read64(otx_ep->hw_addr + CN20K_EP_R_IN_CONTROL(iq_no));
        rte_delay_ms(1);
    } while ((!(reg_val & CNXK_EP_R_IN_CTL_IDLE)) && loop--);
}

if (loop < 0) {
    otx_ep_err("IDLE bit is not set");
    return -EIO;
}
```

And later:

```c
if (loop < 0) {
    otx_ep_err("INSTR DBELL is not zero");
    return -EIO;
}
```

**Error**: When `cn20k_ep_vf_setup_iq_regs()` returns `-EIO`, it has already written `iq->base_addr_dma`, `iq->nb_desc`, `iq->doorbell_reg`, `iq->inst_cnt_reg`, and `iq->inst_cnt_ism` (lines 122-135). The caller (`otx_ep_setup_iqs()` in `otx_ep_rxtx.c`) receives the error and may bail out, but the `iq` structure is left in a partially initialized state. If the application later calls `dev_close()` or tries to clean up, it may double-free or access invalid pointers, because the cleanup code assumes these fields are either fully initialized or zeroed.

This is a resource consistency issue: the function modifies the `iq` structure before checking all preconditions for success, then returns an error without undoing those modifications.

**Suggested Fix**:
Either:
1. Zero the `iq` structure on error before returning, or
2. Check all hardware preconditions (IDLE, DBELL zero) *before* writing any of the `iq` fields.

Option 2 is cleaner:
```c
/* First, verify IDLE and DBELL are clear */
if (!(reg_val & CNXK_EP_R_IN_CTL_IDLE)) {
    do {
        reg_val = oct_ep_read64(otx_ep->hw_addr + CN20K_EP_R_IN_CONTROL(iq_no));
        rte_delay_ms(1);
    } while ((!(reg_val & CNXK_EP_R_IN_CTL_IDLE)) && loop--);

    if (loop < 0) {
        otx_ep_err("IDLE bit is not set");
        return -EIO;
    }
}

loop = OTX_EP_BUSY_LOOP_COUNT;
while ((rte_read64(iq->doorbell_reg) != 0ull) && loop--) {
    rte_write32(OTX_EP_CLEAR_INSTR_DBELL, iq->doorbell_reg);
    rte_delay_ms(1);
}

if (loop < 0) {
    otx_ep_err("INSTR DBELL is not zero");
    return -EIO;
}

/* Now write the iq fields */
oct_ep_write64(iq->base_addr_dma, ...);
...
```

---

### 6. `cnxk_ep_rx.c`: Multi-Segment Rx Frees First Buffer on Error but Continues Loop

**Location**: `drivers/net/octeon_ep/cnxk_ep_rx.c:38-53`

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

**Error**: When a NULL mbuf is encountered at segment `i`, the code:
1. Frees `first_buf` (which may have already chained mbufs from segments `1` to `i-1` via `last_buf->next = mbuf`).
2. Sets `droq->read_idx` to `pkt_start_idx + num_bufs`.
3. Increments `droq->refill_count` by `num_bufs`.
4. Sets `first_buf = NULL` and breaks.

However, the loop has already advanced `droq->read_idx` by `i` descriptors (line 38). Then the code sets `droq->read_idx` again to `pkt_start_idx + num_bufs` (line 44). This means:
- Descriptors from `pkt_start_idx + 1` to `pkt_start_idx + i - 1` have been consumed (their mbufs are freed as part of `first_buf`), but the code resets `read_idx` back to `pkt_start_idx + num_bufs`, which is *after* those descriptors.
- The descriptors from `pkt_start_idx + 1` to `pkt_start_idx + i - 1` are now skipped and never processed. Their corresponding mbufs (if any) were freed, but the descriptor ring entries are not refilled.

This creates a descriptor ring leak: those skipped descriptors are neither processed nor refilled, and the ring will eventually run out of descriptors.

**Suggested Fix**:
Do notwarning: review has no Review-Result line, severity inferred from prose (response may be truncated)