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 <redacted>
Cc: thomas@monjalon.net; Burakov, Anatoly <redacted>;
Ananyev, Konstantin [off-list ref]; dev@dpdk.org;
Richardson, Bruce [off-list ref]; Yigit, Ferruh
[off-list ref]; Shelton, Benjamin H
[off-list ref]; Vangati, Narender
[off-list ref]
Subject: Re: [dpdk-dev] [PATCH 01/22] eal: introduce one device scan

On 6/7/2018 6:08 PM, Qi Zhang wrote:

quoted

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

quoted

Signed-off-by: Qi Zhang <redacted>
---
  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

quoted

  	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);

quoted

+	}

-	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.

quoted

  /**
   * Implementation specific probe function which is responsible for

linking

quoted

   * 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.

quoted

  /**
   * 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

*/

quoted

  	const char *name;            /**< Name of the bus */
  	rte_bus_scan_t scan;         /**< Scan for devices attached to

bus */

quoted

+	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".

quoted

  	rte_bus_probe_t probe;       /**< Probe devices on bus */
  	rte_bus_find_device_t find_device; /**< Find a device on the bus

*/

quoted

  	rte_bus_plug_t plug;         /**< Probe single device for drivers

*/

quoted