Re: [PATCH v24 00/27] NXP DPAA driver enhancements and fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 12 Oct 2026 13:38:18 +0530
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> This series collects a set of fixes and enhancements for the NXP DPAA
> bus, mempool, dma, crypto and net drivers targeting 26.11.
> 
> It includes memory-leak and resource-cleanup fixes on the device
> remove/close paths, more robust frame queue and congestion-group
> shutdown, secondary-process safety guards, BPID and cgrid lifecycle
> handling, and several new features: offline (O/H) port device support,
> enhanced virtual storage profile (VSP) port support, fmcless Rx queue
> configuration via devargs, Rx/Tx taildrop threshold devargs, non
> fmX-macY shared Ethernet naming, and DMA scatter-gather and
> errata-workaround devargs. Documentation and release notes are updated
> accordingly.
> 
> v24:
> * fix clang 23 compilation
 
Still lots of open issues from AI review.

Review of [PATCH v24 01/27..27/27] DPAA bus/net/mempool/dma/crypto

The series applies to main (693eeab) and every commit builds with
-Dwerror=true for bus/dpaa, mempool/dpaa, net/dpaa, dma/dpaa and
crypto/dpaa_sec.

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

New per-driver devargs keep getting added where ethdev already has the
knob. drv_rx_taildrop is already overridden by nb_rx_desc (see 19);
Tx should take its threshold from nb_tx_desc the same way. A DPAA port
should not need different command line options than every other PMD
to control queue depth.

The offline port API in 25 is an ioctl pass-through to an out-of-tree
kernel module (/dev/cdx_ctrl) carrying application specific GTP and
local-gateway configuration. That belongs in the application or the
vendor SDK, not in a DPDK installed header. If it stays, it needs a
real justification in the commit message, proper namespacing, docs
and a test.


[PATCH v24 11/27] drivers: add DPAA cgrid cleanup support

Error: CGRIDs are released while the port's own FQs still reference
them.

dpaa_eth_dev_close() does not retire the port's Rx/Tx FQs before the
CGR teardown, so they are still scheduled with CGE set and a cgid in
range. dpaa_cgr_stale_fq_cleanup() therefore finds this port's own
queues, not leftovers from a previous run, and retires them with
qman_shutdown_fq_by_fqid(), i.e. on the affine portal. A push mode Rx
FQ is scheduled on its own portal's dedicated channel (rxq->ch_id); if
frames are queued the retire goes PENDING and patch 12 returns -EBUSY.
A Tx FQ with frames pending hits the DCP channel case, also -EBUSY.
The loop breaks and then, unconditionally:

	qman_release_cgrid_range(dpaa_intf->cgr_rx[0].cgrid,
				 dpaa_intf->nb_rx_queues);

returns the CGRID to the allocator while an FQ still points at it; the
next port probed can get the same ID. The probe error path has the
same problem: Rx FQs already passed to qman_init_fq() with that cgid
are left in place while free_rx releases the range. Before this patch
the IDs leaked, which was the safe failure.

qman_delete_cgr() can also fail (-EINVAL when called from a different
portal than the creator, without unlinking the node); the range is
released anyway.

Retire the port's own queues by descriptor first (patch 10 made
qman_shutdown_fq() use fq->qp, which is what push mode needs), and
only return the IDs when everything came down cleanly:

	bool clean = true;

	for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++)
		if (qman_shutdown_fq(&dpaa_intf->rx_queues[loop]) != 0)
			clean = false;

	dpaa_cgr_stale_fq_cleanup(...);	/* real leftovers only */

	for (loop = 0; loop < dpaa_intf->nb_rx_queues; loop++)
		if (qman_delete_cgr(&dpaa_intf->cgr_rx[loop]) != 0)
			clean = false;
	if (clean)
		qman_release_cgrid_range(dpaa_intf->cgr_rx[0].cgrid,
					 dpaa_intf->nb_rx_queues);

Same for Tx, and the same rule on the free_tx/free_rx probe path.


[PATCH v24 19/27] net/dpaa: support Rx/Tx taildrop threshold devarg

Warning: drv_rx_taildrop does not do what dpaa.rst says.
dpaa_eth_rx_queue_setup() already sets the CGR threshold from the ring
size:

	qm_cgr_cs_thres_set64(&cgr_opts.cgr.cs_thres, nb_desc, 0);
	ret = qman_modify_cgr(&dpaa_intf->cgr_rx[queue_idx], 0, &cgr_opts);

so any non-zero drv_rx_taildrop is replaced by nb_rx_desc at queue
setup. The only visible effect is that =0 skips CGR creation. Drop
drv_rx_taildrop.

For Tx, dpaa_eth_tx_queue_setup() has nb_desc marked __rte_unused. Use
it the same way as Rx (qman_modify_cgr() on cgr_tx with nb_desc) and
drop drv_tx_taildrop along with the DPAA_TX_TAILDROP_THRESHOLD
getenv(). NAK on the devargs as posted.


[PATCH v24 24/27] net/dpaa: enhance VSP port support

Warning: the shared-MAC base profile is no longer exempt from the
one-pool-per-VSP check. dpaa_eth_rx_queue_setup() stores the pool
before calling dpaa_port_vsp_update():

	dpaa_intf->vsp[vsp_id].vsp_bp[0] = DPAA_MEMPOOL_TO_POOL_INFO(mp);
	dpaa_intf->vsp[vsp_id].bp_num = 1;

and the shared base profile early return only clears bp_num, leaving
vsp_bp[0] set. Previously that case set vsp_bpid to 0, so
dpaa_eth_rx_queue_bp_check() let a second queue on that profile use a
different mempool; now it fails with "Various MPs are assigned". Test
bp_num in the check:

	if (dpaa_intf->vsp[vsp_id].bp_num != 0 &&
	    bpid != dpaa_intf->vsp[vsp_id].vsp_bp[0]->bpid)


[PATCH v24 25/27] drivers: add offline (O/H) port device support

Error: NULL dereference on the first Tx burst. dpaa_oldev_init() sets
the back pointer only on the Rx FQ:

	dpaa_intf->rx_queues[0].dpaa_intf = dpaa_intf;

dpaa_ol_dev_start() installs dpaa_eth_queue_tx(), which does

	struct dpaa_if *dpaa_intf = fq->dpaa_intf;
	...
	if (unlikely(dpaa_intf->ts_enable)) {

and tx_queues[0].dpaa_intf is NULL from rte_zmalloc(). Add

	dpaa_intf->tx_queues[0].dpaa_intf = dpaa_intf;

after dpaa_ol_tx_queue_init(). Multi-segment Tx also allocates from
dpaa_tx_sg_pool, which only net_dpaa probe creates; with drv_oldev and
no fmX-macY port allowed it is NULL.

Warning: rte_pmd_dpaa_oldev.h is installed but exports unprefixed
names: struct dpa_ip_addr_s, dpa_ip_pair_s, dpa_lgw_subnet_s and the
DPA_ISC_*, DPDK_OLDEV_*, DPDK_CLASSIF_*, DPDK_TELECOM_* macros. All
need an rte_pmd_dpaa_ / RTE_PMD_DPAA_ prefix. The structs and flags
have no Doxygen, and the four new functions have no test or testpmd
hook.

Warning: drv_oldev as a bus devarg means any user of the offline port
must also list every Ethernet port with -a, since a bus argument flips
the bus to allowlist scan (the doc spells this out). Create the device
when /dev/cdx_ctrl exists instead and drop the devarg.

Info: rte_dpaa_bus_dev_build() now continues with dpaa_netcfg NULL
when drv_oldev is set, but under RTE_LIBRTE_DPAA_DEBUG_DRIVER

	dump_netcfg(dpaa_netcfg, stdout);

dereferences it.


[PATCH v24 08/27] bus/dpaa: scan max BPID from DTS

Info: the commit message says it updates "the default BMAN HW version
for Layerscape devices"; the code removes version detection. It also
makes bman_global_init() fail when the fsl,bman node is missing or
bman_init_ccsr() fails, where it only logged before. Say so.


[PATCH v24 18/27] dma/dpaa: add devargs for SG and errata workaround

Info: dpaa_dma_data_validation and dpaa_dma_sg_disable are documented
as debugging aids. Debug knobs do not need to be stable user facing
devargs; keep validation under RTE_LIBRTE_DPAA_DEBUG_DRIVER or a log
level instead.


[PATCH v24 20/27] net/dpaa: support fmcless rxq number as devarg

Info: with the default now creating DPAA_MAX_NUM_PCD_QUEUES FQs, the
application already picks how many it uses with nb_rx_queues at
configure. drv_fmcless_rxq only trims resources; consider dropping it.


[PATCH v24 21/27] net/dpaa: support non fmX-macY type of shared ...

Info: rather than having the user supply the kernel interface name,
the driver can look up the netdev bound to the same MAC node in sysfs.
That removes the devarg and works after udev renames.


[PATCH v24 22/27] bus/dpaa: optimize DPAA multi-entry buffer pool ...

Info: the comment and commit message say entry 0 is written separately
to avoid the valid bit, but

	r->bufs[0].opaque = bm_bufs[0].opaque;

stores byte 0, which is __dont_write_directly__verb. It is harmless
(bm_rcr_start() zeroed the line and bm_rcr_pvb_commit() writes the
verb last), but then the split is no different from one memcpy().
Either write only bpid/hi/lo for entry 0, as the kernel does, or drop
the claim. bm_buffer_get64_from_be() should be a static inline, not a
statement-expression macro.


[PATCH v24 01/27] net/dpaa: fix double close and null deref on remove

Info, pre-existing: rte_dpaa_remove() frees dpaa_tx_sg_pool without
clearing it:

	rte_mempool_free(dpaa_tx_sg_pool);

A later probe (hotplug after the last remove) sees a non-NULL pointer
in rte_dpaa_probe() and uses the freed pool. Set it to NULL.

Review-Result: ERROR