Re: [PATCH v14 00/23] NXP DPAA driver enhancements and fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 20 Aug 2026 20:12:49 +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.
> 
> v14:
> * Dropped the ORP queue create and destructor patch.

Still seeing some issues like double free and unitialized data showing
up on AI review.

Reviewed the v14 series applied on c1a46b9.  All 23 patches apply
cleanly with git am.

Resolved since v13
------------------

Thanks for taking the two structural items off the table:

  - The ORP patch is dropped, so the __rte_internal reachability
    question goes with it.
  - The dpaax enter-destructor patch is dropped, which resolves the
    latch-versus-rte_free() contradiction I raised against v13.

Also confirmed fixed in the applied tree:

  - U48_BY_HI16_LO32() now casts hi to uint64_t before the shift.
  - struct ip_pair_s and struct lgw_subnet_s are prefixed as
    dpa_ip_pair_s and dpa_lgw_subnet_s.
  - The release note for the DMA devargs now describes them as
    controlling existing behaviour ("instead of the default
    scatter-gather batching") rather than claiming SG was added.
  - The devargs section of doc/guides/nics/dpaa.rst is now a proper
    definition list.
  - The offline-port control fd is closed and reset to -1 in
    dpaa_ol_dev_close().

The three Fixes: tags (patches 01, 02, 03) all resolve to real
commits with matching subjects, use 12-character hashes, and carry
Cc: stable@dpdk.org.

Errors
------

Patch 21/23: drivers: add offline (O/H) port device support

  dpaa_ol_remove() double-frees the ethdev private data:

      dpaa_ol_dev_close(eth_dev);
      rte_free(eth_dev->data->dev_private);
      return rte_eth_dev_release_port(eth_dev);

  rte_eth_dev_release_port() already does
  rte_free(eth_dev->data->dev_private) in the primary process (see
  lib/ethdev/ethdev_driver.c), so the pointer is freed twice.  The
  driver does not NULL it in between.

  rte_dpaa_remove() in dpaa_ethdev.c gets this right -- it calls
  rte_eth_dev_release_port() and nothing else -- so the OL path is
  the outlier.  Dropping the rte_free() line is the whole fix.

  Related: unlike rte_dpaa_remove(), which patch 06 gave a
  RTE_PROC_PRIMARY guard, dpaa_ol_remove() has none.  In a secondary
  the explicit rte_free() would also be operating on memory the
  primary owns.

Patch 18/23: bus/dpaa: optimize DPAA multi-entry buffer pool operations

  bman_release_fast() writes uninitialized stack bytes into the BMan
  release ring.

      struct bm_buffer bm_bufs[FSL_BM_BURST_MAX];   /* no initializer */
      ...
      bm_bufs[0].be_desc.bpid = bpid;
      for (i = 0; i < num; i++)
              bm_buffer_set64_to_be(&bm_bufs[i], bufs[i]);

      r->bufs[0].opaque = bm_bufs[0].opaque;
      if (num > 1)
              memcpy(&r->bufs[1], &bm_bufs[1],
                      sizeof(struct bm_buffer) * (num - 1));

  bm_buffer_set64_to_be() only writes be_desc.hi and be_desc.lo.  Of
  the four fields in struct bm_hw_buf_desc:

      be_desc.rsv    never written for any entry
      be_desc.bpid   written for entry 0 only

  Entry 0 reaches the ring through the .opaque read, which picks up
  the uninitialized rsv byte.  Entries 1..num-1 are memcpy'd whole,
  so they carry both uninitialized rsv and uninitialized bpid.

  The function this replaces does not have the problem, because it
  masks the top 16 bits off explicitly:

      r->bufs[i].opaque = cpu_to_be64(bufs[i].opaque & MAX_U48);

  so this is a regression introduced by the rework, not a
  pre-existing issue.  BM_RCR_VERB_CMD_BPID_SINGLE means the garbage
  bpid on entries 1..n-1 is probably ignored by the hardware, but
  rsv is a reserved field and this is stack content being published
  to a device-visible ring on every mbuf free.

  Zeroing the array at declaration is the cheap fix; having
  bm_buffer_set64_to_be() clear .opaque before writing hi/lo would
  also work and keeps the cost on the entries actually used.

Warnings
--------

Patch 10/23: drivers: add BMI Tx statistics

  fmbm_tfrc, the Tx Frame Counter, is declared but never wired up.
  It is present in struct tx_bmi_regs, it is the register the Tx
  stat window is anchored on --

      #define FMAN_IF_BMI_TX_STAT_OFFSET_START \
              BMI_TX_REG_OFFSET(fmbm_tfrc)

  -- and it is mirrored into struct dpaa_if_tx_bmi_stats, but
  fman_if_bmi_stats_get_all() does not read it,
  fman_if_bmi_stats_reset() does not reset it, and it has no entry
  in dpaa_xstats_strings[].

  That omission is what the "- 1" in the static_assert is absorbing:

      static_assert(sizeof(struct dpaa_if_rx_bmi_stats)
                              / sizeof(uint32_t)
                      + sizeof(struct dpaa_if_tx_bmi_stats)
                              / sizeof(uint32_t) - 1
                      == DPAA_BMI_XSTATS_COUNT, ...);

  8 + 5 - 1 == 12.  The assert is a good idea, but as written it
  silently tolerates exactly one unexposed struct member, so it will
  not catch the next one.  Either expose fmbm_tfrc (it is the most
  useful of the four Tx counters) and drop the "- 1", or drop the
  member from the struct.  Carrying it in the struct while hiding it
  from the assert is the worst of the three.

Patch 20/23: net/dpaa: enhance VSP port support

  Two fields added by this patch are never read or written anywhere
  in the tree:

      struct dpaa_if_vsp {
              ...
              uint32_t max_size;      /* no users */
      };

      struct dpaa_if {
              ...
              uint8_t base_vsp;       /* no users */
      };

  bp_num and vsp_bp[] are used; these two are not.  Worth dropping
  until something needs them.

Patch 06/23: drivers: add process-type guards for secondary process

  The dma/dpaa guard fixes a real problem but leaves the secondary
  in a state that will still fault.

  Returning early from dpaa_qdma_init() does stop the secondary from
  re-running the mmap and re-allocating queue resources on top of
  the shared struct fsl_qdma, which is a genuine improvement.  But
  dpaa_qdma_probe() has already installed dev_ops and every
  fp_obj->* handler before calling init, and it goes on to set
  state = RTE_DMA_DEV_READY afterwards.

  fsl_qdma lives in the shared hugepage allocation, so the secondary
  sees the primary's values -- including reg_base, which comes from

      fsl_qdma->reg_base = mmap(NULL, regs_size, ...);

  a process-private mapping only valid in the primary.  So a
  secondary that calls rte_dma_copy() on the device gets a READY
  device whose MMIO pointers are not mapped in its address space.

  Either the secondary needs its own mapping kept in process-private
  storage, or probe should not advertise the device as READY for a
  secondary.

Patch 21/23: drivers: add offline (O/H) port device support

  check_fd() is double-checked locking on a non-atomic int:

      static int fd = -1;

      static int check_fd(void)
      {
              if (fd >= 0)              /* unsynchronized read */
                      return 0;
              ret = pthread_mutex_lock(&fd_init_lock);
              assert(!ret);
              if (fd < 0)
                      fd = open(ASK_PATH, O_RDWR);
              ret = pthread_mutex_unlock(&fd_init_lock);

  The fast-path read of fd races with the store under the lock.  Per
  the atomics section in AGENTS.md this wants
  rte_atomic_load_explicit(&fd, rte_memory_order_acquire) on the
  first check and rte_atomic_store_explicit(..., release) on the
  assignment, with fd declared as RTE_ATOMIC(int).

Info
----

Patch 15/23: net/dpaa: support Rx/Tx taildrop threshold devarg

  Carried over from v13 and still unanswered: now that
  drv_tx_taildrop exists, the DPAA_TX_TAILDROP_THRESHOLD getenv()
  fallback in dpaa_dev_init() is redundant.  It is pre-existing code
  so checkpatch will not flag it, but the patch touches that exact
  block, and the new RST documents only the devarg -- the env var is
  now an undocumented second way to set the same value.  Removing it
  here, or noting the deprecation in the release notes, would close
  it out.

Patch 18/23: bus/dpaa: optimize DPAA multi-entry buffer pool operations

  BIT_SIZE, MAX_U48, HI16_OF_U48 and LO32_OF_U48 are unprefixed in
  fsl_bman.h.  The header is not installed, so this is not an ABI
  concern, but bus/dpaa exports its include directory to net/dpaa,
  crypto/dpaa_sec, mempool/dpaa and dma/dpaa, and BIT_SIZE and
  MAX_U48 in particular are generic enough to collide there.  The
  #ifndef guard on BIT_SIZE suggests this already came up once.

Patch 21/23: drivers: add offline (O/H) port device support

  ask_get_channel_info() is declared uint32_t but returns -ENODEV
  and ioctl's -1:

      static uint32_t
      ask_get_channel_info(struct ask_ctrl_offline_channel *ch_info)

  It happens to work because the only caller assigns to an int and
  tests if (ret), but ask_set_fq_info() right below it is correctly
  static int.  Worth matching.

  DPA_ISC_IPV4_ADDR_TYPE and friends, and the struct tags
  dpa_ip_addr_s / dpa_ip_pair_s / dpa_lgw_subnet_s, are still
  outside the rte_ namespace in rte_pmd_dpaa_oldev.h, which is an
  installed header.  The DPDK_-prefixed macros in the same header
  are arguably worse -- DPDK_ reads as a framework namespace rather
  than a PMD one.  Not a blocker, but this is the header
  applications will include.

  check_fd() uses assert() from <assert.h>; RTE_ASSERT is the DPDK
  spelling and compiles out consistently with the rest of the tree.

Patch 10/23: drivers: add BMI Tx statistics

  The zero-fill fallbacks in fman_if_bmi_stats_get_all() use bare
  literals:

      while (i < 8)  value[i++] = 0;
      ...
      while (i < 12) value[i++] = 0;

  These have to stay in step with DPAA_BMI_XSTATS_COUNT on the
  consumer side, which is derived.  Deriving them the same way, or
  at least naming them, keeps the two ends from drifting.

Patch 06/23: drivers: add process-type guards for secondary process

  Commit message: "net/dpaa rivers" should be "drivers".

Patch 23/23: doc: update release notes with NXP DPAA changes

  The RST body was corrected, but the commit message bullet list
  still says "DMA scatter-gather support and ERR050757 workaround",
  which is the claim patch 14's message was reworded to drop.