On Thu, 23 Jul 2026 15:53:48 +0200
David Marchand <[email protected]> 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.
