Re: [PATCH 00/45] net/dpaa2: features and fixes for NXP DPAA2 drivers

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu,  3 Sep 2026 19:23:08 +0530
Prashant Gupta <prashant.gupta_3@nxp.com> wrote:

> This series brings the NXP DPAA2 driver stack up to date with the
> functionality carried in the NXP internal tree, together with a number of
> bug fixes. It covers the crypto (dpaa2_sec), net, dma, mempool, event and
> bus/fslmc drivers.


Since it was big, ran AI review with Fable. It stopped after finding
errors in the first 8 patches.

DPAA2 series review (bundle 2096), patches 1-8 of 45
Base: upstream main d55ccd4; all 45 patches apply cleanly with
git am --3way.

Patch 3: crypto/dpaa2_sec: support AES-GMAC

Warning: The patch adds RTE_CRYPTO_AEAD_AES_GMAC to the public
enum rte_crypto_aead_algorithm in lib/cryptodev/rte_crypto_sym.h
but does not add it to crypto_aead_algorithm_strings[] in
lib/cryptodev/rte_cryptodev.c, so
rte_cryptodev_get_aead_algo_string() returns NULL for it and
rte_cryptodev_get_aead_algo_enum("aes-gmac") fails, which also
breaks test-crypto-perf/testpmd-style string selection. There is
no entry in doc/guides/rel_notes/release_26_11.rst for the new
public API value. The lib/cryptodev change should be its own
patch ahead of the driver patch, with the string table and release
note.

Warning: dpaa2_sec_capabilities[] (the same table returned by
rte_cryptodev_info_get() via dpaa2_sec_dev_infos_get) now
advertises AES-GMAC as a symmetric AEAD capability, but only the
IPsec security path (dpaa2_sec_ipsec_aead_init) handles it; the
plain-crypto dpaa2_sec_aead_init() falls into the default case and
returns -ENOTSUP. An application that walks
rte_cryptodev_sym_capability_get() will find AES-GMAC and then fail
session creation. Either advertise it only through the security
capability crypto_capabilities or add the plain AEAD path.

Patch 6: crypto/dpaa2_sec: add support for env variables

Error: The env fallback overrides devargs rather than acting as a
fallback. dpaa2_sec_get_devargs() is called twice from
dpaa2_sec_dev_init(), once per key:

	dpaa2_sec_get_devargs(cryptodev, DRIVER_DUMP_MODE);
	dpaa2_sec_get_devargs(cryptodev, DRIVER_STRICT_ORDER);

and the env_set: block reads both environment variables
unconditionally:

	env = getenv(DRIVER_STRICT_ORDER);
	if (env)
		internals->en_loose_ordered = !atoi(env);

	env = getenv(DRIVER_DUMP_MODE);
	if (env) {
		dpaa2_sec_dp_dump = atoi(env);

With devargs "drv_dump_mode=2" and env drv_dump_mode=0, the first
call sets dump mode 2 from devargs and returns; the second call
finds no drv_strict_order key, jumps to env_set, and overwrites
dpaa2_sec_dp_dump with 0 from the environment. The same happens
in the other direction for en_loose_ordered. Read the env vars once
after both devargs keys have been processed, and only for keys that
were absent from devargs.

Warning: getenv() in a driver. Devargs already exist for both of
these knobs; per-device runtime configuration belongs in devargs,
and checkpatches flags getenv in drivers/ as a forbidden token.
Lower-case names like "drv_strict_order" are also unusual for
environment variables and easy to confuse with the devargs keys.

Patch 7: drivers: fix double free of dpaa2 device on uninit

Error: Moving dpaa2_dpdmai_dev_uninit() ahead of
rte_dma_pmd_release() in dpaa2_qdma_remove() breaks both teardown
orders.

	dpaa2_dpdmai_dev_uninit(dmadev);

	ret = rte_dma_pmd_release(dpaa2_dev->device.name);

(a) Application called rte_dma_close() before the device is
removed (rte_dev_remove / hotplug unplug -> fslmc_bus_unplug_device
-> drv->remove). rte_dma_close() -> dma_release() does
memset(dev, 0, sizeof(struct rte_dma_dev)), so dpaa2_dev->dmadev
points at a zeroed slot and dpaa2_dpdmai_dev_uninit() dereferences
dev->data (NULL):

	struct dpaa2_dpdmai_dev *dpdmai_dev = dev->data->dev_private;

(b) Device removed without a prior close. uninit runs first, does
rte_free(qdma_dev) and sets dpdmai_dev->qdma_dev = NULL. Then
rte_dma_pmd_release() sees state READY, calls rte_dma_close() ->
dpaa2_qdma_close(), which immediately returns:

	if (!qdma_dev)
		return 0;

so qdma_dev->vqs, the per-VQ fle_pool mempools, ring_cntx_idx and
the Rx queue storage are never freed. Before this patch close ran
uninit last, after freeing those. Suggested order in remove: if the
dmadev is still READY call rte_dma_close() (or dpaa2_qdma_close())
first so the VQ resources are released, then dpdmai_close() the MC
object and free qdma_dev, and only then rte_dma_pmd_release(); and
guard uninit against a dmadev that has already been released.

Info: struct rte_dma_dev is used as a pointer member in
bus_fslmc_driver.h without a forward declaration; adding
"struct rte_dma_dev;" avoids an implicit file-scope tag
declaration inside the struct.

Patch 8: net/dpaa2: fix integer overflow in CCSR region mapping

Warning: The fix is incomplete. page_size is validated, but the
two lines immediately before still use the PAGE_MASK macro, which
expands to ~(sysconf(_SC_PAGESIZE) - 1) and is the same
unchecked signed value Coverity complained about:

	start = addr & PAGE_MASK;
	offset = addr - start;
	len = len & PAGE_MASK;

Compute a local mask from the validated page_size
(e.g. uint64_t page_mask = ~((uint64_t)page_size - 1)) and use it
for start and len; the later "len & ~(page_size - 1)" is then
redundant with the earlier "len = len & PAGE_MASK".

Review-Result: ERROR