[dpdk-dev,V2,2/5] Add Intel FPGA BUS Probe Code
Checks
Commit Message
Signed-off-by: Rosen Xu <rosen.xu@intel.com>
---
lib/librte_eal/common/eal_common_bus.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
Comments
Hello Rosen,
On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu <rosen.xu@intel.com> wrote:
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> ---
> lib/librte_eal/common/eal_common_bus.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index 3e022d5..e3bcebe 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -87,7 +87,7 @@ struct rte_bus_list rte_bus_list =
> rte_bus_probe(void)
> {
> int ret;
> - struct rte_bus *bus, *vbus = NULL;
> + struct rte_bus *bus, *vbus = NULL, *ifpga_bus = NULL;
>
> TAILQ_FOREACH(bus, &rte_bus_list, next) {
> if (!strcmp(bus->name, "vdev")) {
> @@ -95,6 +95,11 @@ struct rte_bus_list rte_bus_list =
> continue;
> }
>
> + if (!strcmp(bus->name, "ifpga")) {
> + ifpga_bus = bus;
> + continue;
> + }
> +
> ret = bus->probe();
> if (ret)
> RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n",
> @@ -108,6 +113,13 @@ struct rte_bus_list rte_bus_list =
> vbus->name);
> }
>
> + if (ifpga_bus) {
> + ret = ifpga_bus->probe();
> + if (ret)
> + RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> + ifpga_bus->name);
> + }
> +
Just like my comment on RFC, I still think this is not the right thing to do.
I understand you want a case where IFPGA bus gets probed only after
PCI bus is probed.
There can be multiple ways. Two of them which I can quickly list
without much deliberation:
1. A framework which can 'defer probing'
So, a bus can register for defer probe and its
check_if_probe_available() function callback is called through
rte_bus_probe()
If it returns OK, its probe is called, else it is added to a defer
list which is called once all first register buses are probed.
2. Modify the priority in RTE_REGISTER_BUS and make it as an argument
or a new variant which can take an argument.
It is not ok to change this function specifically for a bus is because
this method is not scalable.
Is there some specific reason you would like to stick to this approach?
[...]
-
Shreyansh
On Wed, Mar 21, 2018 at 2:37 PM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> Hello Rosen,
>
> On Wed, Mar 21, 2018 at 1:21 PM, Rosen Xu <rosen.xu@intel.com> wrote:
>> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
>> ---
>> lib/librte_eal/common/eal_common_bus.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
>> index 3e022d5..e3bcebe 100644
>> --- a/lib/librte_eal/common/eal_common_bus.c
>> +++ b/lib/librte_eal/common/eal_common_bus.c
>> @@ -87,7 +87,7 @@ struct rte_bus_list rte_bus_list =
>> rte_bus_probe(void)
>> {
>> int ret;
>> - struct rte_bus *bus, *vbus = NULL;
>> + struct rte_bus *bus, *vbus = NULL, *ifpga_bus = NULL;
>>
>> TAILQ_FOREACH(bus, &rte_bus_list, next) {
>> if (!strcmp(bus->name, "vdev")) {
>> @@ -95,6 +95,11 @@ struct rte_bus_list rte_bus_list =
>> continue;
>> }
>>
>> + if (!strcmp(bus->name, "ifpga")) {
>> + ifpga_bus = bus;
>> + continue;
>> + }
>> +
>> ret = bus->probe();
>> if (ret)
>> RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n",
>> @@ -108,6 +113,13 @@ struct rte_bus_list rte_bus_list =
>> vbus->name);
>> }
>>
>> + if (ifpga_bus) {
>> + ret = ifpga_bus->probe();
>> + if (ret)
>> + RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
>> + ifpga_bus->name);
>> + }
>> +
>
> Just like my comment on RFC, I still think this is not the right thing to do.
> I understand you want a case where IFPGA bus gets probed only after
> PCI bus is probed.
> There can be multiple ways. Two of them which I can quickly list
> without much deliberation:
>
> 1. A framework which can 'defer probing'
> So, a bus can register for defer probe and its
> check_if_probe_available() function callback is called through
> rte_bus_probe()
> If it returns OK, its probe is called, else it is added to a defer
> list which is called once all first register buses are probed.
>
> 2. Modify the priority in RTE_REGISTER_BUS and make it as an argument
> or a new variant which can take an argument.
>
> It is not ok to change this function specifically for a bus is because
> this method is not scalable.
A cross on my own comment - I know vdev is already doing this special
probe but if we have a proper mechanism we can avoid that as well. Or,
continue to consider vdev as special :D
>
> Is there some specific reason you would like to stick to this approach?
>
> [...]
>
> -
> Shreyansh
On Wed, Mar 21, 2018 at 03:51:31PM +0800, Rosen Xu wrote:
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> ---
> lib/librte_eal/common/eal_common_bus.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index 3e022d5..e3bcebe 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -87,7 +87,7 @@ struct rte_bus_list rte_bus_list =
> rte_bus_probe(void)
> {
> int ret;
> - struct rte_bus *bus, *vbus = NULL;
> + struct rte_bus *bus, *vbus = NULL, *ifpga_bus = NULL;
>
> TAILQ_FOREACH(bus, &rte_bus_list, next) {
> if (!strcmp(bus->name, "vdev")) {
> @@ -95,6 +95,11 @@ struct rte_bus_list rte_bus_list =
> continue;
> }
>
> + if (!strcmp(bus->name, "ifpga")) {
> + ifpga_bus = bus;
> + continue;
> + }
> +
> ret = bus->probe();
> if (ret)
> RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n",
> @@ -108,6 +113,13 @@ struct rte_bus_list rte_bus_list =
> vbus->name);
> }
>
> + if (ifpga_bus) {
> + ret = ifpga_bus->probe();
> + if (ret)
> + RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> + ifpga_bus->name);
> + }
> +
I do not think this solution is generic and scalable anough to get into
common EAL code.
However, I do not think we need to come up with a dependency scheme
(callback registration to check whether proper conditions are met to
proceed with probing).
I think you have a hard-dependency on the PCI bus because you are simply
probing a PCI device. You do not need an additional bus for this.
@@ -87,7 +87,7 @@ struct rte_bus_list rte_bus_list =
rte_bus_probe(void)
{
int ret;
- struct rte_bus *bus, *vbus = NULL;
+ struct rte_bus *bus, *vbus = NULL, *ifpga_bus = NULL;
TAILQ_FOREACH(bus, &rte_bus_list, next) {
if (!strcmp(bus->name, "vdev")) {
@@ -95,6 +95,11 @@ struct rte_bus_list rte_bus_list =
continue;
}
+ if (!strcmp(bus->name, "ifpga")) {
+ ifpga_bus = bus;
+ continue;
+ }
+
ret = bus->probe();
if (ret)
RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n",
@@ -108,6 +113,13 @@ struct rte_bus_list rte_bus_list =
vbus->name);
}
+ if (ifpga_bus) {
+ ret = ifpga_bus->probe();
+ if (ret)
+ RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
+ ifpga_bus->name);
+ }
+
return 0;
}