Re: [PATCH v3-S1 0/5] dpaa2: bus, DMA and mempool base fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 15 Sep 2026 17:04:17 +0530
Prashant Gupta <prashant.gupta_3@nxp.com> wrote:

> This series is the first of four that upstream the missing NXP dpaa2
> driver changes. It collects the foundational bus/fslmc, dma/dpaa2 and
> mempool/dpaa2 fixes that the later series build on:
> 
> - defer fslmc bus initialization to probe and reduce probe-time logging
>   and MC traffic,
> - fix an array-bounds warning and validate IOVA in the dpaa2 QDMA
>   pre-populate helpers,
> - support fetching the mempool ops index from the primary process in a
>   secondary process.
> 
> Every commit builds cleanly (including the aarch64 DPAA cross build with
> -Werror) and the series is bisectable.
> 
> Gagandeep Singh (1):
>   dma/dpaa2: validate IOVA in pre-populate helpers
> 
> Hemant Agrawal (1):
>   bus/fslmc: reduce probe-time logging and MC traffic
> 
> Jun Yang (2):
>   dma/dpaa2: fix array-bounds warning in dequeue path
>   mempool/dpaa2: support ops index from primary in secondary
> 
> Prashant Gupta (1):
>   bus/fslmc: defer bus initialization to probe
> 
>  drivers/bus/fslmc/fslmc_bus.c            | 92 +++++++++++++----------
>  drivers/bus/fslmc/fslmc_vfio.c           |  3 +-
>  drivers/dma/dpaa2/dpaa2_qdma.c           | 96 ++++++++++++++++--------
>  drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 95 ++++++++++++++++++++++-
>  4 files changed, 209 insertions(+), 77 deletions(-)
> 

Detailed AI review finds errors

Review: [PATCH v3-S1 0/5] NXP fslmc/dpaa2 fixes
Base: main f43632a (26.11.0-rc0)

Series applies cleanly. Per-commit build of bus/fslmc, mempool/dpaa2,
dma/dpaa2 with -Dwerror=true passes at every commit.

Series summary
--------------
Patch 5 does not work: the IPC reply is parsed from the wrong offset,
so every secondary lookup fails, and the lookup sits in the dpaa2_sec
per-op enqueue path. Patch 3's commit message describes a change that
is not in the diff. Patch 1 reverts only half of cdefd2e980bd; the
DPAA bus has the same problem.


Patch 1/5: bus/fslmc: defer bus initialization to probe
-------------------------------------------------------
Warning:

cdefd2e980bd moved init into scan for both NXP buses. Only fslmc is
restored here. rte_dpaa_bus_scan() still calls
rte_mbuf_set_platform_mempool_ops(), which does
rte_memzone_reserve(), and dpaax_iova_table_populate(), which does
rte_zmalloc(). EAL runs rte_bus_scan() (eal.c:680) before
rte_eal_memzone_init() (784) and rte_eal_malloc_heap_init() (802), so
bus/dpaa is broken the same way. Fix both in this series, or say in
the commit message why dpaa is unaffected.

Info:

rte_fslmc_probe() returns 0 on every init failure and skips
rte_bus_generic_probe(), so a VFIO or DMA map failure results in no
devices and a zero return from rte_bus_probe(). This is the
pre-cdefd2e980bd behaviour, but now that the function is being
rewritten, returning ret would let EAL init fail visibly.


Patch 2/5: bus/fslmc: reduce probe-time logging and MC traffic
--------------------------------------------------------------
Warning:

The subject says "and MC traffic" but the patch only changes one log
level. Either drop that part of the subject or include the MC change.
Also fslmc_map_dma() is called from the memory hotplug callback, not
just at probe time.


Patch 3/5: dma/dpaa2: fix array-bounds warning in dequeue path
--------------------------------------------------------------
Error:

Commit message does not match the patch. It says an
idxs[DPAA2_QDMA_MAX_DESC] scratch buffer is added to struct
qdma_virt_queue and idxs[0] is used instead of &idx. Neither happens:
dpaa2_qdma_dq_fd() still passes &idx, and dpaa2_qdma.h is untouched.
The real change is rewriting qdma_cntx_idx_ring_eq() from a per-element
loop to a two-segment copy. Describe that, and state which compiler,
version and target emit the warning, since a Cc: stable fix for a
warning needs to be reproducible.

Warning:

New code uses rte_memcpy() for small variable-length copies of
uint16_t. Use memcpy(); rte_memcpy is being removed from non-datapath
and small-copy users tree-wide.

Info:

Merging the LONG/SG fle_sdd handling is an unrelated cleanup inside a
stable backport. Split it out, or drop it from the fix.


Patch 4/5: dma/dpaa2: validate IOVA in pre-populate helpers
-----------------------------------------------------------
Warning:

The check runs in the enqueue path. On failure fle[SRC].length stays
zero, so every subsequent copy on that object re-runs
rte_fslmc_cold_mem_vaddr_to_iova() plus rte_mem_virt2iova() and emits
DPAA2_QDMA_ERR, flooding the log at packet rate. fle_pool is created
by the driver in dpaa2_qdma_vchan_setup(); validate the pool mapping
once there (e.g. rte_mempool_mem_iter over mp chunks) and fail
vchan_setup, instead of checking per object on first use.


Patch 5/5: mempool/dpaa2: support ops index from primary in secondary
---------------------------------------------------------------------
Error:

Reply parsed at the wrong offset:

	rsp_msg = (void *)mp_reply.msgs;

mp_reply.msgs is struct rte_mp_msg *, whose first member is name[].
msg_type is read from the bytes "dp" of "dpaa2_pool_mp_sync", never
matches DPAA2_POOL_OPS_IDX_RSP, and every request fails with "received
invalid response". Must be:

	rsp_msg = (void *)mp_reply.msgs[0].param;

Also check mp_reply.nb_received == 1 rather than msgs != NULL, and
check len_param before copying msg_data. This path has clearly not
been exercised.

Error:

rte_dpaa2_mpool_get_ops_idx() is called per op in
dpaa2_sec_enqueue_burst() and the ordered variant
(dpaa2_sec_dpseci.c:1544, 1903). In a secondary, whenever the lookup
fails, s_dpaa2_pool_ops_idx stays RTE_MEMPOOL_MAX_OPS_IDX and the next
op issues another rte_mp_request_sync() with a 5 s timeout. That
happens always with the bug above, and also whenever the primary has
not created a dpaa2 pool (no action registered, primary replies
MP_IGN, nb_received is 0). A blocking IPC must not be reachable from
a datapath function; resolve once at init and cache the result,
including failure.

Warning:

The IPC is unnecessary. The mempool ops_index stored in the shared
struct rte_mempool is only valid because primary and secondary
register ops in the same order; the secondary already dispatches
through its own rte_mempool_ops_table with that index. The secondary
can find its own index by scanning rte_mempool_ops_table for
DPAA2_MEMPOOL_OPS_NAME, which removes the handler, the message
protocol and the timeout entirely.

Warning:

Missing Fixes: tag. The commit message describes secondaries failing
due to the unset index.

Info:

dpaa2_mbuf_pool_mp_primary() returns -ENOTSUP on an unknown type
without replying, so the requester waits the full timeout. The action
is never unregistered.

Commit message: rte_mp_action_register() returns ENOTSUP when IPC is
disabled at runtime (--no-shconf / --in-memory), not due to a build
time option.