Re: [dpdk-dev] [PATCH 4/7] eal: support different modules

[Damien Millescamps <damien.millescamps@6wind.com>]

[-- Attachment #1: Type: text/plain, Size: 5791 bytes --]

comments inlined.

On 05/30/2013 07:12 PM, Stephen Hemminger wrote:
> An additional change is that failure to detect module should not be fatal,
> but an error like other EAL setup problems, allowing the application
> to recover.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>
> ---
>  app/chkincs/test_pci.c                  |    3 ++-
>  app/test/test_pci.c                     |    4 ++--
>  lib/librte_eal/common/include/rte_pci.h |    5 ++---
>  lib/librte_eal/linuxapp/eal/eal_pci.c   |    9 ++-------
>  lib/librte_pmd_igb/e1000_ethdev.c       |    2 +-
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c     |    4 ++--
>  6 files changed, 11 insertions(+), 16 deletions(-)
>
> --- a/lib/librte_eal/common/include/rte_pci.h	2013-05-29 08:45:38.000000000 -0700
> +++ b/lib/librte_eal/common/include/rte_pci.h	2013-05-29 09:02:50.000000000 -0700
> @@ -151,12 +151,11 @@ struct rte_pci_driver {
>  	pci_devinit_t *devinit;                 /**< Device init. function. */
>  	struct rte_pci_id *id_table;            /**< ID table, NULL terminated. */
>  	uint32_t drv_flags;                     /**< Flags contolling handling of device. */
> +	const char *module_name;		/**< Associated kernel module */

You solution only permits for one module to be checked during
initialization, while the former solution using flags could be easily
extended to check for more than one module.
However it is true that there is a problem with this module check since
it is historically hard-coded for "igb_uio".

Some better solution could be to have a new flag that will check for a
NULL terminated array of module_name.

>  };
>  
> -/** Device needs igb_uio kernel module */
> -#define RTE_PCI_DRV_NEED_IGB_UIO 0x0001
>  /** Device driver must be registered several times until failure */
> -#define RTE_PCI_DRV_MULTIPLE 0x0002
> +#define RTE_PCI_DRV_MULTIPLE 0x0001
You are breaking a public API here, and I don't see any technical reason
to do so. The RTE_PCI_DRV_NEED_IGB_UIO flag could be deprecated, but
there is no way its value could be recycled into an already existing flag.

>  
>  /**
>   * Probe the PCI bus for registered drivers.
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c	2013-03-28 08:50:50.000000000 -0700
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c	2013-05-29 09:04:00.000000000 -0700
> @@ -76,7 +76,7 @@
>   * This code is used to simulate a PCI probe by parsing information in
>   * sysfs. Moreover, when a registered driver matches a device, the
>   * kernel driver currently using it is unloaded and replaced by
> - * igb_uio module, which is a very minimal userland driver for Intel
> + * a module, which is a very minimal userland driver for Intel
>   * network card, only providing access to PCI BAR to applications, and
>   * enabling bus master.
>   */
> @@ -84,8 +84,6 @@
>  
>  #define PROC_MODULES "/proc/modules"
>  
> -#define IGB_UIO_NAME "igb_uio"
> -
>  #define UIO_NEWID "/sys/bus/pci/drivers/%s/new_id"
>  #define UIO_BIND  "/sys/bus/pci/drivers/%s/bind"
>  
> @@ -700,12 +698,9 @@ int
>  rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, struct rte_pci_device *dev)
>  {
>  	struct rte_pci_id *id_table;
> -	const char *module_name = NULL;
> +	const char *module_name = dr->module_name;
>  	int uio_status = -1;
>  
> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_IGB_UIO)
> -		module_name = IGB_UIO_NAME;
> -
>  	uio_status = pci_uio_check_module(module_name);
>  
>  	for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c	2013-05-29 08:45:38.000000000 -0700
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c	2013-05-29 16:08:07.544000027 -0700
> @@ -547,7 +547,7 @@ static struct eth_driver rte_ixgbe_pmd =
>  	{
>  		.name = "rte_ixgbe_pmd",
>  		.id_table = pci_id_ixgbe_map,
> -		.drv_flags = RTE_PCI_DRV_NEED_IGB_UIO,
> +		.module_name = "igb_uio",
>  	},
>  	.eth_dev_init = eth_ixgbe_dev_init,
>  	.dev_private_size = sizeof(struct ixgbe_adapter),
> @@ -560,7 +560,7 @@ static struct eth_driver rte_ixgbevf_pmd
>  	{
>  		.name = "rte_ixgbevf_pmd",
>  		.id_table = pci_id_ixgbevf_map,
> -		.drv_flags = RTE_PCI_DRV_NEED_IGB_UIO,
> +		.module_name = "igb_uio",
>  	},
>  	.eth_dev_init = eth_ixgbevf_dev_init,
>  	.dev_private_size = sizeof(struct ixgbe_adapter),
> --- a/app/test/test_pci.c	2013-03-28 08:50:50.000000000 -0700
> +++ b/app/test/test_pci.c	2013-05-29 16:05:22.881670915 -0700
> @@ -69,7 +69,7 @@ static int my_driver_init(struct rte_pci
>  			  struct rte_pci_device *dev);
>  
>  /*
> - * To test cases where RTE_PCI_DRV_NEED_IGB_UIO is set, and isn't set, two
> + * To test cases where module_name is set, and isn't set, two
>   * drivers are created (one with IGB devices, the other with IXGBE devices).
>   */
>  
> @@ -101,7 +101,7 @@ struct rte_pci_driver my_driver = {
>  	.name = "test_driver",
>  	.devinit = my_driver_init,
>  	.id_table = my_driver_id,
> -	.drv_flags = RTE_PCI_DRV_NEED_IGB_UIO,
> +	.module_name = "igb_uio",
>  };
>  
>  struct rte_pci_driver my_driver2 = {
> --- a/app/chkincs/test_pci.c	2013-03-28 08:50:50.210414181 -0700
> +++ b/app/chkincs/test_pci.c	2013-05-29 16:07:43.880238599 -0700
> @@ -64,7 +64,8 @@ struct rte_pci_driver my_driver = {
>  	"test_driver",
>  	my_driver_init,
>  	my_driver_id,
> -	RTE_PCI_DRV_NEED_IGB_UIO,
> +	0,
> +	"igb_uio",
>  };
>  
>  static int
> --- a/lib/librte_pmd_igb/e1000_ethdev.c	2013-05-29 08:45:38.888471864 -0700
> +++ b/lib/librte_pmd_igb/e1000_ethdev.c	2013-05-29 16:04:50.410003570 -0700
> @@ -336,7 +336,7 @@ static struct eth_driver rte_igb_pmd = {
>  	{
>  		.name = "rte_igb_pmd",
>  		.id_table = pci_id_igb_map,
> -		.drv_flags = RTE_PCI_DRV_NEED_IGB_UIO,
> +		.module_name = "igb_uio",
>  	},
>  	.eth_dev_init = eth_igb_dev_init,
>  	.dev_private_size = sizeof(struct e1000_adapter),
>
>


[-- Attachment #2: Type: text/html, Size: 7227 bytes --]