|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

## Overview
This is a new PMD for Altera Multi-Channel DMA (MCDMA) devices. The driver provides a DPDK ethdev interface to DMA channels, exposing them as standard Ethernet ports. It includes device probing, queue management, packet burst handlers, and experimental APIs for PIO access and debug.

---

## ERRORS

### 1. MTU confused with frame length (multiple locations)

**mcdma_eth_ops.c:20-28, mcdma_dev_mtu_get()**
```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;
}
```
The first check reads `dev_conf.rxmode.mtu` which is the user's *request* and becomes stale after `rte_eth_dev_configure()` if `rte_eth_dev_set_mtu()` is called later. After configure completes, the canonical MTU is always `dev->data->mtu`.

**Fix:** Remove the first check and rely solely on `dev->data->mtu`:
```c
static uint16_t
mcdma_dev_mtu_get(const struct rte_eth_dev *dev)
{
	if (dev->data->mtu != 0)
		return dev->data->mtu;

	return RTE_ETHER_MTU;
}
```

**mcdma_eth_ops.c:30-34, mcdma_rx_frame_len_from_mtu()**
```c
static uint32_t
mcdma_rx_frame_len_from_mtu(uint16_t mtu)
{
	return (uint32_t)mtu + MCDMA_RX_ETH_OVERHEAD;
}
```
and **mcdma_eth.h:43-44**
```c
#define MCDMA_RX_ETH_OVERHEAD		(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN)
```
This hardcodes overhead as 18 bytes (14 header + 4 CRC) which is correct only for untagged frames. Devices that support VLAN or QinQ need 4 or 8 additional bytes. The hardcoded value will underestimate frame size when VLAN tags are present, causing MTU checks to pass when they should fail or scatter Rx to not be enabled when needed.

**Fix:** Calculate overhead dynamically based on device capabilities, similar to:
```c
static uint32_t
mcdma_rx_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_STRIP)
		overhead += RTE_VLAN_HLEN;  /* add for single VLAN */
	/* Adjust similarly for QinQ if supported */

	return overhead;
}
```
Then use `mcdma_rx_overhead(dev)` instead of the macro.

### 2. MTU used where frame length is needed

**mcdma_eth_ops.c:78**
```c
buf_len -= RTE_PKTMBUF_HEADROOM;
if (frame_len > buf_len) {
	PMD_DRV_LOG(ERR,
		    "port %u: Rx frame %u B exceeds mbuf data room %u B (MTU %u)",
		    dev->data->port_id, frame_len, buf_len,
		    mcdma_dev_mtu_get(dev));
	return -EINVAL;
}
```
The function correctly computes `frame_len` including overhead, but the comparison and error message conflate MTU (L3 payload size) with total frame length. The error message prints MTU but the actual check is against `frame_len`.

This is acceptable as long as `frame_len` is consistently derived from MTU using the overhead calculation. However, if the overhead is hardcoded incorrectly (see Error 1), this check will pass when it should fail.

**Fix:** Ensure `mcdma_rx_frame_len_from_mtu()` uses correct per-device overhead, then this comparison is correct. The error message wording is acceptable.

### 3. Queue release races with datapath

**mcdma_eth_ops.c:655-687, mcdma_dev_queue_release_common()**
```c
static void
mcdma_dev_queue_release_common(struct rte_eth_dev *dev, struct mcdma_queue *q,
			       uint16_t qid, bool is_rx)
{
	if (q == NULL)
		return;

	if (is_rx) {
		if (dev->data->rx_queue_state[qid] == RTE_ETH_QUEUE_STATE_STARTED) {
			if (mcdma_quiesce_queue(q) != 0)
				PMD_DRV_LOG(WARNING,
					    "Rx queue %u quiesce failed on release",
					    qid);
			mcdma_sw_ring_free_inflight(q);
			dev->data->rx_queue_state[qid] =
				RTE_ETH_QUEUE_STATE_STOPPED;
		}
		dev->data->rx_queues[qid] = NULL;
	} else {
		if (dev->data->tx_queue_state[qid] == RTE_ETH_QUEUE_STATE_STARTED) {
			if (mcdma_quiesce_queue(q) != 0)
				PMD_DRV_LOG(WARNING,
					    "Tx queue %u quiesce failed on release",
					    qid);
			mcdma_sw_ring_free_inflight(q);
			dev->data->tx_queue_state[qid] =
				RTE_ETH_QUEUE_STATE_STOPPED;
		}
		dev->data->tx_queues[qid] = NULL;
	}

	mcdma_dev_descq_disable(q);
	mcdma_dev_descq_free(q);
	rte_free(q);
}
```
The function sets `dev->data->{rx,tx}_queues[qid] = NULL` before calling `mcdma_dev_descq_disable(q)` and `rte_free(q)`. If a burst function is running concurrently (e.g., in another thread that has not yet checked the queue state), it may access `q` after it has been freed.

**Fix:** Move `dev->data->*_queues[qid] = NULL` to *after* `mcdma_dev_descq_disable()` and before `rte_free(q)`:
```c
	mcdma_dev_descq_disable(q);
	if (is_rx)
		dev->data->rx_queues[qid] = NULL;
	else
		dev->data->tx_queues[qid] = NULL;

	mcdma_dev_descq_free(q);
	rte_free(q);
```
This ensures the datapath cannot access a freed queue structure.

### 4. Missing scatter Rx / MTU rejection when frame exceeds mbuf

**mcdma_eth_ops.c:52-61, mcdma_rx_seg_len_for_pool()**
```c
buf_len -= RTE_PKTMBUF_HEADROOM;
if (frame_len > buf_len) {
	PMD_DRV_LOG(ERR,
		    "port %u: Rx frame %u B exceeds mbuf data room %u B (MTU %u)",
		    dev->data->port_id, frame_len, buf_len,
		    mcdma_dev_mtu_get(dev));
	return -EINVAL;
}
```
This correctly rejects an MTU that would produce frames larger than the mbuf data room. However, the driver does not advertise `RTE_ETH_RX_OFFLOAD_SCATTER` in `dev_info->rx_offload_capa`, and the configure path explicitly rejects it:

**mcdma_eth_ops.c:806-809**
```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;
}
```
So the only available behavior is to reject large MTUs. This is correct as implemented.

**No fix needed** -- the current behavior (reject MTU when frame > mbuf size) is acceptable when scatter Rx is unsupported. If scatter Rx is added in the future, the logic would need to auto-enable it or advertise the capability and allow the application to request it.

### 5. Resource leak on queue setup error path

**mcdma_eth_ops.c:348-370, mcdma_dev_tx_queue_setup()**
```c
ret = mcdma_sw_descq_alloc(dev, &txq, MCDMA_DIR_H2D, tx_queue_id,
			   nb_tx_desc, socket_id);
if (ret < 0)
	return ret;

ret = mcdma_dev_descq_alloc(dev, txq);
if (ret < 0) {
	mcdma_dev_descq_free(txq);
	rte_free(txq);
	return ret;
}

ret = mcdma_reset_queue(txq);
if (ret) {
	mcdma_dev_descq_free(txq);
	rte_free(txq);
	return ret;
}
```
`mcdma_sw_descq_alloc()` allocates `txq->sw_ring` via `rte_zmalloc_socket()` (line 229). On the error path after `mcdma_dev_descq_alloc()` or `mcdma_reset_queue()`, the code calls `mcdma_dev_descq_free(txq)` which frees the HW ring memzone but does NOT free `txq->sw_ring`. Then `rte_free(txq)` frees the queue structure but not the sw_ring allocation.

**mcdma_dev_descq_free() at line 168:**
```c
static void mcdma_dev_descq_free(struct mcdma_queue *q)
{
	const struct rte_memzone *qzone;
	char z_name[RTE_MEMZONE_NAMESIZE];
	/* ... */
	if (qzone)
		rte_memzone_free(qzone);

	rte_free(q->sw_ring);  // <-- this line DOES free sw_ring
}
```
Wait -- I misread. Line 176 *does* call `rte_free(q->sw_ring)`. So the leak claim is incorrect. `mcdma_dev_descq_free()` does clean up `sw_ring`.

**No error here** -- the cleanup is correct.

### 6. Use-after-free in burst Tx on multi-segment mbuf

**mcdma_eth_rxtx.c:284-311, mcdma_xmit_pkts()**
```c
for (count = 0; count < nb_pkts; count++) {
	/* ... */
	if (mcdma_descq_queue_prepare(q, mb, MCDMA_DIR_H2D) != 0)
		break;
}

rte_wmb();
if (count)
	mcdma_descq_queue_submit(q);

for (i = 0; i < count; i++) {
	q->tx_pkts++;
	q->tx_bytes += rte_pktmbuf_pkt_len(tx_pkts[i]);
}
```
After `mcdma_descq_queue_submit()` notifies the hardware, the DMA engine can begin transferring and completing packets. If a multi-segment packet completes before the stats loop runs, `mcdma_complete_requests()` (called on the next Tx burst or timer) will call `rte_pktmbuf_free(mb)` on the head mbuf (because EOF descriptor holds the sw_ring owner). This frees all segments. Then the stats loop accesses `tx_pkts[i]` which points to the now-freed head mbuf.

**Fix:** Accumulate stats *before* submitting to hardware:
```c
for (i = 0; i < count; i++) {
	q->tx_pkts++;
	q->tx_bytes += rte_pktmbuf_pkt_len(tx_pkts[i]);
}

rte_wmb();
if (count)
	mcdma_descq_queue_submit(q);
```

### 7. Experimental symbol export version mismatch

**rte_pmd_mcdma.c:56-83, 102-104**
```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)
{ /* ... */ }

RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mcdma_pio_write64, 26.11)
int
rte_pmd_mcdma_pio_write64(uint16_t port_id, uint64_t offset, uint64_t val)
{ /* ... */ }

RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mcdma_qcsr_read, 26.11)
/* ... */
```
The version `26.11` is in the future relative to the current DPDK release cycle (as of the patch timestamp 2026-11-09). If this is being submitted to an earlier release (e.g., 25.03 or 25.11), the version must match that release.

**Fix:** Use the correct version for the target DPDK release. If targeting 26.11, this is correct; if targeting 25.11, change to `25.11`; if targeting 25.03, change to `25.03`. Example:
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mcdma_pio_read64, 25.11)
```

---

## WARNINGS

### 1. Global variable without unique prefix (static linking clash risk)

**mcdma_eth.c:15-17**
```c
size_t config_csr_offsets[MCDMA_MAX_CONFIGS] = {
	0, 0x100000, 0x200000, 0x300000, 0x380000,
};
```
This global array is not `static` and has no `mcdma_` prefix. In a static link, `config_csr_offsets` can clash with identically named globals in other drivers or application code.

**Fix:** Declare it `static` (file-local) or prefix it `mcdma_config_csr_offsets`:
```c
static size_t config_csr_offsets[MCDMA_MAX_CONFIGS] = {
	0, 0x100000, 0x200000, 0x300000, 0x380000,
};
```

### 2. Missing `RTE_ETH_RX_OFFLOAD_SCATTER` in rx_offload_capa

**mcdma_eth_ops.c:749-755, mcdma_dev_infos_get()**
```c
dev_info->dev_capa =
	RTE_ETH_DEV_CAPA_RUNTIME_RX_QUEUE_SETUP |
	RTE_ETH_DEV_CAPA_RUNTIME_TX_QUEUE_SETUP;
dev_info->dev_capa &= ~RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP;
/* Multi-seg Tx: one sw_ring owner (warning: review has no Review-Result line, severity inferred from prose (response may be truncated)