Re: [PATCH v11 00/25] DPAA driver fixes and improvements
Stephen Hemminger <[email protected]>
| Newsgroups | org.dpdk.dev |
|---|---|
| Message-ID | <[email protected]> |
On Thu, 13 Aug 2026 20:11:40 +0530 Hemant Agrawal <[email protected]> wrote: > v11: address AI review comments; skipping false positive ones. > - net/dpaa: fix lost close error in rte_dpaa_remove() > - net/dpaa: fix NULL dereference on rte_dpaa_bpid_info in oldev rx > queue setup > - net/dpaa: fix error logging in oldev tx queue setup to include > return code and return proper -EINVAL > > v10: address further AI review comments > - dma/dpaa: expose compile-time static flags (sg_enable, > data_validation, pci_read) as probe-time devargs with corrected > logic and updated documentation > - bus/dpaa: fix pool-channel FQ shutdown always returning -EINVAL > (wrong affinity check; dedicated and pool channel ranges never > overlap) > - mempool/dpaa: fix double-free of bpid_info on pool release > > v9: fix bissect issue and other AI review comments > > This series collects a set of correctness fixes, cleanups and feature > additions across the NXP DPAA bus, net, mempool and DMA drivers. > > 1. Bus/fman infrastructure cleanups (patches 01, 02, 12) > - bus/dpaa: refine fman naming and fix global scope > - bus/dpaa: scan max BPID from DTS > - bus/dpaa: improve log macro and fix bus detection > > 2. Process-type guards (patch 03) > - drivers: add process-type guards to prevent segfaults in secondary > > 3. FQ shutdown hardening (patches 04-08) > - bus/dpaa: shutdown DPAA FQ by fq descriptor > - bus/dpaa: improve FQ shutdown with channel validation > - bus/dpaa: add DPAA cgrid cleanup support > > 4. net/dpaa improvements (patches 09, 11, 13) > - net/dpaa: optimize FM deconfig > - net/dpaa: optimize FMC MAC type parsing > > 5. Statistics (patch 10) > - drivers: add BMI Tx statistics > > 6. mempool/dpaa (patch 14) > - drivers: release DPAA bpid on driver destructor > > 7. dma/dpaa (patches 15-16) > - dma/dpaa: add SG data validation and ERR050757 fix > - dma/dpaa: expose static flags as probe-time devargs > > 8. net/dpaa features (patches 17-22) > - net/dpaa: support Rx/Tx taildrop threshold devarg > - net/dpaa: add Tx rate limiting API > - bus/dpaa: orp queue create and burst enqueue > - net/dpaa: support fmcless rxq number as devargs > - net/dpaa: support non fmX-macY type of shared Ethernet name > - bus/dpaa: optimize DPAA multi-entry buffer pool operations > > 9. Shutdown and cleanup (patches 23-24) > - drivers: improve shutdown fq with channel > - drivers: improve crypto fq resource handling > - net/dpaa: enhance VSP port support > - drivers: add offline (O/H) port device support > - drivers: add dpaax enter destructor to gate EAL memory frees > > 10. Documentation and release notes (patch 25) > - doc: update release notes with NXP DPAA changes > > Gagandeep Singh (3): > net/dpaa: fix device remove > dma/dpaa: add SG data validation and ERR050757 > drivers: add offline (O/H) port device support > > Hemant Agrawal (6): > net/dpaa: optimize FM deconfig > net/dpaa: support Rx/Tx taildrop threshold devarg > net/dpaa: add Tx rate limiting API > net/dpaa: support non fmX-macY type of shared Ethernet name > bus/dpaa: improve log macro and fix bus detection > doc: update release notes with NXP DPAA changes > > Jun Yang (14): > bus/dpaa: refine fman naming and fix global scope > bus/dpaa: scan max BPID from DTS > drivers: shutdown DPAA FQ by fq descriptor > bus/dpaa: improve FQ shutdown with channel validation > bus/dpaa: add DPAA cgrid cleanup support > drivers: add BMI Tx statistics > net/dpaa: optimize FMC MAC type parsing > drivers: release DPAA bpid on driver destructor > bus/dpaa: orp queue create and burst enqueue > net/dpaa: support fmcless rxq number as devargs > bus/dpaa: optimize DPAA multi-entry buffer pool operations > drivers: improve shutdown fq with channel > drivers: improve crypto fq resource handling > drivers: add dpaax enter destructor to gate EAL memory frees > > Prashant Gupta (1): > drivers: add process-type guards for secondary process > > Vanshika Shukla (1): > net/dpaa: enhance VSP port support > > doc/guides/dmadevs/dpaa.rst | 29 +- > doc/guides/nics/dpaa.rst | 39 +- > doc/guides/rel_notes/release_26_11.rst | 29 + > drivers/bus/dpaa/base/fman/fman.c | 23 +- > drivers/bus/dpaa/base/fman/fman_hw.c | 111 ++-- > drivers/bus/dpaa/base/qbman/bman.c | 59 +- > drivers/bus/dpaa/base/qbman/bman_driver.c | 51 +- > drivers/bus/dpaa/base/qbman/qman.c | 207 +++++-- > drivers/bus/dpaa/base/qbman/qman.h | 23 +- > drivers/bus/dpaa/base/qbman/qman_driver.c | 29 +- > drivers/bus/dpaa/bus_dpaa_driver.h | 11 +- > drivers/bus/dpaa/dpaa_bus.c | 164 +++-- > drivers/bus/dpaa/dpaa_bus_base_symbols.c | 4 + > drivers/bus/dpaa/include/fman.h | 20 +- > drivers/bus/dpaa/include/fsl_bman.h | 49 +- > drivers/bus/dpaa/include/fsl_qman.h | 54 +- > drivers/common/dpaax/compat.h | 22 +- > drivers/common/dpaax/dpaax_iova_table.c | 25 +- > drivers/crypto/dpaa_sec/dpaa_sec.c | 109 +++- > drivers/dma/dpaa/dpaa_qdma.c | 110 +++- > drivers/mempool/dpaa/dpaa_mempool.c | 64 +- > drivers/mempool/dpaa/dpaa_mempool.h | 3 +- > drivers/net/dpaa/dpaa_ethdev.c | 310 ++++++++-- > drivers/net/dpaa/dpaa_ethdev.h | 25 +- > drivers/net/dpaa/dpaa_flow.c | 221 +++++-- > drivers/net/dpaa/dpaa_flow.h | 7 +- > drivers/net/dpaa/dpaa_fmc.c | 73 ++- > drivers/net/dpaa/dpaa_oldev.c | 705 ++++++++++++++++++++++ > drivers/net/dpaa/fmlib/fm_lib.c | 32 +- > drivers/net/dpaa/fmlib/fm_port_ext.h | 4 +- > drivers/net/dpaa/meson.build | 11 +- > drivers/net/dpaa/rte_pmd_dpaa.h | 23 +- > drivers/net/dpaa/rte_pmd_dpaa_oldev.h | 93 +++ > 33 files changed, 2282 insertions(+), 457 deletions(-) > create mode 100644 drivers/net/dpaa/dpaa_oldev.c > create mode 100644 drivers/net/dpaa/rte_pmd_dpaa_oldev.h > Ran this through Claude Opus 5 with current AGENTS.md Errors Patch 21/25: net/dpaa: enhance VSP port support Two NULL derefs from the vsp_bpid[] -> vsp[].vsp_bp[] conversion. vsp_bp[] is an array of "struct dpaa_bp_info *", and dev_private is rte_zmalloc'd, so the pointers are NULL until a queue is set up. 1. dpaa_eth_rx_queue_setup(), non-VSP branch: dpaa_intf->vsp[0].vsp_bp[0]->bpid = DPAA_MEMPOOL_TO_POOL_INFO(mp)->bpid; Nothing ever assigns vsp[0].vsp_bp[0], so this writes through NULL on the first rte_eth_rx_queue_setup() of any plain (num_profiles == 0) port. The old line was a plain array store, "vsp_bpid[0] = ...". The correct conversion is "vsp[0].vsp_bp[0] = DPAA_MEMPOOL_TO_POOL_INFO(mp);". 2. dpaa_eth_rx_queue_bp_check() reads "dpaa_intf->vsp[vsp_id].vsp_bp[0]->bpid" at line 1138. It is called from rx_queue_setup at line 1187, before the vsp_bp[0] assignment at ~1230, so the first Rx queue setup on a VSP port dereferences NULL too. The old code read vsp_bpid[vsp_id], a uint32_t that is simply 0 when unset. Array bound removed. dpaa_port_vsp_configure() lost if (vsp_id >= DPAA_VSP_PROFILE_MAX_NUM) return -1; and dpaa_port_vsp_update() replaced it with if (vsp_id >= (fif->base_profile_id + fif->num_profiles)) but dpaa_intf->vsp[] is sized DPAA_VSP_PROFILE_MAX_NUM (8), and base_profile_id + num_profiles comes from the DTS with no relation to that. The check that bounded the array is gone. rx_queue_setup also indexes vsp[fif->base_profile_id] unbounded. Keep the DPAA_VSP_PROFILE_MAX_NUM check in addition to the new one. The commit message's lead claim -- "Add fman_onic MAC type handling to get_rx_port_type()" -- is not in this patch. get_rx_port_type() is untouched. Patch 15/25: bus/dpaa: orp queue create and burst enqueue Missing store barrier before the verb write in qman_enqueue_multi_orp(). The loop is: eq->fqid = ...; eq->fd.* = ...; eq->orp = ...; eq->seqnum = ...; eq->__dont_write_directly__verb = eq_verbs; /* publishes */ dcbf(eq); lwsync(); The verb byte is what makes the EQCR entry valid to QMan, and the lwsync() lands after it. On arm64/ppc the verb store can become visible before the FD stores. qman_enqueue_multi() in the same file gets this right: it writes all FDs, then lwsync(), then the verbs in a second pass, then dcbf in a third. Follow that shape. Also: qman_enqueue_multi_orp() has no callers, and force_ooo is only ever set false in qman_create_fq() and read in two places -- nothing sets it true. The whole patch is unreachable. Either land it with its consumer or hold it. Patch 22/25: drivers: add offline (O/H) port device support dpaa_create_device_list() returns before creating the OL device: dpaa_bus.device_count = 0; if (!dpaa_netcfg && dpaa_bus.oldev_enabled) return 0; /* Creating OL Device */ if (dpaa_bus.oldev_enabled) { ... } rte_dpaa_bus_dev_build() was changed in this same patch to tolerate a NULL netcfg when oldev_enabled, so the configuration the feature exists for creates no devices at all. Move the early return below the OL block, or drop it. rte_dpaa_remove() in dpaa_oldev.c is "return 0" -- no rte_eth_dev_release_port(), no dev_private free, no rx/tx queue free, no FQ shutdown. dpaa_ol_dev_close() is likewise a no-op. The port and all rte_zmalloc'd memory leak, and the FQs stay live in hardware. dpaa_oldev_init() sets nb_rx_queues = nb_tx_queues = DPAA_DEFAULT_NUM_PCD_QUEUES but only initializes index 0 (dpaa_ol_rx_queue_init(&rx_queues[0], 0), same for tx). dev_info advertises the full count, so an app configuring queue 1 gets a zeroed qman_fq with fqid 0, and tx_queue_setup passes "fq_info.tx_fq_id = 0" to the kernel ioctl. RTE_EXPORT_EXPERIMENTAL_SYMBOL(..., 25.11) on all four rte_pmd_dpaa_ol_* functions. The series targets 26.11 -- patch 14 uses 26.11 correctly. rte_pmd_dpaa_oldev.h is installed as public API (added to headers in meson.build) and violates the namespace rules throughout: MAX_NUM_PORTS, MAX_NUM_SUBNETS, struct ip_addr_s, struct ip_pair_s, struct lgw_subnet_s, DPA_ISC_*, and DPDK_CLASSIF_* / DPDK_TELECOM_*. MAX_NUM_PORTS and "struct ip_addr_s" in a header an application includes will collide. There is also no Doxygen anywhere in the file -- no @file, no @param/@return on any of the four functions, and no experimental warning block. Patch 14/25: net/dpaa: add Tx rate limiting API rte_pmd_dpaa_port_set_rate_limit() applies the limit to the Rx port handle when one exists. dpaa_intf->port_handle is set by set_fm_port_handle() using get_rx_port_type(); only the "!dpaa_intf->port_handle" branch opens a Tx port via get_tx_port_type(). So in FMC-less mode with PCD configured, a *Tx* rate-limit API programs the Rx port. Always open the Tx port. No driver check. The function does RTE_ETH_VALID_PORTID_OR_ERR_RET() and then casts dev_private to struct dpaa_if * and process_private to struct fman_if *. Called with a valid non-DPAA port id it prints dpaa_intf->name and reads fif->mac_type / fif->mac_idx from a type-confused pointer. The existing rte_pmd_dpaa_set_tx_loopback() in the same driver guards with is_dpaa_supported(dev); do the same. Patch 20/25: drivers: improve shutdown fq with channel CGR use-after-free on the dev_init error paths. qman_create_cgr() ends with "list_add(&cgr->node, &p->cgr_cbs)", so every created CGR is linked into the portal list. The free_rx label does rte_free(dpaa_intf->cgr_rx); rte_free(dpaa_intf->cgr_tx); with no qman_delete_cgr() and no qman_release_cgrid_range(), leaving freed memory on p->cgr_cbs and leaking the CGRIDs. This predates the series, but this patch is the one that adds qman_release_cgrid_range() to the success path, so the error path should be fixed in the same commit. Warnings Patch 12/25: dma/dpaa: add SG data validation and ERR050757 The doc for dpaa_dma_pci_read_disable describes something the code does not do: "The workaround inserts a PCI read-back after each DMA write to flush posted writes." The code sets srttype = FSL_QDMA_CMD_RWTTYPE and the source stride (ssen/sss/ssd) to bound read transaction length. Nothing to do with write flushing. The commit message says the patch "adds scatter-gather support" and "adds a workaround for ERR050757". Both already existed; the patch adds devargs to *disable* them. s_sg_enable / s_data_validation / s_pci_read are file-static but set per-device in dpaa_qdma_init(), so with more than one qdma device the last probe wins for all of them. They are also int where s_hw_err_check next to them is bool. In the new !s_sg_enable loop, a mid-burst fsl_qdma_enqueue_desc_single() failure returns a hardcoded -EIO, discarding the real code. The pending_num == 1 path above it propagates ret properly; a ring-full is transient and shouldn't be reported as -EIO. Patch 13/25: net/dpaa: support Rx/Tx taildrop threshold devarg RTE_PMD_REGISTER_PARAM_STRING has no separators between the keys: DRIVER_RECV_ERR_PKTS "=<int>" DRIVER_RX_TAILDROP "=<int>" DRIVER_TX_TAILDROP "=<int>" concatenates to "recv_err_pkts=<int>drv_rx_taildrop=<int>...". Patch 12 gets this right with trailing spaces; patches 16 and 17 add to the same broken string. td_threshold / td_tx_threshold are file-static and never reset. They are consumed within the same dpaa_dev_init() call so a port's own devarg works, but a port probed *without* the devarg inherits whatever the previously probed port set, rather than the CGR_RX_PERFQ_THRESH default. The DPAA_TX_TAILDROP_THRESHOLD getenv fallback is retained now that a devarg exists. Worth a deprecation note. Patch 16/25: net/dpaa: support fmcless rxq number as devargs num_rx_fqs = (int)fmcless_rxq with only an upper clamp. drv_fmcless_rxq=0 or a negative value flows into dpaa_intf->nb_rx_queues (uint16_t), so -1 becomes 65535 in dev_info.max_rx_queues. Reject anything < 1. The FMCLESS default changes from rte_lcore_count() to DPAA_MAX_NUM_PCD_QUEUES (8). That is user-visible and belongs in the release notes. Patch 18/25: bus/dpaa: optimize DPAA multi-entry buffer pool operations BIT_SIZE, MAX_U48, HI16_OF_U48, LO32_OF_U48 and U48_BY_HI16_LO32 move from bman.c file scope into fsl_bman.h, which net/dpaa, mempool/dpaa, crypto/dpaa_sec and dma/dpaa all include. Only BIT_SIZE is #ifndef-guarded. These need a dpaa_/bm_ prefix. U48_BY_HI16_LO32(hi, lo) does "(hi) << 32". It is safe at the one call site because hi is a local uint64_t, but as a header macro any caller passing a 32-bit hi gets undefined behaviour. bman_hw_extract_addr() was a static inline with type checking; it is replaced by bm_buffer_get64_from_be(), a statement-expression macro. That is a step backwards -- keep it an inline function. Commit message says the patch uses "a single bm_hw_buf_desc structure for HW initialization of the first entry and copy it to remaining entries." The code sets each entry's address individually. Patch 23/25: drivers: improve crypto fq resource handling RTE_DPAA_MAX_RX_QUEUE is MAX_DPAA_CORES * 1024 = 4096, so "uint32_t fqids[RTE_DPAA_MAX_RX_QUEUE]" is a 16 KB stack frame in dpaa_sec_dev_init(). Consider rte_malloc or splitting the two allocations. Switching the Rx FQs from QMAN_FQ_FLAG_DYNAMIC_FQID to a single qman_alloc_fqid_range(fqids, 4096, 0, 0) now requires 4096 *contiguous* FQIDs at probe (8192 with two SEC devices). That is a much stronger constraint than per-FQ dynamic allocation and can fail on a fragmented allocator. It's a real boot-time win, but the contiguity requirement should be called out. dpaa_sec_uninit() calls qman_shutdown_fq() for all max_nb_queue_pairs and RTE_DPAA_MAX_RX_QUEUE entries regardless of whether dev_init got that far, so a partially-initialized device shuts down FQs with fqid 0. Patch 24/25: drivers: add dpaax enter destructor to gate EAL memory frees kfree() becomes a statement expression "({ if ... else ...; })". Use do { } while (0) -- it is the idiom and avoids compilers that warn on a void-valued statement expression. is_dpaax_in_destructor() is an installed __rte_internal symbol with a generic is_* name, unlike its dpaax_enter_destructor() sibling. Rename to dpaax_is_in_destructor(). Both are declared in compat.h, a Linux-compat shim; dpaax_iova_table.h is the right home. The gate is process-global and one-way, so whichever destructor runs first (dpaa_cleanup 102, dpaa_finish 103, dpaa_mpool_finish 104) disables kfree() for all the rest. That is presumably intended, but it makes the RTE_FINI_PRIO ordering load-bearing and converts a use-after-free into a deliberate exit-time leak. Say so in the commit message. Patch 25/25: doc: update release notes with NXP DPAA changes Missing entries for user-visible additions in this series: - the four experimental rte_pmd_dpaa_ol_* functions and the new installed header rte_pmd_dpaa_oldev.h (patch 22) -- new public API needs a release note - drv_sh_if_name (patch 17) and drv_bh_port (patch 22) - dpaa_dma_sg_disable / dpaa_dma_data_validation / dpaa_dma_pci_read_disable (patch 12) - the four new BMI Tx xstats (patch 08) - the FMCLESS Rx queue count default change (patch 16) The commit message lists qman_enqueue_multi_orp as a documented feature; it is __rte_internal and correctly absent from the .rst. Drop it from the message. Info Patch 17/25: dpaa_get_devargs_str() is forward-declared at the top of dpaa_ethdev.c rather than defined before use; moving the definition would be cleaner. The devarg is re-parsed on every dev_configure(). Patch 19/25: the subject says "and fix bus detection" but the body and the diff are only the log-macro conversion plus moving DPAA_DEV_PATH1/2 to the top of the file -- where they now sit under the "we allow up to 4 push mode queues" comment, which belongs to DPAA_MAX_PUSH_MODE_QUEUE. Seven DPAA_BUS_LOG() call sites remain (dpaa_bus.c 695/754/773, netcfg_layer.c 98/109/118/134). Two continuation lines were left at their old alignment. Patch 22/25: dpaa_oldev.c defines its own static rte_dpaa_probe() and rte_dpaa_remove(), duplicating the names in dpaa_ethdev.c and using the rte_ prefix on static functions. Both are marked __rte_unused on dpaa_dev, which is used. dpaa_poll_queue_default_config() is copied verbatim from dpaa_ethdev.c. assert() is used without including <assert.h>. DPAA_OL_* and DPA_ISC_WQ_ID are #defined mid-function. fq_info is passed to an ioctl without a memset, so padding bytes are uninitialized stack. "rxq == NULL" after "rxq = &dpaa_intf->rx_queues[queue_idx]" is dead -- test the array pointer instead. The fd opened by check_fd() is never closed. Patch 10/25: DPAA_PMD_INFO("MAC index of %s is %d") fires once per port entry per interface during FMC parse; DEBUG is the right level. Consecutive blank lines: dpaa_oldev.c:114, rte_pmd_dpaa_oldev.h:8 and :47, dpaax_iova_table.c:37, dpaa_bus.c:739, dpaa_ethdev.c:2602. No lines over 100 chars and no trailing whitespace in the series. check-git-log flagged the Fixes: tag in patch 01, but that is an artifact of my shallow clone -- the format is correct.