Re: [PATCH v12 00/26] NXP DPAA driver fixes and enhancements

On 18-08-2026 19:57, Stephen Hemminger wrote:
> On Tue, 18 Aug 2026 16:47:04 +0530
> Hemant Agrawal<[email protected]> wrote:
>
>> This series contains a collection of fixes and enhancements for the NXP
>> DPAA bus, mempool, dma, crypto and net drivers. It addresses several
>> resource cleanup and shutdown issues, adds new offline (O/H) port device
>> support, and introduces a number of performance and usability
>> improvements.
>>
>> Highlights:
>> - Fix device remove, Tx confirmation queue leak and FQD dest wq decoding.
>> - Add process-type guards for secondary process and improve FQ shutdown
>>    and cgrid cleanup handling.
>> - Add offline (O/H) port device support with a new PMD-specific API.
>> - Add Tx rate limiting API, Rx/Tx taildrop threshold devarg, fmcless rxq
>>    number devarg and non fmX-macY shared Ethernet name support.
>> - Optimize FMAN deconfig, FMC MAC type parsing and buffer pool
>>    operations.
>>
>> v11 -> v12:
>> - fixed recent AI review comments
>
> There are still more AI review comments to address here:
>
> Reviewed the v12 series (26 patches) applied on c1a46b9.  Series
> applies cleanly with git am.  This is a source review of the applied
> tree; I did not build it this time.
>
> All the blocking items from v11 are addressed.  Thanks for splitting
> the FQD decoding fix out into its own patch.  What is left is mostly
> commit messages that describe something other than what the patch
> does, plus the offline port driver and the destructor patch.
>
> Fixed since v11
> ---------------
>
>    - FQD dest_wq decoding is now patch 03 with a Fixes: tag and
>      Cc:[email protected].  The wq/channel split is correct.
>    - BMI Tx counters are enabled and reset via fmbm_tstc, so the four
>      new tx_* xstats will actually count (patch 10).
>    - rte_pmd_dpaa_port_set_rate_limit() guards with is_dpaa_supported()
>      and always opens the Tx port (patch 16).
>    - dev_init error paths delete every created CGR before freeing
>      cgr_rx/cgr_tx and release the CGRID range, tracked by nb_rx_cgr /
>      nb_tx_cgr and the *_cgrid_allocated flags (patch 08).
>    - qman_enqueue_multi_orp() now writes all frame data, then lwsync(),
>      then the verb bytes, then dcbf, matching qman_enqueue_multi()
>      (patch 17).
>    - dpaa_eth_rx_queue_bp_check() checks vsp_bp[0] before dereferencing
>      it, and dpaa_port_vsp_configure() has the DPAA_VSP_PROFILE_MAX_NUM
>      bound back (patch 22).
>    - dpaa_create_device_list() no longer returns before creating the OL
>      device, and dpaa_ol_remove() releases queues, closes the fd and
>      releases the port (patch 23).
>    - rte_pmd_dpaa_oldev.h has Doxygen throughout and the two API structs
>      are prefixed (patch 23).
>
> Errors
> ------
>
> Patch 23/26: drivers: add offline (O/H) port device support
>
>    All four OL exports still carry 25.11:
>
>        RTE_EXPORT_EXPERIMENTAL_SYMBOL(
>                rte_pmd_dpaa_ol_set_classif_info, 25.11)
>        RTE_EXPORT_EXPERIMENTAL_SYMBOL(
>                rte_pmd_dpaa_ol_reset_classif_info, 25.11)
>        RTE_EXPORT_EXPERIMENTAL_SYMBOL(
>                rte_pmd_dpaa_ol_set_lgw_info, 25.11)
>        RTE_EXPORT_EXPERIMENTAL_SYMBOL(
>                rte_pmd_dpaa_ol_reset_lgw_info, 25.11)
>
>    The series targets 26.11; patch 26 adds to release_26_11.rst and
>    patch 16 uses 26.11 correctly.
<fixed it in v13>
>    Only queue index 0 is ever initialized.  dpaa_oldev_init() sets
>
>        /* num_fqs is DPAA_DEFAULT_NUM_PCD_QUEUES */
>        dpaa_intf->nb_rx_queues = num_fqs;
>        dpaa_intf->nb_tx_queues = num_fqs;
>
>    but calls dpaa_ol_rx_queue_init(&rx_queues[0], 0) and
>    dpaa_ol_tx_queue_init(&tx_queues[0], 0) only.  dpaa_ol_dev_info()
>    reports the full count in max_rx_queues/max_tx_queues, so an
>    application that configures queue 1 gets a zeroed qman_fq, and
>    dpaa_ol_tx_queue_setup() passes fq_info.tx_fq_id = 0 to the kernel
>    ioctl.  Either initialize all num_fqs queues or advertise 1.
<this is false positive;  dpaa_ol_dev_info based 
max_rx_queues/max_tx_queues are being assigned from 
dpaa_intf->nb_rx_queues and nb_tx_queues only; so oldev will only report 
max queue as 1>
>
>    dpaa_ol_dev_close() is still a bare "return 0".  All the cleanup
>    lives in dpaa_ol_remove(), so an application that calls
>    rte_eth_dev_close() without removing the device leaves the FQs live
>    in hardware and the rx/tx queue allocations in place.
<fixed it in v13>
>
> Warnings
> --------
>
> Patch 02/26: net/dpaa: fix Tx confirmation queue memory leak
>
>    The free is placed after the early return for offline ports:
>
>        if (fif->mac_type == fman_offline_internal ||
>            fif->mac_type == fman_onic)
>                return 0;
>        ...
>        rte_free(dpaa_intf->tx_conf_queues);
>
>    so for those two MAC types the leak the commit message describes is
>    still there, along with rx_queues, tx_queues, cgr_rx, cgr_tx and
>    fc_conf.  The early return predates the series, but patch 23 makes
>    offline ports a lot more interesting, so it is worth fixing here or
>    in a follow-up.
<fixed it in v13>
> Patch 08/26: drivers: add DPAA cgrid cleanup support
>
>    qman_pending_fq_by_cgrid() walks the FQID space from 1 upward, one
>    qman_query_fq_np() per FQID plus a qman_query_fq() for every FQ not
>    in OOS state, and stops only when QMan answers -ERANGE.  It is called
>    from dpaa_cgr_stale_fq_cleanup() once per CGR, and that in turn is
>    called once per Rx queue and once per Tx queue in
>    dpaa_eth_dev_close().  Worse, the caller loops:
>
>        while (qman_pending_fq_by_cgrid(cgrid, &fqid) == 0)
>
>    and each call restarts the scan at fqid = 1, so N stale FQs cost N
>    full scans.
>
>    The cgrd.i_bcnt idle check keeps this off the clean-shutdown path,
>    which is the common case, but the situation this code exists for is
>    precisely the one where the CGR is not idle.  Consider passing a
>    start FQID into the function so the caller can resume the scan, or
>    querying the congestion state once and walking only the FQs this
>    process knows about.
<fixed it in v13>
> Patch 13/26: drivers: release DPAA bpid on driver destructor
>
>    rte_dpaa_bpid_info is freed once no pool holds it:
>
>        for (i = 0; i < DPAA_MAX_BPOOLS; i++)
>                if (rte_dpaa_bpid_info[i].mp)
>                        break;
>        if (i == DPAA_MAX_BPOOLS) {
>                rte_free(rte_dpaa_bpid_info);
>                rte_dpaa_bpid_info = NULL;
>        }
>
>    but every Rx queue still holds a copy of that pointer in
>    fq->bp_array (dpaa_ethdev.c and dpaa_oldev.c both do
>    "rxq->bp_array = rte_dpaa_bpid_info"), and dpaa_eth_queue_rx() /
>    dpaa_rx_cb() reinstall it:
>
>        if (unlikely(rte_dpaa_bpid_info == NULL &&
>                     rte_eal_process_type() == RTE_PROC_SECONDARY))
>                rte_dpaa_bpid_info = fq->bp_array;
>
>    The array is rte_zmalloc'd, so it is shared.  A primary that frees
>    its last mempool while a secondary still has ports open leaves the
>    secondary reinstalling a dangling pointer on the fast path.  Freeing
>    it only from the destructor, or refcounting against open ports rather
>    than against live mempools, would avoid this.
>
>    The "if (!rte_dpaa_bpid_info) return;" guard is below the block that
>    already dereferenced rte_dpaa_bpid_info, so it does not protect
>    anything.
<fixed it in v13>
>
> Patch 14/26: dma/dpaa: add SG data validation and ERR050757
>
>    The commit message describes work the patch does not do.  SG support
>    and the ERR050757 workaround both already exist upstream;
>    fsl_qdma_enqueue_desc_sg() and the sdf->ssen/sss/ssd programming are
>    pre-existing.  What this patch actually adds is three devargs to turn
>    those off, plus a gate on the existing data-validation helper.  The
>    body should say that.
>
>    s_sg_enable, s_data_validation and s_pci_read are file-scope statics
>    written from per-device devargs in dpaa_qdma_init().  With more than
>    one QDMA device the last one probed silently sets the behaviour for
>    all of them.  These belong in struct fsl_qdma_engine.  (s_hw_err_check
>    has the same problem already, so this is three more of the same.)
>
>    s_data_validation, s_sg_enable and s_pci_read are int used purely as
>    booleans; s_hw_err_check next to them is already bool.
<fixed it in v13>
> Patch 15/26: net/dpaa: support Rx/Tx taildrop threshold devarg
>
>    The getenv is kept as a fallback:
>
>        if (dpaa_get_devargs_int(dev->devargs, DRIVER_TX_TAILDROP, ...)
>                ...
>        else if (getenv("DPAA_TX_TAILDROP_THRESHOLD"))
>
>    The point of the patch is to replace the environment variable with a
>    devarg.  Drop the getenv in the same patch, and note the removal in
>    the release notes if you are worried about existing users.
>
>    The new "Device Arguments" section in doc/guides/nics/dpaa.rst is a
>    bullet list where each item is a term followed by a description
>    paragraph.  A definition list reads better and produces better
>    output:
>
>        ``drv_rx_taildrop`` / ``drv_tx_taildrop``
>           Configure the Rx / Tx frame queue taildrop congestion
>           threshold.  A value of ``0`` disables taildrop.
> Patch 17/26: bus/dpaa: orp queue create and burst enqueue
>
>    Still no consumer.  qman_enqueue_multi_orp() has no caller in the
>    tree, force_ooo is only ever set false in qman_create_fq(), and the
>    ORP_RWS_WIN_*, ORP_AUTO_ADVANCE_* and ORP_LATE_ARRIVE_* enums added
>    to fsl_qman.h are unreferenced.  The subject says "orp queue create"
>    but there is no create path in the diff.  Please land this with the
>    code that uses it, or hold it until that code is ready.
NXP customer applications use it.
> Patch 20/26: bus/dpaa: optimize DPAA multi-entry buffer pool operations
>
>    The commit message says the first descriptor is initialized and
>    copied to the remaining entries.  The code does the reverse: entry 0
>    is written on its own precisely because copying it would trip the
>    valid bit, and entries 1..n-1 are the ones memcpy'd.
>
>        r->bufs[0].opaque = bm_bufs[0].opaque;
>        if (num > 1)
>                memcpy(&r->bufs[1], &bm_bufs[1],
>                       sizeof(struct bm_buffer) * (num - 1));
>
>    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, where they are now
>    visible to everything that includes it, with no BM_ or RTE_ prefix.
>    BIT_SIZE in particular is generic and is wrapped in #ifndef, so it
>    will silently pick up somebody else's definition rather than warn.
>
>    U48_BY_HI16_LO32() shifts hi left by 32 with no cast:
>
>        #define U48_BY_HI16_LO32(hi, lo) \
>                (((hi) << BIT_SIZE(uint32_t)) | (lo))
>
>    The one caller declares hi as uint64_t so it is fine today, but as a
>    header macro it will be undefined behaviour the first time somebody
>    passes a 32-bit value.  Cast inside the macro.
fixed
> Patch 21/26: bus/dpaa: improve log macro and fix bus detection
>
>    The subject promises a bus detection fix that is not in the patch.
>    Both access(DPAA_DEV_PATH1/2) checks exist upstream unchanged, and
>    DPAA_DEV_PATH1/DPAA_DEV_PATH2 were already defined ahead of both.
>    The only change is moving the two #defines to the top of the file.
>    The commit body mentions only the log macro conversion, which is
>    what the patch does.
>
>    The conversion is also incomplete: 12 DPAA_BUS_LOG() call sites
>    remain, including two in the function the patch edits
>    (rte_dpaa_bus_scan) and three in netcfg_layer.c.
fixed
> Patch 23/26: drivers: add offline (O/H) port device support
>
>    rte_pmd_dpaa_oldev.h is installed as public API (it is in the
>    headers list in meson.build) and still puts unprefixed names in the
>    application namespace:
>
>        MAX_NUM_PORTS, MAX_NUM_SUBNETS
>        struct ip_pair_s, struct lgw_subnet_s
>        DPA_ISC_IPV4_ADDR_TYPE, DPA_ISC_IPV6_ADDR_TYPE
>        DPA_ISC_IPV4_SUBNET_TYPE, DPA_ISC_IPV6_SUBNET_TYPE
>        DPDK_CLASSIF_*, DPDK_TELECOM_*
>
>    MAX_NUM_PORTS is the one that will actually bite; plenty of
>    applications define that themselves.  The DPDK_ prefix is worse than
>    none since it implies these are project-wide.  Please give everything
>    in this header the rte_pmd_dpaa_ / RTE_PMD_DPAA_ prefix you already
>    used for the two structs.
>
>    struct dpaa_ip4_addr_s holds uint32_t ip_addr[4] and is used for v6
>    addresses too, so the ip4 in the name is misleading.
fixed
>
>