| Message ID | 20180607123849.14439-2-qi.z.zhang@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series |
|
| Related | show |
| Context | Check | Description |
|---|---|---|
| ci/Intel-compilation | fail | Compilation issues |
| ci/checkpatch | success | coding style OK |
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 */ >
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 > */ > >
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 */
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(-)