[dpdk-dev,01/22] eal: introduce one device scan
Checks
Commit Message
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
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
> */
> >
@@ -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);
@@ -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 */