|SUCCESS| [v4,7/7] eal: configure initial device probing

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163030

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-15

Reviewing this DPDK patch series against coding standards and contribution guidelines.

commit 20260414123900.3355883-2-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

    devtools: check packet forwarding in null test

> diff --git a/devtools/test-null.sh b/devtools/test-null.sh
> index e3ac168ce3..db16f36bfa 100755
> --- a/devtools/test-null.sh
> +++ b/devtools/test-null.sh

[ ... ]

> +logfile=$build/test-null.log

Does this code clean up the log file between runs or when an error occurs?
The logfile variable is initialized but there's no removal of old log data.

> +# we expect two ports and some traffic is received and transmitted
> +grep -q 'io packet forwarding - ports=2 -' $logfile
> +grep 'RX-packets: ' $logfile | tail -1 | grep -q 'RX-packets:[[:space:]]*[^0[:space:]]'
> +grep 'TX-packets: ' $logfile | tail -1 | grep -q 'TX-packets:[[:space:]]*[^0[:space:]]'

Does this code check the exit status of the first grep command? The
script should fail if the pattern is not found but there's no explicit
error handling.

commit 20260414123900.3355883-3-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

    bus/fslmc: fix bus cleanup

> diff --git a/drivers/bus/fslmc/fslmc_vfio.c b/drivers/bus/fslmc/fslmc_vfio.c
> index 550d4e0e8d..7daa18d850 100644
> --- a/drivers/bus/fslmc/fslmc_vfio.c
> +++ b/drivers/bus/fslmc/fslmc_vfio.c

[ ... ]

> @@ -1411,22 +1411,9 @@ fslmc_close_iodevices(struct rte_dpaa2_device *dev,
>  	case DPAA2_ETH:
>  	case DPAA2_CRYPTO:
>  	case DPAA2_QDMA:
> -		probe_all = rte_fslmc_bus.bus.conf.scan_mode !=
> -			    RTE_BUS_SCAN_ALLOWLIST;
> -		TAILQ_FOREACH(drv, &rte_fslmc_bus.driver_list, next) {
> -			if (drv->drv_type != dev->dev_type)
> -				continue;
> -			if (rte_dev_is_probed(&dev->device))
> -				continue;
> -			if (probe_all ||
> -			    (dev->device.devargs &&
> -			     dev->device.devargs->policy ==
> -			     RTE_DEV_ALLOWED)) {
> -				ret = drv->remove(dev);
> -				if (ret)
> -					DPAA2_BUS_ERR("Unable to remove");
> -			}
> -		}
> +		drv = dev->driver;
> +		if (drv && drv->remove && drv->remove(dev))
> +			DPAA2_BUS_ERR("Unable to remove");

Can dev->driver ever be NULL when entering this close function? If a
device is partially initialized but never fully probed, could this code
skip needed cleanup?

commit 20260414123900.3355883-6-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

    bus: factorize devargs lookup

> diff --git a/drivers/bus/dpaa/dpaa_bus.c b/drivers/bus/dpaa/dpaa_bus.c
> index 8b4a71f852..1d12f2dceb 100644
> --- a/drivers/bus/dpaa/dpaa_bus.c
> +++ b/drivers/bus/dpaa/dpaa_bus.c

[ ... ]

> @@ -562,6 +562,18 @@ rte_dpaa_bus_parse(const char *name, void *out)
>  	return 0;
>  }
>  
> +static int
> +dpaa_bus_dev_compare(const char *name1, const char *name2)
> +{
> +	char devname1[32], devname2[32];
> +
> +	if (rte_dpaa_bus_parse(name1, devname1) != 0 ||
> +			rte_dpaa_bus_parse(name2, devname2) != 0)
> +		return 1;
> +
> +	return strncmp(devname1, devname2, sizeof(devname1));
> +}

Does this comparison function handle the case where devname1 equals
devname2 but strncmp returns non-zero because the strings are longer
than 32 characters? Should this use strcmp instead of strncmp if
both strings fit in the buffer?

> diff --git a/drivers/bus/fslmc/fslmc_bus.c b/drivers/bus/fslmc/fslmc_bus.c
> index 38f14c3050..cf881b3eec 100644
> --- a/drivers/bus/fslmc/fslmc_bus.c
> +++ b/drivers/bus/fslmc/fslmc_bus.c

[ ... ]

> @@ -308,6 +292,18 @@ rte_fslmc_parse(const char *name, void *addr)
>  	return ret;
>  }
>  
> +static int
> +fslmc_dev_compare(const char *name1, const char *name2)
> +{
> +	char devname1[32], devname2[32];
> +
> +	if (rte_fslmc_parse(name1, devname1) != 0 ||
> +			rte_fslmc_parse(name2, devname2) != 0)
> +		return 1;
> +
> +	return strncmp(devname1, devname2, sizeof(devname1));
> +}

Same question as for dpaa_bus_dev_compare - should this use strcmp
rather than strncmp?

> diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
> index 0a2311a342..474fa9331e 100644
> --- a/lib/eal/common/eal_common_bus.c
> +++ b/lib/eal/common/eal_common_bus.c

[ ... ]

> +RTE_EXPORT_INTERNAL_SYMBOL(rte_bus_find_devargs)
> +struct rte_devargs *
> +rte_bus_find_devargs(const struct rte_bus *bus, const char *name)
> +{
> +	rte_bus_dev_compare_t cmp = bus->dev_compare;
> +	const char *bus_name = rte_bus_name(bus);
> +	struct rte_devargs *devargs;
> +
> +	if (cmp == NULL)
> +		cmp = strcmp;
> +
> +	RTE_EAL_DEVARGS_FOREACH(bus_name, devargs) {
> +		const char *devargs_name = devargs->name;
> +
> +		/* The name in the devargs is usually prefixed with <bus>: */
> +		if (strncmp(devargs_name, bus_name, strlen(bus_name)) == 0)
> +			devargs_name += strlen(bus_name) + 1;

Does this code validate that devargs_name[strlen(bus_name)] equals ':'
before advancing the pointer? If the prefix check passes but there's no
colon, could this skip a character incorrectly?

commit 20260414123900.3355883-7-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

    bus: factorize device selection

> diff --git a/lib/eal/common/eal_common_bus.c b/lib/eal/common/eal_common_bus.c
> index c92183b10a..b33f5b4bf4 100644
> --- a/lib/eal/common/eal_common_bus.c
> +++ b/lib/eal/common/eal_common_bus.c

[ ... ]

> @@ -258,18 +258,23 @@ RTE_EXPORT_INTERNAL_SYMBOL(rte_bus_device_is_ignored)
>  bool
>  rte_bus_device_is_ignored(const struct rte_bus *bus, const char *dev_name)
>  {
> +	const struct internal_config *internal_conf = eal_get_internal_configuration();
>  	struct rte_devargs *devargs = rte_bus_find_devargs(bus, dev_name);
> +	enum rte_bus_scan_mode scan_mode = bus->conf.scan_mode;
>  
> -	switch (bus->conf.scan_mode) {
> -	case RTE_BUS_SCAN_ALLOWLIST:
> +	if (scan_mode == RTE_BUS_SCAN_UNDEFINED) {
> +		if (internal_conf->no_auto_probing != 0)
> +			scan_mode = RTE_BUS_SCAN_ALLOWLIST;
> +		else
> +			scan_mode = RTE_BUS_SCAN_BLOCKLIST;
> +	}

Does this code verify that internal_conf is not NULL before
dereferencing it?