[dpdk-dev,01/22] eal: introduce one device scan
diff mbox series

Message ID 20180607123849.14439-2-qi.z.zhang@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • enable hotplug on multi-process
Related show

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Zhang, Qi Z June 7, 2018, 12:38 p.m. UTC
When hot plug a new device, it is not necessary to scan everything
on the bus since the devname and devargs are already there. So new
rte_bus ops "scan_one" is introduced, bus driver can implement this
function to simply the hotplug process.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_eal/common/eal_common_dev.c  | 17 +++++++++++++----
 lib/librte_eal/common/include/rte_bus.h |  4 ++++
 2 files changed, 17 insertions(+), 4 deletions(-)

Comments

Shreyansh Jain June 8, 2018, 11:12 a.m. UTC | #1
On 6/7/2018 6:08 PM, Qi Zhang wrote:
> When hot plug a new device, it is not necessary to scan everything
> on the bus since the devname and devargs are already there. So new
> rte_bus ops "scan_one" is introduced, bus driver can implement this
> function to simply the hotplug process.
              ^^^^^^^^^
              simplify

> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>   lib/librte_eal/common/eal_common_dev.c  | 17 +++++++++++++----
>   lib/librte_eal/common/include/rte_bus.h |  4 ++++
>   2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 61cb3b162..1ad033536 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -147,11 +147,20 @@ int __rte_experimental rte_eal_hotplug_add(const char *busname, const char *devn
>   	if (ret)
>   		goto err_devarg;
>   
> -	ret = bus->scan();
> -	if (ret)
> -		goto err_devarg;
> +	/**
> +	 * if bus support to scan specific device by devargs,
> +	 * we don't need to scan all devices on the bus.
> +	 */
> +	if (bus->scan_one) {
> +		dev = bus->scan_one(da);
> +	} else {
> +		ret = bus->scan();
> +		if (ret)
> +			goto err_devarg;
> +
> +		dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
> +	}
>   
> -	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
>   	if (dev == NULL) {
>   		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
>   			devname);
> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> index eb9eded4e..b15cff892 100644
> --- a/lib/librte_eal/common/include/rte_bus.h
> +++ b/lib/librte_eal/common/include/rte_bus.h
> @@ -83,6 +83,7 @@ enum rte_iova_mode {
>    */
>   typedef int (*rte_bus_scan_t)(void);
>   
> +typedef struct rte_device *(*rte_bus_scan_one_t)(struct rte_devargs *);

You should add comments over the declaration, just like the other 
similar declarations.
And, a new line should be here.

>   /**
>    * Implementation specific probe function which is responsible for linking
>    * devices on that bus with applicable drivers.
> @@ -95,6 +96,8 @@ typedef int (*rte_bus_scan_t)(void);
>    */
>   typedef int (*rte_bus_probe_t)(void);
>   
> +
> +

And please remove the extra lines added above in next version of patch.

>   /**
>    * Device iterator to find a device on a bus.
>    *
> @@ -204,6 +207,7 @@ struct rte_bus {
>   	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
>   	const char *name;            /**< Name of the bus */
>   	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
> +	rte_bus_scan_one_t scan_one; /**< Scan one device by devargs */

I think you mean "Scan one device using devargs" rather than "Scan one 
device by devargs".

>   	rte_bus_probe_t probe;       /**< Probe devices on bus */
>   	rte_bus_find_device_t find_device; /**< Find a device on the bus */
>   	rte_bus_plug_t plug;         /**< Probe single device for drivers */
>
Zhang, Qi Z June 13, 2018, 1:32 p.m. UTC | #2
Hi Shreyansh:
	Thanks for your review.
	Will fix base on your comments in v2.
Regards
Qi

> -----Original Message-----
> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
> Sent: Friday, June 8, 2018 7:12 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Shelton, Benjamin H
> <benjamin.h.shelton@intel.com>; Vangati, Narender
> <narender.vangati@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/22] eal: introduce one device scan
> 
> On 6/7/2018 6:08 PM, Qi Zhang wrote:
> > When hot plug a new device, it is not necessary to scan everything on
> > the bus since the devname and devargs are already there. So new
> > rte_bus ops "scan_one" is introduced, bus driver can implement this
> > function to simply the hotplug process.
>               ^^^^^^^^^
>               simplify
> 
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >   lib/librte_eal/common/eal_common_dev.c  | 17 +++++++++++++----
> >   lib/librte_eal/common/include/rte_bus.h |  4 ++++
> >   2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/eal_common_dev.c
> > b/lib/librte_eal/common/eal_common_dev.c
> > index 61cb3b162..1ad033536 100644
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -147,11 +147,20 @@ int __rte_experimental
> rte_eal_hotplug_add(const char *busname, const char *devn
> >   	if (ret)
> >   		goto err_devarg;
> >
> > -	ret = bus->scan();
> > -	if (ret)
> > -		goto err_devarg;
> > +	/**
> > +	 * if bus support to scan specific device by devargs,
> > +	 * we don't need to scan all devices on the bus.
> > +	 */
> > +	if (bus->scan_one) {
> > +		dev = bus->scan_one(da);
> > +	} else {
> > +		ret = bus->scan();
> > +		if (ret)
> > +			goto err_devarg;
> > +
> > +		dev = bus->find_device(NULL, cmp_detached_dev_name,
> devname);
> > +	}
> >
> > -	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
> >   	if (dev == NULL) {
> >   		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
> >   			devname);
> > diff --git a/lib/librte_eal/common/include/rte_bus.h
> > b/lib/librte_eal/common/include/rte_bus.h
> > index eb9eded4e..b15cff892 100644
> > --- a/lib/librte_eal/common/include/rte_bus.h
> > +++ b/lib/librte_eal/common/include/rte_bus.h
> > @@ -83,6 +83,7 @@ enum rte_iova_mode {
> >    */
> >   typedef int (*rte_bus_scan_t)(void);
> >
> > +typedef struct rte_device *(*rte_bus_scan_one_t)(struct rte_devargs
> > +*);
> 
> You should add comments over the declaration, just like the other similar
> declarations.
> And, a new line should be here.
> 
> >   /**
> >    * Implementation specific probe function which is responsible for
> linking
> >    * devices on that bus with applicable drivers.
> > @@ -95,6 +96,8 @@ typedef int (*rte_bus_scan_t)(void);
> >    */
> >   typedef int (*rte_bus_probe_t)(void);
> >
> > +
> > +
> 
> And please remove the extra lines added above in next version of patch.
> 
> >   /**
> >    * Device iterator to find a device on a bus.
> >    *
> > @@ -204,6 +207,7 @@ struct rte_bus {
> >   	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list
> */
> >   	const char *name;            /**< Name of the bus */
> >   	rte_bus_scan_t scan;         /**< Scan for devices attached to
> bus */
> > +	rte_bus_scan_one_t scan_one; /**< Scan one device by devargs */
> 
> I think you mean "Scan one device using devargs" rather than "Scan one
> device by devargs".
> 
> >   	rte_bus_probe_t probe;       /**< Probe devices on bus */
> >   	rte_bus_find_device_t find_device; /**< Find a device on the bus
> */
> >   	rte_bus_plug_t plug;         /**< Probe single device for drivers
> */
> >

Patch
diff mbox series

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 61cb3b162..1ad033536 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -147,11 +147,20 @@  int __rte_experimental rte_eal_hotplug_add(const char *busname, const char *devn
 	if (ret)
 		goto err_devarg;
 
-	ret = bus->scan();
-	if (ret)
-		goto err_devarg;
+	/**
+	 * if bus support to scan specific device by devargs,
+	 * we don't need to scan all devices on the bus.
+	 */
+	if (bus->scan_one) {
+		dev = bus->scan_one(da);
+	} else {
+		ret = bus->scan();
+		if (ret)
+			goto err_devarg;
+
+		dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
+	}
 
-	dev = bus->find_device(NULL, cmp_detached_dev_name, devname);
 	if (dev == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot find unplugged device (%s)\n",
 			devname);
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
index eb9eded4e..b15cff892 100644
--- a/lib/librte_eal/common/include/rte_bus.h
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -83,6 +83,7 @@  enum rte_iova_mode {
  */
 typedef int (*rte_bus_scan_t)(void);
 
+typedef struct rte_device *(*rte_bus_scan_one_t)(struct rte_devargs *);
 /**
  * Implementation specific probe function which is responsible for linking
  * devices on that bus with applicable drivers.
@@ -95,6 +96,8 @@  typedef int (*rte_bus_scan_t)(void);
  */
 typedef int (*rte_bus_probe_t)(void);
 
+
+
 /**
  * Device iterator to find a device on a bus.
  *
@@ -204,6 +207,7 @@  struct rte_bus {
 	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
 	const char *name;            /**< Name of the bus */
 	rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
+	rte_bus_scan_one_t scan_one; /**< Scan one device by devargs */
 	rte_bus_probe_t probe;       /**< Probe devices on bus */
 	rte_bus_find_device_t find_device; /**< Find a device on the bus */
 	rte_bus_plug_t plug;         /**< Probe single device for drivers */