[dpdk-dev,v5,04/12] eal: integrate bus scan and probe with EAL
Checks
Commit Message
Still a dummy implementation as no real bus driver exists. This adds calls
from EAL to bus specific scan, match functions.
Once driver->probe is in place, and a bus handler has been installed,
the code would become effective.
Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
lib/librte_eal/bsdapp/eal/eal.c | 7 +++++++
lib/librte_eal/common/eal_common_bus.c | 30 ++++++++++++++++++++++++++++++
lib/librte_eal/common/include/rte_bus.h | 19 +++++++++++++++++++
lib/librte_eal/linuxapp/eal/eal.c | 7 +++++++
4 files changed, 63 insertions(+)
Comments
2016-12-26 18:53, Shreyansh Jain:
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -844,6 +845,9 @@ rte_eal_init(int argc, char **argv)
> if (rte_eal_intr_init() < 0)
> rte_panic("Cannot init interrupt-handling thread\n");
>
> + if (rte_eal_bus_scan())
> + rte_panic("Cannot scan the buses for devices\n");
Yes, definitely. Just one scan functions which scan registered bus.
> @@ -884,6 +888,9 @@ rte_eal_init(int argc, char **argv)
> if (rte_eal_pci_probe())
> rte_panic("Cannot probe PCI\n");
>
> + if (rte_eal_bus_probe())
> + rte_panic("Cannot probe devices\n");
> +
> if (rte_eal_dev_init() < 0)
> rte_panic("Cannot init pmd devices\n");
What is the benefit of initializing (probe) a device outside of the scan?
Currently, it is done in two steps, so you are keeping the same behaviour.
I imagine a model where the scan function decide to initialize the
device and can require some help from a callback to make this decision.
So the whitelist/blacklist policy can be implemented with callbacks at
the scan level and possibly the responsibility of the application.
Note that the callback model would be a change for a next release.
On Wednesday 04 January 2017 03:16 AM, Thomas Monjalon wrote:
> 2016-12-26 18:53, Shreyansh Jain:
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -844,6 +845,9 @@ rte_eal_init(int argc, char **argv)
>> if (rte_eal_intr_init() < 0)
>> rte_panic("Cannot init interrupt-handling thread\n");
>>
>> + if (rte_eal_bus_scan())
>> + rte_panic("Cannot scan the buses for devices\n");
>
> Yes, definitely. Just one scan functions which scan registered bus.
>
>> @@ -884,6 +888,9 @@ rte_eal_init(int argc, char **argv)
>> if (rte_eal_pci_probe())
>> rte_panic("Cannot probe PCI\n");
>>
>> + if (rte_eal_bus_probe())
>> + rte_panic("Cannot probe devices\n");
>> +
>> if (rte_eal_dev_init() < 0)
>> rte_panic("Cannot init pmd devices\n");
>
> What is the benefit of initializing (probe) a device outside of the scan?
> Currently, it is done in two steps, so you are keeping the same behaviour.
Yes, only for compatibility to existing model of two-step process.
Ideally, only a single step is enough (init->probe).
During the discussion in [1] also this point was raised - at that time
for VDEV and applicability to PCI.
[1] http://dpdk.org/ml/archives/dev/2016-December/051306.html
If you want, I can merge these two. I postponed it because 1) it is an
independent change and should really impact bus and 2) I was not sure
of dependency of init *before* pthread_create for all workers.
>
> I imagine a model where the scan function decide to initialize the
> device and can require some help from a callback to make this decision.
> So the whitelist/blacklist policy can be implemented with callbacks at
> the scan level and possibly the responsibility of the application.
> Note that the callback model would be a change for a next release.
>
Agree. But, that is not really part of Bus patches - isn't it? Or, you
want to change that with this series?
On Friday 06 January 2017 04:08 PM, Shreyansh Jain wrote:
> On Wednesday 04 January 2017 03:16 AM, Thomas Monjalon wrote:
>> 2016-12-26 18:53, Shreyansh Jain:
>>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>>> @@ -844,6 +845,9 @@ rte_eal_init(int argc, char **argv)
>>> if (rte_eal_intr_init() < 0)
>>> rte_panic("Cannot init interrupt-handling thread\n");
>>>
>>> + if (rte_eal_bus_scan())
>>> + rte_panic("Cannot scan the buses for devices\n");
>>
>> Yes, definitely. Just one scan functions which scan registered bus.
>>
>>> @@ -884,6 +888,9 @@ rte_eal_init(int argc, char **argv)
>>> if (rte_eal_pci_probe())
>>> rte_panic("Cannot probe PCI\n");
>>>
>>> + if (rte_eal_bus_probe())
>>> + rte_panic("Cannot probe devices\n");
>>> +
>>> if (rte_eal_dev_init() < 0)
>>> rte_panic("Cannot init pmd devices\n");
>>
>> What is the benefit of initializing (probe) a device outside of the scan?
>> Currently, it is done in two steps, so you are keeping the same
>> behaviour.
>
> Yes, only for compatibility to existing model of two-step process.
> Ideally, only a single step is enough (init->probe).
>
> During the discussion in [1] also this point was raised - at that time
> for VDEV and applicability to PCI.
>
> [1] http://dpdk.org/ml/archives/dev/2016-December/051306.html
>
> If you want, I can merge these two. I postponed it because 1) it is an
> independent change and should really impact bus and 2) I was not sure
> of dependency of init *before* pthread_create for all workers.
I forgot _not_ in above - rephrasing:
If you want, I can merge these two. I postponed it because 1) it is an
independent change and should _not_ really impact bus and 2) I was not
sure of dependency of init *before* pthread_create for all workers.
>
>>
>> I imagine a model where the scan function decide to initialize the
>> device and can require some help from a callback to make this decision.
>> So the whitelist/blacklist policy can be implemented with callbacks at
>> the scan level and possibly the responsibility of the application.
>> Note that the callback model would be a change for a next release.
>>
>
> Agree. But, that is not really part of Bus patches - isn't it? Or, you
> want to change that with this series?
>
>
>
2017-01-06 17:30, Shreyansh Jain:
> On Friday 06 January 2017 04:08 PM, Shreyansh Jain wrote:
> > On Wednesday 04 January 2017 03:16 AM, Thomas Monjalon wrote:
> >> 2016-12-26 18:53, Shreyansh Jain:
> >>> --- a/lib/librte_eal/linuxapp/eal/eal.c
> >>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> >>> @@ -844,6 +845,9 @@ rte_eal_init(int argc, char **argv)
> >>> if (rte_eal_intr_init() < 0)
> >>> rte_panic("Cannot init interrupt-handling thread\n");
> >>>
> >>> + if (rte_eal_bus_scan())
> >>> + rte_panic("Cannot scan the buses for devices\n");
> >>
> >> Yes, definitely. Just one scan functions which scan registered bus.
> >>
> >>> @@ -884,6 +888,9 @@ rte_eal_init(int argc, char **argv)
> >>> if (rte_eal_pci_probe())
> >>> rte_panic("Cannot probe PCI\n");
> >>>
> >>> + if (rte_eal_bus_probe())
> >>> + rte_panic("Cannot probe devices\n");
> >>> +
> >>> if (rte_eal_dev_init() < 0)
> >>> rte_panic("Cannot init pmd devices\n");
> >>
> >> What is the benefit of initializing (probe) a device outside of the scan?
> >> Currently, it is done in two steps, so you are keeping the same
> >> behaviour.
> >
> > Yes, only for compatibility to existing model of two-step process.
> > Ideally, only a single step is enough (init->probe).
> >
> > During the discussion in [1] also this point was raised - at that time
> > for VDEV and applicability to PCI.
> >
> > [1] http://dpdk.org/ml/archives/dev/2016-December/051306.html
> >
> > If you want, I can merge these two. I postponed it because 1) it is an
> > independent change and should really impact bus and 2) I was not sure
> > of dependency of init *before* pthread_create for all workers.
>
> I forgot _not_ in above - rephrasing:
>
> If you want, I can merge these two. I postponed it because 1) it is an
> independent change and should _not_ really impact bus and 2) I was not
> sure of dependency of init *before* pthread_create for all workers.
I'm OK with your approach.
> >> I imagine a model where the scan function decide to initialize the
> >> device and can require some help from a callback to make this decision.
> >> So the whitelist/blacklist policy can be implemented with callbacks at
> >> the scan level and possibly the responsibility of the application.
> >> Note that the callback model would be a change for a next release.
> >
> > Agree. But, that is not really part of Bus patches - isn't it? Or, you
> > want to change that with this series?
No it is not the scope of this series.
Please could you add it in the cover letter as a next step?
Thanks
Hi,
...
...
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 2206277..2c223de 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
...
+/* Scan all the buses for registering devices */ int
+rte_eal_bus_scan(void)
+{
+ int ret;
+ struct rte_bus *bus = NULL;
+
+ TAILQ_FOREACH(bus, &rte_bus_list, next) {
+ ret = bus->scan(bus);
+ if (ret) {
+ RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
+ bus->name);
+ /* TODO: Should error on a particular bus block scan
+ * for all others?
+ */
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
Nitpick - the return type of rte_eal_bus_scan() is int and not void:
* Scan all the buses attached to the framework.
+ *
+ * @param void
+ * @return void
+ */
+int rte_eal_bus_scan(void);
Rami Rosen
Hello,
On Sunday 08 January 2017 05:51 PM, Rosen, Rami wrote:
> Hi,
> ....
> ....
>
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 2206277..2c223de 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> ....
>
> +/* Scan all the buses for registering devices */ int
> +rte_eal_bus_scan(void)
> +{
> + int ret;
> + struct rte_bus *bus = NULL;
> +
> + TAILQ_FOREACH(bus, &rte_bus_list, next) {
> + ret = bus->scan(bus);
> + if (ret) {
> + RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> + bus->name);
> + /* TODO: Should error on a particular bus block scan
> + * for all others?
> + */
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
>
> Nitpick - the return type of rte_eal_bus_scan() is int and not void:
Thanks for review. I will fix this before sending v6.
>
> * Scan all the buses attached to the framework.
> + *
> + * @param void
> + * @return void
> + */
> +int rte_eal_bus_scan(void);
>
>
> Rami Rosen
>
-
Shreyansh
On Friday 06 January 2017 07:16 PM, Thomas Monjalon wrote:
> 2017-01-06 17:30, Shreyansh Jain:
>> On Friday 06 January 2017 04:08 PM, Shreyansh Jain wrote:
>>> On Wednesday 04 January 2017 03:16 AM, Thomas Monjalon wrote:
>>>> 2016-12-26 18:53, Shreyansh Jain:
>>>>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>>>>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>>>>> @@ -844,6 +845,9 @@ rte_eal_init(int argc, char **argv)
>>>>> if (rte_eal_intr_init() < 0)
>>>>> rte_panic("Cannot init interrupt-handling thread\n");
>>>>>
>>>>> + if (rte_eal_bus_scan())
>>>>> + rte_panic("Cannot scan the buses for devices\n");
>>>>
>>>> Yes, definitely. Just one scan functions which scan registered bus.
>>>>
>>>>> @@ -884,6 +888,9 @@ rte_eal_init(int argc, char **argv)
>>>>> if (rte_eal_pci_probe())
>>>>> rte_panic("Cannot probe PCI\n");
>>>>>
>>>>> + if (rte_eal_bus_probe())
>>>>> + rte_panic("Cannot probe devices\n");
>>>>> +
>>>>> if (rte_eal_dev_init() < 0)
>>>>> rte_panic("Cannot init pmd devices\n");
>>>>
>>>> What is the benefit of initializing (probe) a device outside of the scan?
>>>> Currently, it is done in two steps, so you are keeping the same
>>>> behaviour.
>>>
>>> Yes, only for compatibility to existing model of two-step process.
>>> Ideally, only a single step is enough (init->probe).
>>>
>>> During the discussion in [1] also this point was raised - at that time
>>> for VDEV and applicability to PCI.
>>>
>>> [1] http://dpdk.org/ml/archives/dev/2016-December/051306.html
>>>
>>> If you want, I can merge these two. I postponed it because 1) it is an
>>> independent change and should really impact bus and 2) I was not sure
>>> of dependency of init *before* pthread_create for all workers.
>>
>> I forgot _not_ in above - rephrasing:
>>
>> If you want, I can merge these two. I postponed it because 1) it is an
>> independent change and should _not_ really impact bus and 2) I was not
>> sure of dependency of init *before* pthread_create for all workers.
>
> I'm OK with your approach.
>
>>>> I imagine a model where the scan function decide to initialize the
>>>> device and can require some help from a callback to make this decision.
>>>> So the whitelist/blacklist policy can be implemented with callbacks at
>>>> the scan level and possibly the responsibility of the application.
>>>> Note that the callback model would be a change for a next release.
>>>
>>> Agree. But, that is not really part of Bus patches - isn't it? Or, you
>>> want to change that with this series?
>
> No it is not the scope of this series.
> Please could you add it in the cover letter as a next step?
Yes, I will add to cover letter as Pending Item.
> Thanks
>
@@ -64,6 +64,7 @@
#include <rte_string_fns.h>
#include <rte_cpuflags.h>
#include <rte_interrupts.h>
+#include <rte_bus.h>
#include <rte_pci.h>
#include <rte_dev.h>
#include <rte_devargs.h>
@@ -577,6 +578,9 @@ rte_eal_init(int argc, char **argv)
rte_config.master_lcore, thread_id, cpuset,
ret == 0 ? "" : "...");
+ if (rte_eal_bus_scan())
+ rte_panic("Cannot scan the buses for devices\n");
+
RTE_LCORE_FOREACH_SLAVE(i) {
/*
@@ -613,6 +617,9 @@ rte_eal_init(int argc, char **argv)
if (rte_eal_pci_probe())
rte_panic("Cannot probe PCI\n");
+ if (rte_eal_bus_probe())
+ rte_panic("Cannot probe devices\n");
+
if (rte_eal_dev_init() < 0)
rte_panic("Cannot init pmd devices\n");
@@ -172,6 +172,36 @@ rte_eal_bus_unregister(struct rte_bus *bus)
RTE_LOG(INFO, EAL, "Unregistered [%s] bus.\n", bus->name);
}
+/* Scan all the buses for registering devices */
+int
+rte_eal_bus_scan(void)
+{
+ int ret;
+ struct rte_bus *bus = NULL;
+
+ TAILQ_FOREACH(bus, &rte_bus_list, next) {
+ ret = bus->scan(bus);
+ if (ret) {
+ RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
+ bus->name);
+ /* TODO: Should error on a particular bus block scan
+ * for all others?
+ */
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+/* Match driver<->device and call driver->probe() */
+int
+rte_eal_bus_probe(void)
+{
+ /* Until driver->probe is available, this is dummy implementation */
+ return 0;
+}
+
/* dump one bus info */
static int
bus_dump_one(FILE *f, struct rte_bus *bus)
@@ -212,6 +212,25 @@ void rte_eal_bus_unregister(struct rte_bus *bus);
*/
struct rte_bus *rte_eal_bus_get(const char *bus_name);
+/** @internal
+ * Scan all the buses attached to the framework.
+ *
+ * @param void
+ * @return void
+ */
+int rte_eal_bus_scan(void);
+
+/** @internal
+ * For each device on the bus, perform a driver 'match' and call the
+ * driver's probe for device initialization.
+ *
+ * @param void
+ * @return
+ * 0 for successful match/probe
+ * !0 otherwise
+ */
+int rte_eal_bus_probe(void);
+
/**
* Dump information of all the buses registered with EAL.
*
@@ -69,6 +69,7 @@
#include <rte_string_fns.h>
#include <rte_cpuflags.h>
#include <rte_interrupts.h>
+#include <rte_bus.h>
#include <rte_pci.h>
#include <rte_dev.h>
#include <rte_devargs.h>
@@ -844,6 +845,9 @@ rte_eal_init(int argc, char **argv)
if (rte_eal_intr_init() < 0)
rte_panic("Cannot init interrupt-handling thread\n");
+ if (rte_eal_bus_scan())
+ rte_panic("Cannot scan the buses for devices\n");
+
RTE_LCORE_FOREACH_SLAVE(i) {
/*
@@ -884,6 +888,9 @@ rte_eal_init(int argc, char **argv)
if (rte_eal_pci_probe())
rte_panic("Cannot probe PCI\n");
+ if (rte_eal_bus_probe())
+ rte_panic("Cannot probe devices\n");
+
if (rte_eal_dev_init() < 0)
rte_panic("Cannot init pmd devices\n");