Re: [RFC 0/4] net/mcdma: add Altera MCDMA poll mode driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 10 Nov 2026 00:09:22 +0800
"Subhransu S. Prusty" <subhransu.sekhar.prusty@altera.com> wrote:

> This RFC series introduces a poll mode driver for Altera Multi-Channel
> Direct Memory Access (MCDMA) PCI devices. The driver exposes
> host-to-device and device-to-host descriptor queues as standard DPDK
> Ethernet ports for custom FPGA datapaths.
> 
> While the MCDMA IP supports multiple PFs and VFs per PF, this RFC
> implements single PF with multiple VFs; each function exposes multiple
> Rx/Tx queue pairs backed by H2D/D2H descriptor rings and per-queue
> QCSR.
> 
> User guide reference:
> https://docs.altera.com/r/docs/683821/25.3.1/multi-channel-dma-ip-for-pci-express-user-guide/before-you-begin
> 
> The series is split for review as follows:
>  1. Core driver skeleton, queue management, documentation, and features
>     matrix, excluding datapath bursts, debug dumps, and PIO APIs.
>  2. Rx and Tx packet burst handlers.
>  3. Experimental PIO BAR access APIs.
>  4. Experimental QCSR inspection and dump APIs.
> 
> Functional testing was performed on Linux with vfio-pci, using DPDK
> pktgen on a single PF and VF configuration. The attached user logic
> was adapted as needed for validation.
> 
> We would appreciate feedback on the ethdev design, the experimental
> PIO APIs, and PF/VF devargs.

Lots of feedback from AI review. Mostly it needs to be tested with testpmd
before review consideration. And you are too late for 26.11

Review: [RFC 0/4] net/mcdma: add Altera MCDMA poll mode driver

Applied on 26.11-rc2, builds clean with -Dwerror at each commit.
Post-apply sources read, not just diffs.

Series-level
------------

Target release. 26.11 is at rc2; a new driver lands in 27.03.
RTE_EXPORT_EXPERIMENTAL_SYMBOL(..., 26.11) in patches 3/4 should say
27.03, and the series needs a release notes entry in the 27.03 file.

PCI ID and PF/VF model (you asked about devargs). The ID table is
vendor 0x1172, device 0x0000, i.e. the Altera default IDs. Any
Altera design left at default IDs will probe as MCDMA. Whether a
function is PF or VF is decided by the presence of pf_index=, and a
VF needs parent_pf_idx= to find its channel count. The usual way to
do this in DPDK is distinct device IDs for PF and VF (and a real
device ID for the MCDMA IP); then is_pf comes from the match entry
and both devargs go away. A VF probed with no devargs at all
currently fails (see patch 1).

PMD-specific API (you asked about it). Patch 4 reimplements what
ethdev already has: eth_dev_ops.get_reg / rte_eth_dev_get_reg_info_ext()
gives named register dumps that testpmd and dpdk-proc-info already
consume. Implement .get_reg and drop rte_pmd_mcdma_qcsr_read/dump.
Patch 3 (PIO) has no generic equivalent, see notes there.

testpmd. mcdma.rst says the driver has not been tested with testpmd.
It should be before a non-RFC submission; it will exercise close,
stop, mtu set and secondary process paths that pktgen does not.


Patch 1/4: net/mcdma: add Altera MCDMA poll mode driver skeleton
----------------------------------------------------------------

Errors

1. No .dev_close in mcdma_ethdev_ops. rte_eth_dev_close() returns
   -ENOTSUP, so queues are never released; on remove,
   mcdma_eth_dev_uninit() only frees mac_addrs:

	static int mcdma_eth_dev_uninit(struct rte_eth_dev *dev)
	{
		rte_free(dev->data->mac_addrs);

   Every queue's memzone, sw_ring and struct mcdma_queue leak, and
   the HW queues stay enabled. Add:

	static int mcdma_dev_close(struct rte_eth_dev *dev)
	{
		uint16_t i;

		if (rte_eal_process_type() != RTE_PROC_PRIMARY)
			return 0;
		for (i = 0; i < dev->data->nb_rx_queues; i++)
			mcdma_dev_rx_queue_release(dev, i);
		for (i = 0; i < dev->data->nb_tx_queues; i++)
			mcdma_dev_tx_queue_release(dev, i);
		return 0;
	}

   and have uninit call it. rte_eth_dev_release_port() already frees
   mac_addrs, so uninit need not.

2. MTU read from dev_conf after configure:

	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;

   rxmode.mtu is the request consumed by configure; after that
   dev->data->mtu is authoritative and rte_eth_dev_set_mtu() only
   updates dev->data->mtu. mtu_set() works around this by writing
   dev->data->dev_conf.rxmode.mtu = mtu itself (line 744), which a
   PMD must not do. Delete mcdma_dev_mtu_get() and the rxmode.mtu
   store; use dev->data->mtu everywhere (ethdev initialises it to
   RTE_ETHER_MTU, so no fallback is needed).

3. Rx queue stop cannot succeed on an idle queue, and the release
   path frees buffers the HW still owns.

	static int mcdma_wait_queue_drain(struct mcdma_queue *q)
	{
		for (retry = 0; retry < MCDMA_CNT_HEAD_MOVE; retry++) {
			rte_rmb();
			if (q->cons_didx == q->prod_didx)
				return 0;

   For D2H, cons_didx only advances when packets arrive, so with
   buffers posted and no traffic this spins 1 s and returns
   -ETIMEDOUT. mcdma_quiesce_queue() returns before disabling or
   resetting the queue, rx_queue_stop() returns the error with the
   queue still STARTED, and dev_stop() fails. In
   mcdma_dev_queue_release_common() the failure is only logged and
   execution continues:

			mcdma_sw_ring_free_inflight(q);
		...
		mcdma_dev_descq_disable(q);

   so mbufs are freed while the queue is still enabled, then the
   queue is disabled. Fix: disable (Q_CTRL=0) and reset the queue
   first, then reclaim freed..prod unconditionally; only H2D needs
   a bounded drain wait before that, and it should fall through to
   disable+reset on timeout rather than return.

4. No secondary-process handling while mcdma.ini claims
   "Multiprocess aware = Y". mcdma_eth_dev_init() runs the same
   code in every process:

	priv->dev = dev;
	...
	ret = mcmda_parse_args(priv, pci_dev);
	...
	dev->data->mac_addrs = rte_zmalloc("mcdma", ...);

   A secondary overwrites the shared priv->dev with its own local
   rte_eth_dev pointer (dereferenced later by the primary in
   descq_free/queue_init), reprograms the VF weight CSRs, and
   replaces the shared mac_addrs pointer (leaking the primary's).
   Either add the standard early return for RTE_PROC_SECONDARY
   after setting burst pointers, or change the .ini to N. Also
   drop the dead `dev->data->dev_private = priv;`.

5. dpdk-devbind.py entry does not match the device the driver
   binds:

	+  {'Class': '06', 'Vendor': '8086', 'Device': '1172'},  # Altera FPGA

   The driver matches vendor 0x1172 device 0x0000; here 0x1172 is in
   the Device field with Intel's vendor ID, and class 06 is a bridge.
   Should be {'Class': <device class>, 'Vendor': '1172', 'Device':
   '0000'} (or the real device ID, see cover).

Warnings

6. BAR 0 is used before it is checked:

	priv->bar_base = pci_dev->mem_resource[MCDMA_CONFIG_BAR].addr;
	ret = mcmda_parse_args(priv, pci_dev);   /* writes VF weight CSRs */
	...
	PMD_DRV_LOG(INFO, "MCDMA RTL VERSION : 0x%x",
		    mcdma_dmacsr_reg_readl(priv->bar_base, MCDMA_CSR_VER_NUM));

   Return -ENODEV if mem_resource[0].addr == NULL (as is done for the
   PIO BAR).

7. Devargs handling is inverted:

	if (!devargs)
		return -EINVAL;
	kvlist = rte_kvargs_parse(devargs->args, mcdma_kvargs_keys);
	if (!kvlist)
		return 0;

   A device probed without -a (no devargs) fails probe; a device with
   a malformed or unknown key (rte_kvargs_parse() returns NULL) is
   accepted silently. Swap: no devargs is the VF default, NULL kvlist
   is -EINVAL.

8. ip_num_chan is taken from a 16-bit register field and never
   bounded, but drives the QCSR address:

	uint32_t qcsr_offset = (dir == MCDMA_DIR_H2D) ?
			       (MCDMA_QCSR_H2D_OFFSET + (q_id * MCDMA_QCSR_SIZE)) :
			       (q_id * MCDMA_QCSR_SIZE);

   For q_id >= 2048 a D2H queue's QCSR lands in the H2D region.
   MCDMA_MAX_CHAN_PER_FUNC (512) and MCDMA_QCSR_NUM_MAX_PER_DIR
   (2048) exist but are unused; clamp ip_num_chan to
   MCDMA_MAX_CHAN_PER_FUNC in dev_init.

9. Writeback target is a 16-bit field in the middle of the hot
   software state:

		volatile uint16_t freed_cons_didx;
		uint16_t prod_didx;
		volatile uint16_t cons_didx;
		uint16_t submitted_didx;
	...
	cons_idx = rte_mem_virt2iova((const void *)(uintptr_t)&q->cons_didx);

   Q_HEAD_POINTER / Q_COMPLETED_POINTER are 32-bit registers (error
   flag in bit 24). If the IP writes back the 32-bit register image,
   submitted_didx is clobbered on every completion. Independent of
   width, DMA writes into the same cache line as prod_didx and the
   stats cause ping-pong with the lcore polling the queue. Give the
   writeback its own cache-line-aligned uint32_t (or a slot in the
   descriptor memzone) and read the 16-bit index from it. The
   volatile on freed_cons_didx (SW-only) is noise.

10. Whole-IP reset from dev_configure():

	if (mcdma_dev->is_pf && !mcdma_dev->pf_idx) {
		mcdma_dmacsr_reg_writel(mcdma_dev->bar_base,
					MCDMA_CSR_SW_RESET, 0x1);
		rte_delay_ms(MCDMA_SW_RESET_DELAY_MS);

   dev_configure can be called repeatedly (every reconfigure), it
   blocks 1 s, and MCDMA_CSR_SW_RESET resets every function's queues,
   including VFs owned by other processes or VMs. Do this once at
   probe, or not at all; configure is not the place.

11. MMIO accessors reinvent rte_io.h and then pay for full fences:

	static inline uint32_t mcdma_reg_readl(volatile void *addr)
	{
		return *(volatile uint32_t *)addr;
	}
	...
	rte_wmb();
	mcdma_qcsr_reg_writel(q->qcsr, MCDMA_QCSR_Q_TAIL_POINTER, hw_tail);

   Use rte_read32()/rte_write32()/rte_read64()/rte_write64(); they
   carry the required rte_io_*mb() (a compiler barrier on x86) and
   the explicit rte_wmb()/rte_rmb() (sfence/lfence on x86, in every
   burst) go away. Same for mcdma_pio_reg_read64/write64.

12. Const and namespace:

	size_t config_csr_offsets[MCDMA_MAX_CONFIGS] = {
	static struct rte_pci_id mcdma_pci_id_tbl[] = {
	static struct eth_dev_ops mcdma_ethdev_ops = {

   config_csr_offsets is an unprefixed exported global (static
   linking clash); make all three `static const`.

13. Debug output at ERR level in rx_queue_setup:

	if (mb_pool)
		PMD_DRV_LOG_DBG(ERR,
				"qid: %u, mbuf_avail_count = %u, mbuf_in_use_count = %u",

   MCDMA_LOG_ENABLE is hard-wired to 1 so this prints at ERR on every
   queue setup. Drop it, or use DEBUG level with plain PMD_DRV_LOG
   and remove the PMD_DRV_LOG_DBG/MCDMA_LOG_ENABLE pair.

14. mtu_set on a started queue updates rxq->rx_seg_len but
    Q_PAYLOAD_COUNT is only written from mcdma_dev_queue_init():

	mcdma_qcsr_reg_writel(q->qcsr, MCDMA_QCSR_Q_PAYLOAD_COUNT, payload);

    If that register bounds the D2H transfer, a larger MTU set at
    runtime is not applied until the queue is restarted. Either
    reprogram it in mtu_set or return -EBUSY when any Rx queue is
    started.

15. didx numbering is inconsistent with its own comments:

	/* 16 bits - Monotonic index and IP requires it to start from 1 */
	uint16_t didx;
	...
	#define MCDMA_DIDX_NEXT(cur, num_descs) \
		((uint16_t)((((uint16_t)(cur) + 1) % (num_descs))))

    MCDMA_DIDX_NEXT yields 0..num_descs-1, so after num_descs
    descriptors the didx (and the tail pointer written to HW) is 0,
    which the comments and mcdma_hw_didx_to_ring() treat as "none".
    The unused mcdma_hw_didx_nudge() and MCDMA_RING_TO_HW_DIDX()
    implement 1..num_descs. One of the two schemes is wrong; pick
    one, document it, delete the other.

16. dev_info inconsistency:

	dev_info->max_rx_pktlen = MCDMA_MAX_RX_FRAME_LEN;   /* 0x1FFFFF */
	dev_info->max_mtu = MCDMA_MAX_MTU;                  /* 65535 */

    max_rx_pktlen should be max_mtu + MCDMA_RX_ETH_OVERHEAD.

17. Documentation does not match code:
    - mcdma.ini: "Runtime Rx queue setup" / "Runtime Tx queue setup"
      are not feature keys; default.ini has "Runtime queue setup".
      "Extended stats" is missing although xstats_get is implemented.
      "Multiprocess aware = Y" (see 4).
    - mcdma.rst: build option is -Denable_drivers=net/altera/mcdma,
      not net/mcdma; says xstats are not implemented (they are);
      says SOF/EOF are tracked in an mbuf dynamic field (nothing in
      the code does; MCDMA_DESC_SOF/EOF are unused).
    - index.rst: mcdma inserted after memif; alphabetical is before.

Info

18. (uint32_t)log2(q->num_descs) uses libm on an integer power of
    two; rte_log2_u32(q->num_descs).

19. mac_addrs is left all-zero. Use rte_eth_random_addr() as the
    other DMA/virtual PMDs do; RTE_ETH_DEV_NOLIVE_MAC_ADDR does
    nothing without a mac_addr_set op.

20. mcdma_qcsr_read_ptr_errors() returns the state of two sticky
    bits (0..2) and stats_get adds it to ierrors/oerrors on each
    call, so the "counter" is a gauge that never clears.

21. vf_weight_parse_handler(): uint8_t weight_val = atoi(token)
    truncates (257 -> 1 passes the range check); use strtoul.
    Function name mcmda_parse_args() is a typo.

22. mcdma_dev_descq_free() looks the memzone up by name; keep the
    memzone pointer in struct mcdma_queue instead. q->qbuf_dma can
    be qzone->iova.

23. Leftovers, all unused: MCDMA_QCSR_Q(), MCDMA_MAX_PYLD_CNT,
    MCDMA_MAX_VF_PER_PF, MCDMA_IFC_DATA_WIDTH, MCDMA_MAX_CHAN,
    MCDMA_PREV_IDX, MCDMA_RING_AVAIL_COUNT, mcdma_queue_cons_ring(),
    mcdma_qcsr_reg_updatel(), mcdma_pio_reg_readl/writel(),
    priv->bam (assigned, never read), RTE_PCI_DEV_ID_DECL_XNIC
    macro defined inside the initialiser.


Patch 2/4: net/mcdma: add Rx and Tx packet burst paths
------------------------------------------------------

Errors

1. Heap allocation on every rx_burst:

	repl = rte_malloc_socket(NULL, q->burst_size * sizeof(*repl), 0,
				 q->socket_id);
	...
	rte_free(repl);

   rte_malloc takes the heap lock and this runs on every poll that
   refills. burst_size is fixed at MCDMA_MAX_BURST (32):

	struct rte_mbuf *repl[MCDMA_MAX_BURST];

Warnings

2. rte_mempool_avail_count() per poll:

	mb_avail = rte_mempool_avail_count(q->mb_pool);

   It walks every lcore cache (O(RTE_MAX_LCORE)) and is documented
   as not for the datapath. Just call rte_pktmbuf_alloc_bulk() (which
   also replaces the get_bulk + per-mbuf rte_pktmbuf_reset loop) and
   handle failure.

3. ERR log in the Rx hot path on mbuf exhaustion:

		q->rx_nombuf += batch;
		PMD_DRV_LOG(ERR, "No MBUFS, queue id = %u, nb_avail = %u, batch: %u, "

   Pool exhaustion is a steady state under load; this is a log
   storm. The counter is enough; if a message is wanted use
   RTE_LOG_DP_LINE.

4. Rx mbufs never get m->port:

	mb->pkt_len = d->rx_pyld_cnt;
	rte_pktmbuf_data_len(mb) = d->rx_pyld_cnt;

   Add mb->port = q->ethdev->data->port_id (cache port_id in the
   queue rather than chasing ethdev).

Info

5. The "nudge" branch in mcdma_descq_queue_submit() is unreachable:
   both callers only submit when prod_didx != submitted_didx, and
   ring_avail() reserves a slot so a full wrap cannot make them
   equal. Remove it (and the `volatile uint16_t qhead` local, and
   the 100000-iteration busy loop without delay).

6. mcdma_desc_slots_for_segs()/mcdma_desc_segs_for_slots() read
   d->link from the descriptor ring to locate link slots. Link slots
   are at (idx & 127) == 127 by construction; compute instead of
   touching DMA memory.

7. Write-only fields: q->processed_cur, q->ring_avail,
   q->ring_to_submit, q->complete. Remove.

8. In mcdma_descq_queue_prepare(): the seg_len > MCDMA_IP_MAX_FRAME_LEN
   clamp is dead (data_len is uint16_t); `if (mb->nb_segs == 0) break;`
   returns short on a corrupt mbuf, which the caller will resend
   forever. Drop both.


Patch 3/4: net/mcdma: add experimental PIO access APIs
------------------------------------------------------

Info

1. Each 64-bit access does rte_eth_dev_is_valid_port() + strcmp on
   the driver name + bounds check. An application polling user-logic
   registers will not want that per access. Consider instead a
   one-time rte_pmd_mcdma_pio_bar_get(port_id, void **addr, size_t
   *len) and let the caller use rte_read64/rte_write64, or at least
   add 32-bit variants; most AVMM user logic is 32-bit.

2. MCDMA_PIO_BAR is hard-wired to BAR 2 while the BAM BAR is a
   devarg. Either both configurable or both fixed.

3. Symbol version 26.11 -> 27.03 (see cover).


Patch 4/4: net/mcdma: add experimental QCSR dump APIs
-----------------------------------------------------

Warnings

1. Replace rte_pmd_mcdma_qcsr_read()/dump() with eth_dev_ops.get_reg.
   rte_eth_dev_get_reg_info_ext() supports a filter string and
   register names, so "show port <n> regs" in testpmd and
   dpdk-proc-info give the same dump with no PMD-specific header.
   The SW indices (prod/cons/freed/submitted) can be exposed as
   pseudo-registers or dropped; exposing struct mcdma_queue internals
   through an installed header (struct rte_pmd_mcdma_qcsr) ties ABI
   to driver internals.

Info

2. mcdma_qcsr_regs_read() reads the SW indices unsynchronised with
   the polling lcore; fine for a debug dump, but the rte_rmb() there
   buys nothing.