|WARNING| [v0,1/1] net/octeon_ep: add CN20K SDP VF endpoint support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-30

# DPDK Patch Review - series.patch

## Summary

This patch introduces CN20K SDP VF endpoint support to the octeon_ep driver, adding mailbox, ring setup, and RX/TX handling. The review identifies several **critical correctness bugs** that must be fixed, along with style and documentation issues.

---

## ERRORS (Must Fix)

### 1. Resource Leak on Error Path in `otx_ep_cn20k_mbox_bbuf_init`

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

**Location:** Line 348-360

**Issue:** If `rte_zmalloc` fails after `mbox_info` has been allocated in `otx_ep_cn20k_setup_mbox`, the error path frees `mbox_info` but does NOT set `otx_ep->mbox_info` back to NULL. This leaves a dangling pointer that `otx_ep_cn20k_delete_mbox` will later attempt to use.

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

free_mbox:
    rte_free(mbox_info);
    otx_ep->mbox_info = NULL;  // MISSING before this line
    return ret;
```

**Fix:** Add the NULL assignment on the error path:

```c
free_mbox:
    rte_free(mbox_info);
    otx_ep->mbox_info = NULL;  // Add this line
    return ret;
```

### 2. Use-After-Free in Mailbox Interrupt Handler

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

**Location:** Lines 495-505

**Issue:** `otx_ep_cn20k_mbox_intr_handler` accesses `otx_ep->hw_addr` without verifying that `otx_ep` or `otx_ep->hw_addr` are still valid. If the device is being closed concurrently, this leads to use-after-free.

**Code:**
```c
static void
otx_ep_cn20k_mbox_intr_handler(void *param)
{
    struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)param;
    struct otx_ep_device *otx_ep = (struct otx_ep_device *)eth_dev->data->dev_private;
    uint64_t intr_status;

    /* No validation that otx_ep or hw_addr is still valid */
    intr_status = oct_ep_read64(otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_RINT);
```

**Fix:** Add NULL checks at the start of the handler:

```c
static void
otx_ep_cn20k_mbox_intr_handler(void *param)
{
    struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)param;
    struct otx_ep_device *otx_ep;
    uint64_t intr_status;

    if (eth_dev == NULL || eth_dev->data == NULL || eth_dev->data->dev_private == NULL)
        return;

    otx_ep = (struct otx_ep_device *)eth_dev->data->dev_private;

    if (otx_ep->hw_addr == NULL)
        return;

    intr_status = oct_ep_read64(otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_RINT);
    ...
}
```

### 3. Missing Error Check on `rte_intr_enable`

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

**Location:** Lines 519-524

**Issue:** The check `if (!(rc == -1 || rc == 0))` is inverted. `rte_intr_enable` returns 0 on success, negative on error. The condition should be `if (rc != 0)` or `if (rc < 0)`.

**Code:**
```c
rc = rte_intr_enable(pdev->intr_handle);

if (!(rc == -1 || rc == 0)) {
    otx_ep_err("rte_intr_enable failed");
    return -1;
}
```

**Fix:**
```c
rc = rte_intr_enable(pdev->intr_handle);
if (rc != 0) {
    otx_ep_err("rte_intr_enable failed: %d", rc);
    return rc;
}
```

### 4. Unchecked Return Value from `rte_intr_callback_register`

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

**Location:** Lines 515-517

**Issue:** `rte_intr_callback_register` can fail, but its return value is not checked. If registration fails, the subsequent `rte_intr_enable` will enable interrupts without a handler registered, leading to lost interrupts or crashes.

**Code:**
```c
rte_intr_callback_register(pdev->intr_handle, otx_ep_cn20k_mbox_intr_handler,
                           (void *)eth_dev);

rc = rte_intr_enable(pdev->intr_handle);
```

**Fix:**
```c
rc = rte_intr_callback_register(pdev->intr_handle, otx_ep_cn20k_mbox_intr_handler,
                                (void *)eth_dev);
if (rc != 0) {
    otx_ep_err("Failed to register interrupt callback: %d", rc);
    return rc;
}

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);
    return rc;
}
```

### 5. Missing `rte_smp_wmb()` Before ISM Update

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

**Location:** Line 172 (IQ setup) and Line 249 (OQ setup)

**Issue:** The ISM counter (`*iq->inst_cnt_ism = 0`) is written without a preceding memory barrier. The hardware or another core may observe the ISM write before the ISM address register write completes, leading to a race.

**Code:**
```c
rte_write64(ism_addr, (uint8_t *)otx_ep->hw_addr +
            CN20K_EP_R_IN_CNTS_ISM(iq_no));
iq->inst_cnt_ism =
    (uint32_t __rte_atomic *)((uint8_t *)otx_ep->ism_buffer_mz->addr
                 + CNXK_EP_IQ_ISM_OFFSET(iq_no));
otx_ep_dbg("SDP_R[%d] INST Q ISM virt: %p, dma: 0x%" PRIX64, iq_no,
           (void *)(uintptr_t)iq->inst_cnt_ism, ism_addr);
*iq->inst_cnt_ism = 0;  // BUG: no barrier before this write
```

**Fix:**
```c
rte_write64(ism_addr, (uint8_t *)otx_ep->hw_addr +
            CN20K_EP_R_IN_CNTS_ISM(iq_no));
iq->inst_cnt_ism =
    (uint32_t __rte_atomic *)((uint8_t *)otx_ep->ism_buffer_mz->addr
                 + CNXK_EP_IQ_ISM_OFFSET(iq_no));
otx_ep_dbg("SDP_R[%d] INST Q ISM virt: %p, dma: 0x%" PRIX64, iq_no,
           (void *)(uintptr_t)iq->inst_cnt_ism, ism_addr);
rte_smp_wmb();  // Ensure ISM address write is visible before zeroing
*iq->inst_cnt_ism = 0;
```

**Apply the same fix in OQ setup (line 249):**
```c
rte_write64(ism_addr, (uint8_t *)otx_ep->hw_addr +
            CN20K_EP_R_OUT_CNTS_ISM(oq_no));
droq->pkts_sent_ism =
    (uint32_t __rte_atomic *)((uint8_t *)otx_ep->ism_buffer_mz->addr
                 + CNXK_EP_OQ_ISM_OFFSET(oq_no));
otx_ep_dbg("SDP_R[%d] OQ ISM virt: %p dma: 0x%" PRIX64, oq_no,
           (void *)(uintptr_t)droq->pkts_sent_ism, ism_addr);
rte_smp_wmb();
*droq->pkts_sent_ism = 0;
```

### 6. Missing Error Handling in `cn20k_ep_process_pkts_scalar_mseg`

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

**Location:** Lines 31-46

**Issue:** The loop contains `if (unlikely(!mbuf)) break;` but does not handle cleanup of the partially assembled packet chain. If `mbuf` is NULL mid-chain, `first_buf` is left with `first_buf->next` pointing to freed or invalid memory. The packet is still added to `rx_pkts[]`, causing use-after-free or corruption.

**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))
        break;  // BUG: first_buf chain is incomplete, but rx_pkts[pkts] = first_buf still happens
    ...
}
last_buf->next = NULL;

droq->read_idx = otx_ep_incr_index(droq->read_idx, 1, nb_desc);
droq->refill_count += num_bufs;
...
rx_pkts[pkts] = mbuf;  // This assigns first_buf, which may be incomplete
```

**Fix:**
```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)) {
        /* Free the partial chain */
        rte_pktmbuf_free(first_buf);
        goto skip_packet;
    }
    ...
}
last_buf->next = NULL;
droq->read_idx = otx_ep_incr_index(droq->read_idx, 1, nb_desc);
droq->refill_count += num_bufs;
mbuf = first_buf;
rx_pkts[pkts] = mbuf;
bytes_rsvd += mbuf->pkt_len;
continue;

skip_packet:
    /* Adjust counters for the skipped packet */
    droq->pkts_pending--;
    droq->stats.rx_err++;
    /* Do NOT add to rx_pkts */
```

---

## WARNINGS (Should Fix)

### 1. Missing Release Notes

This patch adds CN20K hardware support, which is a significant feature addition. The release notes must be updated to document:
- New hardware support (CN20K SDP VF endpoint)
- New device IDs (`PCI_DEVID_CN20KA_EP_NET_VF`, `PCI_DEVID_CNF20KA_EP_NET_VF`)
- Any user-visible behavior changes (mailbox architecture, max_rings devarg)

**Action:** Add a section to `doc/guides/rel_notes/release_XX_YY.rst` (where XX_YY is the target release).

### 2. Missing Feature Documentation

The new `max_rings` devarg is added but not documented.

**File:** `drivers/net/octeon_ep/otx_ep_ethdev.c`, Line 977

**Code:**
```c
RTE_PMD_REGISTER_PARAM_STRING(net_otx_ep,
                              OTX_ISM_ENABLE "=<0|1> "
                              OTX_MAX_RINGS "=<1-8>");
```

**Action:** Document the `max_rings` parameter in the driver documentation (likely `doc/guides/nics/octeon_ep.rst` or similar). Explain:
- What it controls (number of rings to allocate)
- Valid range (1-8)
- Default value (if any)

### 3. Excessive Logging in Fast Path

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

**Location:** Lines 82, 127 (mbox_write/mbox_read)

**Issue:** `otx_ep_dbg` calls in mailbox write/read functions are in a hot path (every mailbox operation). Debug logs should not be in performance-critical paths unless guarded by a compile-time flag.

**Action:** Remove or move these logs outside the critical section, or guard with `#ifdef DEBUG`.

### 4. Use of `rte_smp_wmb()` Instead of Atomic Operations

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

**Location:** Lines 135, 206

**Issue:** The code uses `rte_smp_wmb()` around mailbox register writes. While this is acceptable for MMIO ordering, the pattern of "write header, wmb, write payload, wmb, write control" suggests the code is trying to enforce ordering that the hardware may not respect unless the writes themselves are ordered. Consider if explicit ordering on the `oct_ep_write64` calls (using `rte_write64` which is already ordered on most architectures) is sufficient, making the standalone `wmb` unnecessary.

**Recommendation:** Verify hardware requirements. If the hardware requires the sequence of writes to be visible in a specific order, the current pattern is correct. If not, the standalone barriers may be redundant.

### 5. Non-const Function Pointer Array

**File:** `drivers/net/octeon_ep/otx_ep_common.h` (implied by `otx_ep->fn_list`)

**Issue:** The `fn_list` structure (assumed to be a function pointer table) is populated at runtime in `cn20k_ep_vf_setup_device`, but is not declared `const`. If the contents are fixed after initialization, it should be `const` to prevent accidental overwrites.

**Action:** If `fn_list` is never modified after initialization, declare it `const struct otx_ep_fn_list` in the device structure.

---

## INFO (Consider)

### 1. Redundant `rte_smp_wmb()` in Mailbox Read

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

**Location:** Line 135

**Code:**
```c
rte_smp_wmb();
if (i == 0) {
    /* Dummy write, loads Offset = 0 data into CSR. Value doesn't matter,
     * it gets ignored by hardware.
     */
    oct_ep_write64(0, otx_ep->hw_addr + CN20K_SDP_RMT_VFX_MBOX_RD_CMD_DATA);
    rte_smp_wmb();  // Redundant? MMIO writes are already ordered
```

**