|WARNING| [RFC,4/4] net/mcdma: add experimental QCSR dump APIs

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-11-09

# DPDK MCDMA PMD Review

This review covers the Altera MCDMA poll mode driver patch series (4 patches).
The review is organized by severity and follows AGENTS.md guidelines.

---

## ERRORS

### 1. Missing error check on function that can fail

**File:** `drivers/net/altera/mcdma/mcdma_eth_ops.c`  
**Line:** 304 (patch 1)

```c
ret = mcdma_rx_seg_len_for_pool(dev, mb_pool, &rxq->rx_seg_len);
if (ret != 0) {
    mcdma_dev_descq_free(rxq);
    rte_free(rxq);
    return ret;
}
```

After this error path, if `mcdma_dev_descq_alloc()` fails, the code frees `rxq->sw_ring` via `mcdma_dev_descq_free()`. However, `mcdma_sw_descq_alloc()` allocated `rxq->sw_ring` but on error only frees the queue structure itself without nulling the `sw_ring` pointer. The subsequent call to `mcdma_dev_descq_free()` attempts to free `rxq->sw_ring` again.

**File:** `drivers/net/altera/mcdma/mcdma_eth_ops.c`  
**Lines:** 223-233 (patch 1)

```c
q->sw_ring = rte_zmalloc_socket("desc_sw_ctx",
                q->num_descs * sizeof(struct rte_mbuf *), 0,
                socket_id);
if (!q->sw_ring) {
    PMD_DRV_LOG(ERR, "Failed to allocate sw ring");
    rte_free(q);
    return -ENOMEM;
}
```

The allocation succeeds but no cleanup path nulls `q->sw_ring` before the queue is freed.

**Suggested fix:**
In `mcdma_sw_descq_alloc()`, on sw_ring allocation failure:
```c
if (!q->sw_ring) {
    PMD_DRV_LOG(ERR, "Failed to allocate sw ring");
    rte_free(q);
    *q_ptr = NULL;  /* Prevent double-free */
    return -ENOMEM;
}
```

Or set `q->sw_ring = NULL` after `rte_free(q->sw_ring)` in `mcdma_dev_descq_free()`.

---

### 2. `volatile` used instead of atomics for inter-thread shared variables

**File:** `drivers/net/altera/mcdma/mcdma_eth.h`  
**Lines:** 133, 138, 140 (patch 1)

```c
/* SW shadow of last freed HW didx (1 .. num_descs), 0 = none freed */
volatile uint16_t freed_cons_didx;
/* HW writeback: last completed didx (1 .. num_descs), 0 = none */
volatile uint16_t cons_didx;
```

These variables are shared between the datapath (burst functions) and potentially other threads reading queue state. The `volatile` keyword does not provide atomicity or memory ordering guarantees required for inter-thread synchronization.

**Suggested fix:**
Use `rte_atomic_load_explicit(&q->cons_didx, rte_memory_order_acquire)` when reading from the hardware writeback or other threads, and `rte_atomic_store_explicit(&q->cons_didx, value, rte_memory_order_release)` when writing. Declare the fields as `RTE_ATOMIC(uint16_t)`.

---

### 3. Integer multiply without widening cast

**File:** `drivers/net/altera/mcdma/mcdma_eth_ops.c`  
**Line:** 237 (patch 1)

```c
size_t len = MCDMA_DESC_PAGE_SIZE * q->num_desc_pages;
```

`MCDMA_DESC_PAGE_SIZE` is 4096 (uint32_t), `q->num_desc_pages` is uint32_t. The multiplication is performed at 32 bits; if `num_desc_pages` is large enough, the product overflows before assignment to `size_t` (64-bit).

**Suggested fix:**
```c
size_t len = (size_t)MCDMA_DESC_PAGE_SIZE * q->num_desc_pages;
```

---

### 4. Use-after-free on multi-segment Tx mbuf

**File:** `drivers/net/altera/mcdma/mcdma_eth_rxtx.c`  
**Lines:** 215-219 (patch 2)

```c
if (dir == MCDMA_DIR_H2D) {
    q->sw_ring[prod] = d->eof ? mb : NULL;
} else {
    q->sw_ring[prod] = seg;
}
```

For Tx (H2D), only the descriptor with `d->eof == 1` stores the head mbuf `mb` in `sw_ring[prod]`. Earlier descriptors in the chain store `NULL`. However, the mbuf segments are still referenced by hardware. If completion frees `sw_ring[prod]` for a non-EOF descriptor, the corresponding segment mbuf is not freed but its `next` pointer may still be dereferenced during completion.

The comment on line 218 says "One sw_ring owner per packet: head mbuf on the EOF descriptor so completion frees the whole chain once." This is correct only if completion skips freeing NULL entries.

**Check completion path:**
```c
for (i = 0; i < nb_to_free; i++) {
    idx = MCDMA_NEXT_IDX(mcdma_queue_freed_ring(q), q->num_descs);
    d = MCDMA_DESC_PTR(q, idx);
    if (!d->link && d->eof && q->sw_ring[idx] != NULL)
        rte_pktmbuf_free(q->sw_ring[idx]);

    q->sw_ring[idx] = NULL;
    q->freed_cons_didx = d->didx;
    if (!d->link)
        q->complete++;
}
```

This code only frees if `d->eof` is set, so it appears safe. However, the non-EOF descriptors have `sw_ring[idx] == NULL` and their mbuf segments are not accounted for. If the chain is freed via `rte_pktmbuf_free()` on the head, the segments are released by mbuf's own chaining logic, so there is no actual leak here.

**Correction:** The code is safe. The head mbuf's `next` pointer chain is freed when `rte_pktmbuf_free(mb)` is called on EOF. Non-EOF sw_ring slots are NULL and no use-after-free occurs. This is not a bug.

**Action:** Omit this item.

---

### 5. Missing bounds check on descriptor chain traversal

**File:** `drivers/net/altera/mcdma/mcdma_eth_rxtx.c`  
**Lines:** 181-186 (patch 2)

```c
for (cnt = 0; cnt < mb->nb_segs; cnt++) {
    prod = prod_nxt;
    d = MCDMA_DESC_PTR(q, prod);
    if (d->link) { /* skip the link desc */
        q->didx = MCDMA_DIDX_NEXT(q->didx, q->num_descs);
        d->didx = q->didx;
```

The loop iterates `mb->nb_segs` times, which is supplied by the application and could be incorrect. However, `mb->nb_segs` is a standard mbuf field that DPDK ensures is consistent with the actual segment chain length during allocation. The loop walks the mbuf chain via `seg->next` on line 232:

```c
seg = seg->next;
```

If `nb_segs` were larger than the actual chain, `seg->next` would become NULL and dereferencing it would crash. If `nb_segs` were smaller, some segments would not be processed.

The check on line 176 rejects multi-segment mbufs for Rx, so only Tx is affected. For Tx, the application is trusted to provide well-formed mbufs. If the mbuf is malformed (nb_segs incorrect), that is an application bug, not a PMD bug.

**Correction:** The PMD trusts the mbuf metadata, which is standard DPDK practice. This is not a PMD bug.

**Action:** Omit this item.

---

### 6. Missing `RTE_EXPORT_*` macro on new public functions

**File:** `drivers/net/altera/mcdma/rte_pmd_mcdma.c`  
**Lines:** 56, 83 (patch 3)

The functions `rte_pmd_mcdma_pio_read64` and `rte_pmd_mcdma_pio_write64` are declared `__rte_experimental` in the header and are exported:

```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mcdma_pio_read64, 26.11)
int
rte_pmd_mcdma_pio_read64(uint16_t port_id, uint64_t offset, uint64_t *val)
```

This is correct. The version `26.11` should match the DPDK release introducing the symbol. Assuming this patch targets v26.11 or later, the annotation is valid.

**Correction:** No issue here.

**Action:** Omit this item.

---

### 7. Experimental API in installed header without version in export macro

**File:** `drivers/net/altera/mcdma/rte_pmd_mcdma.c`  
**Patch 4, Lines 205, 240**

```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mcdma_qcsr_read, 26.11)
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mcdma_qcsr_dump, 26.11)
```

The version parameter `26.11` is included. This is correct.

**Correction:** No issue.

**Action:** Omit this item.

---

## WARNINGS

### 1. Reading `rxmode.mtu` after configure instead of `dev->data->mtu`

**File:** `drivers/net/altera/mcdma/mcdma_eth_ops.c`  
**Lines:** 21-28 (patch 1)

```c
static uint16_t
mcdma_dev_mtu_get(const struct rte_eth_dev *dev)
{
    if (dev->data->dev_conf.rxmode.mtu != 0)
        return dev->data->dev_conf.rxmode.mtu;
    if (dev->data->mtu != 0)
        return dev->data->mtu;

    return RTE_ETHER_MTU;
}
```

This helper is called from `mcdma_dev_configure()` and `mcdma_dev_queue_init()`. In `mcdma_dev_configure()`, reading `rxmode.mtu` is correct (that is where the user's request is consumed). However, `mcdma_dev_queue_init()` is called during queue start, which happens after configure completes. At that point, the canonical MTU is `dev->data->mtu`, not `rxmode.mtu`. If the application calls `rte_eth_dev_set_mtu()` after configure, `rxmode.mtu` becomes stale but `dev->data->mtu` is updated.

**Suggested fix:**
In `mcdma_dev_queue_init()`, use `dev->data->mtu` directly. Or refactor `mcdma_dev_mtu_get()` to only be used during configure and always read `dev->data->mtu` elsewhere.

---

### 2. Missing scatter Rx capability advertisement and enablement

**File:** `drivers/net/altera/mcdma/mcdma_eth_ops.c`  
**Lines:** 731-735 (patch 1)

```c
if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_SCATTER) {
    PMD_DRV_LOG(ERR, "Rx scattered/multi-segment mbufs not supported");
    return -EINVAL;
}

dev->data->scattered_rx = 0;
```

The PMD explicitly rejects `RTE_ETH_RX_OFFLOAD_SCATTER`. However, the MTU can be set such that frames exceed the mbuf data room. The PMD should either:
1. Auto-enable scatter when the frame size exceeds the mbuf capacity, or
2. Advertise `RTE_ETH_RX_OFFLOAD_SCATTER` in `rx_offload_capa` and require the application to enable it explicitly.

Currently, if the MTU is larger than the mbuf data room and scatter is not enabled, the PMD will truncate or drop oversized packets silently.

**Suggested fix:**
Advertise `RTE_ETH_RX_OFFLOAD_SCATTER` in `dev_info->rx_offload_capa` and implement scatter Rx, or document that MTU is capped by the mbuf pool's data room size.

---

### 3. MTU set does not re-select Rx burst function

**File:** `drivers/net/altera/mcdma/mcdma_eth_ops.c`  
**Lines:** 758-789 (patch 1)

```c
static int mcdma_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
{
    /* ... validation ... */
    for (qid = 0; qid < dev->data->nb_rx_queues; qid++) {
        /* ... check mbuf pool ... */
        rxq->rx_seg_len = frame_len;
    }

    dev->data->dev_conf.rxmode.mtu = mtu;
    dev->data->mtu = mtu;

    return 0;
}
```

The function updates `rxq->rx_seg_len` but does not call any function to re-select the Rx burst handler. If the PMD had separate scalar and scattered burst functions (it does not currently), this would be required. Since the PMD does not support scatter Rx at all, this is not a functional bug, but it is a design limitation.

**Suggested fix:**
Document that scatter Rx is not supported and MTU changes require the queue to be stopped and restarted.

---

### 4. Hardcoded Ethernet overhead instead of per-device calculation

**File:** `drivers/net/altera/mcdma/mcdma_eth.h`  
**Line:** 43 (patch 1)

```c
#define MCDMA_RX_ETH_OVERHEAD   (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN)
```

The overhead is hardcoded to 18 bytes (14-byte header + 4-byte CRC). This does not account for VLAN tags (4 bytes) or QinQ (8 bytes). If the device supports tagged frames, the overhead should be derived from device capabilities.

**Suggested fix:**
Add a function to compute overhead based on device configuration, similar to how other PMDs handle VLAN overhead:
```c
static uint32_t mcdma_eth_overhead(const struct rte_eth_dev *dev) {
    uint32_t overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
    if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_VLAN)
        overhead += RTE_VLAN_HLEN;
    if (dev->data->dev_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_QINQ)
        overhead += RTE_VLAN_HLEN;
    return overhead;
}
```

---

### 5. Missing `RTE_EXPORT_*` macro placement

**File:** `drivers/net/altera/mcdma/rte_pmd_mcdma.c`  
**Patches 3 and 4**

The `RTE_EXPORT_EXPERIMENTAL_SYMBOL` macros are placedwarning: review has no Review-Result line, severity inferred from prose (response may be truncated)