Re: [RFC 00/11] Device unplug and bus cleanup refactoring for NXP

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 23 Jul 2026 15:53:48 +0200
David Marchand <david.marchand@redhat.com> wrote:

> Hello Hemant,
> 
> This is a followup to the refactoring started in 26.07.
> 
> I took some time with my best AI friend to cleanup DPAA and FSLMC bus
> drivers.
> 
> Like the last time, only compilation has been checked.
> I have no hardware to test runtime.
> 
> One thing that could be broken is either the order of devices
> initialisation, or bugs in the device filtering that I tried to
> simplify.
> 
> I think it is worth testing and fixing, as it will make the two NXP
> bus drivers similar to other bus drivers (but keep the special IO devices
> handling internal to the FSLMC bus for example).
> 

Detailed AI review spotted some things.

Review of [RFC 00/11] bus refactoring for NXP buses

Reviewed against DPDK main (c1a46b9), applied with git am, cross-built
with config/arm/arm64_dpaa_linux_gcc -Dwerror=true.

Patch 02/11: bus/dpaa: allocate interrupt during probing

Error: fd leak on the dpaa_setup_intr() failure path.

  ret = dpaa_setup_intr(dpaa_dev->intr_handle);
  if (ret != 0) {
          ...
          goto release_intr;
  }
  ...
          dpaa_close_intr(dpaa_dev->intr_handle);
  release_intr:
          rte_intr_instance_free(dpaa_dev->intr_handle);

dpaa_setup_intr() opens an eventfd and stores it with rte_intr_fd_set()
before calling rte_intr_type_set().  If rte_intr_type_set() fails, the fd
is already installed in the handle, but the goto lands past
dpaa_close_intr(), so the instance is freed with the fd still open.
Move the label above dpaa_close_intr(), or close the fd inside
dpaa_setup_intr() on its own error paths.

Warning: error propagation lost in dpaa_bus_cleanup().  The old code
returned -1 when drv->remove() failed; the rewritten loop does
"rte_errno = errno; goto next;" and the function unconditionally returns
0.  The local "ret" is also assigned and never read afterwards.  This is
transient (patch 03 replaces the body with rte_bus_generic_cleanup())
but each commit should stand on its own; track the failure in a variable
and return it.

Info: jumping into the middle of an if-block ("release_intr:" inside the
"if (ret != 0)" body) is legal but hard to follow.  A separate error
block would read better.

Patch 04/11: bus/fslmc: fix memory leaks in scan

Warning: fslmc_bus_remove_device() is described as fully freeing a
device, but dev->device.name is strdup()'d in scan_one_fslmc_device()
and never freed here or anywhere else.  Same gap in fslmc_free_device()
added by patch 11.  Since this patch is specifically about scan-time
leaks, freeing the name belongs here.

Patch 07/11: bus/fslmc: refactor device filtering for multiprocess

Error: allowlist mode is broken by moving the ignore test into scan.
scan_one_fslmc_device() now calls rte_bus_device_is_ignored(), which in
RTE_BUS_SCAN_ALLOWLIST mode returns true for every device lacking an
explicit RTE_DEV_ALLOWED devargs.  A single "-a fslmc:dpni.1" sets
rte_fslmc_bus.conf.scan_mode to ALLOWLIST (eal_common_devargs.c:349),
after which dpbp/dpcon/dpci/dprc/dpdmux/dprtc are dropped at scan time.
Those control objects are not probed by a driver; they are initialised
by fslmc_vfio_process_group(), so dropping them at scan leaves the bus
non-functional.  fslmc_filter_control_devices() applies the same test to
DPMCP and DPIO, so allowlist mode also fails "No MC Portal device found"
(-ENODEV).  The old code tested only devargs->policy == RTE_DEV_BLOCKED,
and the generic probe loop already skips ignored devices at probe time,
which is why allowlists worked before.

Error: the DPIO split drops the only DPIO on a primary process.  The old
code guarded the split with "dpio_count > 1":

  if (!is_dpio_in_blocklist && dpio_count > 1) {

The new helper has no such guard, so with exactly one DPIO,
last_index == 0, current_device == 0, and the primary branch removes it:

  } else if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
                  current_device == last_index) {
          fslmc_bus_remove_device(dev);

Restore the dpio_count > 1 condition.

Warning: when a DPMCP is blocklisted, surplus MPORTAL devices are left
in the bus list.  fslmc_vfio_process_group() now breaks out of the
MPORTAL loop after the first device.  When is_dpmcp_in_blocklist is set,
fslmc_filter_control_devices() skips the split, so more than one MPORTAL
can remain and the extras are never removed.  The previous loop had no
break and removed all of them.

Patch 11/11: bus/fslmc: use generic cleanup

Error: cleanup ordering makes fslmc_vfio_close_group() a no-op.

  rte_bus_generic_cleanup(bus);

  ret = fslmc_vfio_close_group();

rte_bus_generic_cleanup() unplugs every device, calls
rte_bus_remove_device() and then bus->free_device(), emptying
rte_fslmc_bus.device_list.  fslmc_vfio_close_group() then iterates that
now-empty list, so fslmc_close_iodevices() is never called for DPIO,
DPCON, DPCI, DPBP or DPDMUX; only fslmc_vfio_clear_group() still runs.
Call fslmc_vfio_close_group() before rte_bus_generic_cleanup().

Warning: the return value of rte_bus_generic_cleanup() is discarded.  It
reports unplug failures via -1/rte_errno; fslmc_cleanup() overwrites
"ret" with the fslmc_vfio_close_group() result and loses it.

Warning: fslmc_free_device() only calls free().  It does not release
dev->intr_handle and does not decrement fslmc_bus_device_count[], both
of which fslmc_bus_remove_device() handles.  Devices torn down through
the generic path therefore leave the per-type counters permanently
stale.

Info (01/11): removing the "struct rte_dpaa2_device *dev" declaration
from the process_once block leaves a stray blank line at the top of the
block in rte_fslmc_scan().

Info (06/11): dev_types[] is a non-static, non-const local array, so it
is rebuilt on the stack for every device.  Make it "static const".

Info (06/11): the sscanf() return value is unchecked, and the new
prefix-based match is weaker than the old strtok() guard.  A name such
as "dpni." matches the prefix, leaves dev_id pointing at "", sscanf()
fails, and the device is registered with object_id 0 -- colliding with a
real dpni.0.  The old code rejected it because strtok(NULL, ".")
returned NULL.  Check that sscanf() returns 1 and skip the device
otherwise.