From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-bk0-x235.google.com (mail-bk0-x235.google.com [IPv6:2a00:1450:4008:c01::235]) by dpdk.org (Postfix) with ESMTP id 95830234 for ; Mon, 3 Jun 2013 10:58:19 +0200 (CEST) Received: by mail-bk0-f53.google.com with SMTP id mx10so1726554bkb.40 for ; Mon, 03 Jun 2013 01:58:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=message-id:date:from:organization:user-agent:mime-version:to :subject:references:in-reply-to:content-type:x-gm-message-state; bh=DN6o0tsu8n9Zb/BmlZtThxAf5/j657T3WJ14M64Mwrs=; b=DtrUO0mut2TU6yQbz27ExjamLt6V/GEF51lKumIpKmSmv7IWiC9NuAj/knyGLl0jht elxwxX5hyjFJsVzSELYEYabgXBnYsliHcdtAoHRiY4SjftUVrGrdqbiy+B9S2I2q0DXz kcQkrZGoVjSFkPjK+yNUEJMpJRVBV8xnDwcZQnyjN0AvpO/wgk4n+aYcNhb+EROYU+4L GHvAj21EtTgQgUG/iglGTppHzZREG72rIJ+pkpTa6TXJh1JjzGBOEb8C09D9uKJER6Rg 0Wtgy/CliRmdECh+0/FCBu3XdjYRvOXNP2p69xw1iVLidRxhVCK45VcjeXriHFf1KQjF dXnQ== X-Received: by 10.205.46.131 with SMTP id uo3mr6111998bkb.43.1370249906737; Mon, 03 Jun 2013 01:58:26 -0700 (PDT) Received: from [192.168.0.11] (mic92-3-81-56-67-82.fbx.proxad.net. [81.56.67.82]) by mx.google.com with ESMTPSA id b12sm19418368bkz.0.2013.06.03.01.58.24 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 03 Jun 2013 01:58:25 -0700 (PDT) Message-ID: <51AC5A99.1050207@6wind.com> Date: Mon, 03 Jun 2013 10:58:01 +0200 From: Damien Millescamps Organization: 6WIND S.A. User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/20130509 Thunderbird/17.0.6 MIME-Version: 1.0 To: dev@dpdk.org References: <20130530171234.301927271@vyatta.com> <20130530171626.948387515@vyatta.com> In-Reply-To: <20130530171626.948387515@vyatta.com> Content-Type: multipart/alternative; boundary="------------030207050708040102080701" X-Gm-Message-State: ALoCoQlH0K3mxKcUXPOSZ5MVNKaAmlM9mIVfegIb9ITJ4h03y0QZqDA2GeUxPRiy5FrHFpUlBdEW Subject: Re: [dpdk-dev] [PATCH 4/7] eal: support different modules X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Jun 2013 08:58:20 -0000 This is a multi-part message in MIME format. --------------030207050708040102080701 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 > > --- > 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), > > --------------030207050708040102080701 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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),

--------------030207050708040102080701--