Re: [dpdk-dev] [PATCH v2 6/7] pci: Combine rte_eal_pci_scan and rte_eal_pci_probe

[Shreyansh Jain <shreyansh.jain@nxp.com>]

On Thursday 24 November 2016 01:37 AM, Ben Walker wrote:
> Two functions is both confusing and unnecessary. Previously,
> rte_eal_pci_scan populated an internal list of devices by
> scanning sysfs. Then, rte_eal_pci_probe would match registered
> drivers to that internal list. These are not really useful
> operations to perform separately, though, so
> simplify the api down to just rte_eal_pci_probe which can
> be called repeatedly through the lifetime of the application
> to scan for new or removed PCI devices and load or unload
> drivers as required.

Agree with this.
And similar case exists for rte_eal_dev_init() for which there is 
another patch floated by Jerin [1].

Also, I am already merging these two (EAL Bus model) [2]. So, we have:
  - Only a probe called from EAL for all devices, whether PCI, VDEV or 
another other type
  - Probe in turns performs all scans and driver->probes()

Concern I have is that with the change of placement for device scan, 
would it impact the external modules/PMDs as currently the scanned list 
is created *before* eal_plugins_init(). But, now the list is created 
*after* plugin initialization.

There are other similar inits like creation of slave threads. As well as 
concern raised by Ferruh about device enumeration.

I don't have clear idea of all such dependencies. Maybe David and Ferruh 
in CC might be able to comment better.

[1] http://dpdk.org/ml/archives/dev/2016-November/050415.html
[2] http://dpdk.org/ml/archives/dev/2016-November/050301.html

>
> Signed-off-by: Ben Walker <benjamin.walker@intel.com>
> ---
>  app/test/test_pci.c                             |  2 +-
>  lib/librte_eal/bsdapp/eal/eal.c                 |  3 ---
>  lib/librte_eal/bsdapp/eal/eal_pci.c             | 17 +----------------
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 -
>  lib/librte_eal/common/eal_common_pci.c          |  6 ++++++
>  lib/librte_eal/common/eal_private.h             | 14 +++++---------
>  lib/librte_eal/common/include/rte_pci.h         | 17 +++++------------
>  lib/librte_eal/linuxapp/eal/eal.c               |  3 ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c           | 18 +-----------------
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 -
>  10 files changed, 19 insertions(+), 63 deletions(-)
>
> diff --git a/app/test/test_pci.c b/app/test/test_pci.c
> index cda186d..fdd84f7 100644
> --- a/app/test/test_pci.c
> +++ b/app/test/test_pci.c
> @@ -180,7 +180,7 @@ test_pci_setup(void)
>  		TAILQ_INSERT_TAIL(&real_pci_device_list, dev, next);
>  	}
>
> -	ret = rte_eal_pci_scan();
> +	ret = rte_eal_pci_probe();
>  	TEST_ASSERT_SUCCESS(ret, "failed to scan PCI bus");
>  	rte_eal_pci_dump(stdout);
>
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 35e3117..fd44528 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -561,9 +561,6 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_timer_init() < 0)
>  		rte_panic("Cannot init HPET or TSC timers\n");
>
> -	if (rte_eal_pci_init() < 0)
> -		rte_panic("Cannot init PCI\n");
> -
>  	eal_check_mem_on_local_socket();
>
>  	if (eal_plugins_init() < 0)
> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
> index 8b3ed88..6c3a169 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> @@ -361,7 +361,7 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
>   * list. Call pci_scan_one() for each pci entry found.
>   */
>  int
> -rte_eal_pci_scan(void)
> +pci_scan(void)
>  {
>  	int fd;
>  	unsigned dev_count = 0;
> @@ -667,18 +667,3 @@ rte_eal_pci_ioport_unmap(struct rte_pci_ioport *p)
>
>  	return ret;
>  }
> -
> -/* Init the PCI EAL subsystem */
> -int
> -rte_eal_pci_init(void)
> -{
> -	/* for debug purposes, PCI can be disabled */
> -	if (internal_config.no_pci)
> -		return 0;
> -
> -	if (rte_eal_pci_scan() < 0) {
> -		RTE_LOG(ERR, EAL, "%s(): Cannot scan PCI bus\n", __func__);
> -		return -1;
> -	}
> -	return 0;
> -}
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 2f81f7c..67c469c 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -44,7 +44,6 @@ DPDK_2.0 {
>  	rte_eal_pci_probe;
>  	rte_eal_pci_probe_one;
>  	rte_eal_pci_register;
> -	rte_eal_pci_scan;
>  	rte_eal_pci_unregister;
>  	rte_eal_process_type;
>  	rte_eal_remote_launch;
> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
> index 4f8c3a0..d50a534 100644
> --- a/lib/librte_eal/common/eal_common_pci.c
> +++ b/lib/librte_eal/common/eal_common_pci.c
> @@ -81,6 +81,7 @@
>  #include <rte_devargs.h>
>
>  #include "eal_private.h"
> +#include "eal_internal_cfg.h"
>
>  struct pci_driver_list pci_driver_list =
>  	TAILQ_HEAD_INITIALIZER(pci_driver_list);
> @@ -423,6 +424,11 @@ rte_eal_pci_probe(void)
>  	int probe_all = 0;
>  	int ret = 0;
>
> +	if (internal_config.no_pci)
> +		return 0;
> +
> +	pci_scan();
> +

So, no check for error reported by scan?
I think we should, 1) because pci_scan() returns one, and 2) because in 
case scan failed, there is no reason probe would do anything worthwhile.

>  	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) == 0)
>  		probe_all = 1;
>
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 9e7d8f6..54f18ea 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -108,18 +108,14 @@ int rte_eal_timer_init(void);
>   */
>  int rte_eal_log_init(const char *id, int facility);
>
> -/**
> - * Init the PCI infrastructure
> +struct rte_pci_driver;
> +struct rte_pci_device;
> +
> +/* Scan the PCI bus for devices
>   *
>   * This function is private to EAL.
> - *
> - * @return
> - *   0 on success, negative on error
>   */
> -int rte_eal_pci_init(void);
> -
> -struct rte_pci_driver;
> -struct rte_pci_device;
> +int pci_scan(void);
>
>  /**
>   * Update a pci device object by asking the kernel for the latest information.
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 5d0feac..2154a54 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -386,20 +386,13 @@ RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
>  void rte_eal_pci_unregister(struct rte_pci_driver *driver);
>
>  /**
> - * Scan the content of the PCI bus, and the devices in the devices
> - * list
> - *
> - * @return
> - *  0 on success, negative on error
> - */
> -int rte_eal_pci_scan(void);
> -
> -/**
> - * Probe the PCI bus for registered drivers.
> + * Scan the PCI bus for devices and match them to their driver.
>   *
>   * Scan the content of the PCI bus, and call the probe() function for
> - * all registered drivers that have a matching entry in its id_table
> - * for discovered devices.
> + * all registered drivers that have a matching entry in their id_table.
> + * If a device already has a driver loaded, probe will not be called.
> + * If a previously discovered device is no longer present on the system,
> + * the associated driver's remove() callback will be called.
>   *
>   * @return
>   *   - 0 on success.
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 2075282..f47f361 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -802,9 +802,6 @@ rte_eal_init(int argc, char **argv)
>  	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
>  		rte_panic("Cannot init logs\n");
>
> -	if (rte_eal_pci_init() < 0)
> -		rte_panic("Cannot init PCI\n");
> -
>  #ifdef VFIO_PRESENT
>  	if (rte_eal_vfio_setup() < 0)
>  		rte_panic("Cannot init VFIO\n");
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 936f076..5146385 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -479,7 +479,7 @@ parse_pci_addr_format(const char *buf, int bufsize, struct rte_pci_addr *addr)
>   * list
>   */
>  int
> -rte_eal_pci_scan(void)
> +pci_scan(void)
>  {
>  	struct dirent *e;
>  	DIR *dir;
> @@ -810,19 +810,3 @@ rte_eal_pci_ioport_unmap(struct rte_pci_ioport *p)
>
>  	return ret;
>  }
> -
> -/* Init the PCI EAL subsystem */
> -int
> -rte_eal_pci_init(void)
> -{
> -	/* for debug purposes, PCI can be disabled */
> -	if (internal_config.no_pci)
> -		return 0;
> -
> -	if (rte_eal_pci_scan() < 0) {
> -		RTE_LOG(ERR, EAL, "%s(): Cannot scan PCI bus\n", __func__);
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 83721ba..856728e 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -44,7 +44,6 @@ DPDK_2.0 {
>  	rte_eal_pci_probe;
>  	rte_eal_pci_probe_one;
>  	rte_eal_pci_register;
> -	rte_eal_pci_scan;
>  	rte_eal_pci_unregister;
>  	rte_eal_process_type;
>  	rte_eal_remote_launch;
>


-- 
-
Shreyansh