[dpdk-dev,RFC,3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus

Message ID 1520300638-134954-4-git-send-email-rosen.xu@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Xu, Rosen March 6, 2018, 1:43 a.m. UTC
  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

Shreyansh Jain March 6, 2018, 6:20 a.m. UTC | #1
On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =
>  rte_bus_scan(void)
>  {
>         int ret;
> -       struct rte_bus *bus = NULL;
> +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;
>
>         TAILQ_FOREACH(bus, &rte_bus_list, next) {
> +               if (!strcmp(bus->name, "ifpga")) {
> +                       ifpga_bus = bus;
> +                       continue;
> +               }
> +
>                 ret = bus->scan();
>                 if (ret)
>                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
>                                 bus->name);
>         }
>
> +       if (ifpga_bus) {
> +               ret = ifpga_bus->scan();
> +               if (ret)
> +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> +                               ifpga_bus->name);
> +       }
> +

You are doing this just so that PCI scans are completed *before* ifpga scans?
Well, I understand that this certainly is an issue that we can't yet
define a priority ordering of bus scans.

But, I think what you are require is a simpler:

In the file ifpga_bus.c:

+RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
...
...
#define RTE_REGISTER_BUS(nm, bus) \
RTE_INIT_PRIO(businitfn_ ##nm, 110); \

If you define your own version of RTE_REGISTER_BUS with the priority
number higher, it would be inserted later in the bus list.
rte_register_bus doesn't do any inherent ordering.
This would save the changes you are doing in the
lib/librte_eal/common/eal_common_bus.c file.

But I think there has to be a better provision of defining priority of
bus scans - I am sure when new devices come in, there would be
possibility of dependencies as in your case.

>         return 0;
>  }
>
> --
> 1.8.3.1
>
  
Xu, Rosen March 6, 2018, 10:42 a.m. UTC | #2
-----Original Message-----
From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] 

Sent: Tuesday, March 06, 2018 14:20
To: Xu, Rosen <rosen.xu@intel.com>
Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>
Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus

On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644

> --- a/lib/librte_eal/common/eal_common_bus.c

> +++ b/lib/librte_eal/common/eal_common_bus.c

> @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =

>  rte_bus_scan(void)

>  {

>         int ret;

> -       struct rte_bus *bus = NULL;

> +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;

>

>         TAILQ_FOREACH(bus, &rte_bus_list, next) {

> +               if (!strcmp(bus->name, "ifpga")) {

> +                       ifpga_bus = bus;

> +                       continue;

> +               }

> +

>                 ret = bus->scan();

>                 if (ret)

>                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",

>                                 bus->name);

>         }

>

> +       if (ifpga_bus) {

> +               ret = ifpga_bus->scan();

> +               if (ret)

> +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",

> +                               ifpga_bus->name);

> +       }

> +


You are doing this just so that PCI scans are completed *before* ifpga scans?
Rosen: yes
Well, I understand that this certainly is an issue that we can't yet define a priority ordering of bus scans.

But, I think what you are require is a simpler:

In the file ifpga_bus.c:

+RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
...
...
#define RTE_REGISTER_BUS(nm, bus) \
RTE_INIT_PRIO(businitfn_ ##nm, 110); \

If you define your own version of RTE_REGISTER_BUS with the priority number higher, it would be inserted later in the bus list.
rte_register_bus doesn't do any inherent ordering.
This would save the changes you are doing in the lib/librte_eal/common/eal_common_bus.c file.

But I think there has to be a better provision of defining priority of bus scans - I am sure when new devices come in, there would be possibility of dependencies as in your case.
Rosen: is the priority scan of bus is implemented?

>         return 0;

>  }

>

> --

> 1.8.3.1

>
  
Gaëtan Rivet March 6, 2018, 10:46 a.m. UTC | #3
On Tue, Mar 06, 2018 at 10:42:14AM +0000, Xu, Rosen wrote:
> 
> 
> -----Original Message-----
> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] 
> Sent: Tuesday, March 06, 2018 14:20
> To: Xu, Rosen <rosen.xu@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus
> 
> On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644
> > --- a/lib/librte_eal/common/eal_common_bus.c
> > +++ b/lib/librte_eal/common/eal_common_bus.c
> > @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =
> >  rte_bus_scan(void)
> >  {
> >         int ret;
> > -       struct rte_bus *bus = NULL;
> > +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;
> >
> >         TAILQ_FOREACH(bus, &rte_bus_list, next) {
> > +               if (!strcmp(bus->name, "ifpga")) {
> > +                       ifpga_bus = bus;
> > +                       continue;
> > +               }
> > +
> >                 ret = bus->scan();
> >                 if (ret)
> >                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> >                                 bus->name);
> >         }
> >
> > +       if (ifpga_bus) {
> > +               ret = ifpga_bus->scan();
> > +               if (ret)
> > +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > +                               ifpga_bus->name);
> > +       }
> > +
> 
> You are doing this just so that PCI scans are completed *before* ifpga scans?
> Rosen: yes
> Well, I understand that this certainly is an issue that we can't yet define a priority ordering of bus scans.
> 
> But, I think what you are require is a simpler:
> 
> In the file ifpga_bus.c:
> 
> +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
> ...
> ...
> #define RTE_REGISTER_BUS(nm, bus) \
> RTE_INIT_PRIO(businitfn_ ##nm, 110); \
> 
> If you define your own version of RTE_REGISTER_BUS with the priority number higher, it would be inserted later in the bus list.
> rte_register_bus doesn't do any inherent ordering.
> This would save the changes you are doing in the lib/librte_eal/common/eal_common_bus.c file.
> 
> But I think there has to be a better provision of defining priority of bus scans - I am sure when new devices come in, there would be possibility of dependencies as in your case.
> Rosen: is the priority scan of bus is implemented?

No, there is no priority set for scanning order.
However, the order in which buses are registered, will modify the order
in which scans are done.

Thus, if you change the priority of your registration, you should be
able to ensure that your scan comes last.

> 
> >         return 0;
> >  }
> >
> > --
> > 1.8.3.1
> >
  
Bruce Richardson March 6, 2018, 11:36 a.m. UTC | #4
On Tue, Mar 06, 2018 at 11:46:22AM +0100, Gaëtan Rivet wrote:
> On Tue, Mar 06, 2018 at 10:42:14AM +0000, Xu, Rosen wrote:
> > 
> > 
> > -----Original Message-----
> > From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] 
> > Sent: Tuesday, March 06, 2018 14:20
> > To: Xu, Rosen <rosen.xu@intel.com>
> > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>
> > Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus
> > 
> > On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644
> > > --- a/lib/librte_eal/common/eal_common_bus.c
> > > +++ b/lib/librte_eal/common/eal_common_bus.c
> > > @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =
> > >  rte_bus_scan(void)
> > >  {
> > >         int ret;
> > > -       struct rte_bus *bus = NULL;
> > > +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;
> > >
> > >         TAILQ_FOREACH(bus, &rte_bus_list, next) {
> > > +               if (!strcmp(bus->name, "ifpga")) {
> > > +                       ifpga_bus = bus;
> > > +                       continue;
> > > +               }
> > > +
> > >                 ret = bus->scan();
> > >                 if (ret)
> > >                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > >                                 bus->name);
> > >         }
> > >
> > > +       if (ifpga_bus) {
> > > +               ret = ifpga_bus->scan();
> > > +               if (ret)
> > > +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > > +                               ifpga_bus->name);
> > > +       }
> > > +
> > 
> > You are doing this just so that PCI scans are completed *before* ifpga scans?
> > Rosen: yes
> > Well, I understand that this certainly is an issue that we can't yet define a priority ordering of bus scans.
> > 
> > But, I think what you are require is a simpler:
> > 
> > In the file ifpga_bus.c:
> > 
> > +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
> > ...
> > ...
> > #define RTE_REGISTER_BUS(nm, bus) \
> > RTE_INIT_PRIO(businitfn_ ##nm, 110); \
> > 
> > If you define your own version of RTE_REGISTER_BUS with the priority number higher, it would be inserted later in the bus list.
> > rte_register_bus doesn't do any inherent ordering.
> > This would save the changes you are doing in the lib/librte_eal/common/eal_common_bus.c file.
> > 
> > But I think there has to be a better provision of defining priority of bus scans - I am sure when new devices come in, there would be possibility of dependencies as in your case.
> > Rosen: is the priority scan of bus is implemented?
> 
> No, there is no priority set for scanning order.
> However, the order in which buses are registered, will modify the order
> in which scans are done.
> 
> Thus, if you change the priority of your registration, you should be
> able to ensure that your scan comes last.
> 

Can we register the bus only when a PCI device match is found at
runtime, e.g. as part of the PCI driver instance initialization?

/Bruce
  
Gaëtan Rivet March 6, 2018, 11:59 a.m. UTC | #5
On Tue, Mar 06, 2018 at 11:36:17AM +0000, Bruce Richardson wrote:
> On Tue, Mar 06, 2018 at 11:46:22AM +0100, Gaëtan Rivet wrote:
> > On Tue, Mar 06, 2018 at 10:42:14AM +0000, Xu, Rosen wrote:
> > > 
> > > 
> > > -----Original Message-----
> > > From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] 
> > > Sent: Tuesday, March 06, 2018 14:20
> > > To: Xu, Rosen <rosen.xu@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>
> > > Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus
> > > 
> > > On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644
> > > > --- a/lib/librte_eal/common/eal_common_bus.c
> > > > +++ b/lib/librte_eal/common/eal_common_bus.c
> > > > @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =
> > > >  rte_bus_scan(void)
> > > >  {
> > > >         int ret;
> > > > -       struct rte_bus *bus = NULL;
> > > > +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;
> > > >
> > > >         TAILQ_FOREACH(bus, &rte_bus_list, next) {
> > > > +               if (!strcmp(bus->name, "ifpga")) {
> > > > +                       ifpga_bus = bus;
> > > > +                       continue;
> > > > +               }
> > > > +
> > > >                 ret = bus->scan();
> > > >                 if (ret)
> > > >                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > > >                                 bus->name);
> > > >         }
> > > >
> > > > +       if (ifpga_bus) {
> > > > +               ret = ifpga_bus->scan();
> > > > +               if (ret)
> > > > +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > > > +                               ifpga_bus->name);
> > > > +       }
> > > > +
> > > 
> > > You are doing this just so that PCI scans are completed *before* ifpga scans?
> > > Rosen: yes
> > > Well, I understand that this certainly is an issue that we can't yet define a priority ordering of bus scans.
> > > 
> > > But, I think what you are require is a simpler:
> > > 
> > > In the file ifpga_bus.c:
> > > 
> > > +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
> > > ...
> > > ...
> > > #define RTE_REGISTER_BUS(nm, bus) \
> > > RTE_INIT_PRIO(businitfn_ ##nm, 110); \
> > > 
> > > If you define your own version of RTE_REGISTER_BUS with the priority number higher, it would be inserted later in the bus list.
> > > rte_register_bus doesn't do any inherent ordering.
> > > This would save the changes you are doing in the lib/librte_eal/common/eal_common_bus.c file.
> > > 
> > > But I think there has to be a better provision of defining priority of bus scans - I am sure when new devices come in, there would be possibility of dependencies as in your case.
> > > Rosen: is the priority scan of bus is implemented?
> > 
> > No, there is no priority set for scanning order.
> > However, the order in which buses are registered, will modify the order
> > in which scans are done.
> > 
> > Thus, if you change the priority of your registration, you should be
> > able to ensure that your scan comes last.
> > 
> 
> Can we register the bus only when a PCI device match is found at
> runtime, e.g. as part of the PCI driver instance initialization?
> 
> /Bruce

Technically, yes. You would append a new bus during rte_bus_probe, so
the linked list would simply have a new node and you would then probe
it. You would need to make sure you scan your bus first, so you would
have some weird conditions (whether you are loaded during probe or
naturally, you'd have to do your scan or not).

However, this seems like a terrible idea. You introduce an edge case
that will need to be carried over in most of the bus API implementation.

This new bus seems like a specialization of the PCI bus. Why not directly
use the PCI bus and have your driver linked to either a rawdev or a vdev,
where you could store your metadata and expose a specialized interface?
  
Xu, Rosen March 15, 2018, 1:17 a.m. UTC | #6
-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com] 
Sent: Tuesday, March 06, 2018 20:00
To: Richardson, Bruce <bruce.richardson@intel.com>
Cc: Xu, Rosen <rosen.xu@intel.com>; Shreyansh Jain <shreyansh.jain@nxp.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>
Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus

On Tue, Mar 06, 2018 at 11:36:17AM +0000, Bruce Richardson wrote:
> On Tue, Mar 06, 2018 at 11:46:22AM +0100, Gaëtan Rivet wrote:
> > On Tue, Mar 06, 2018 at 10:42:14AM +0000, Xu, Rosen wrote:
> > > 
> > > 
> > > -----Original Message-----
> > > From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
> > > Sent: Tuesday, March 06, 2018 14:20
> > > To: Xu, Rosen <rosen.xu@intel.com>
> > > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; 
> > > Zhang, Tianfei <tianfei.zhang@intel.com>
> > > Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel 
> > > FPGA Bus Second Scan, it should be scanned after PCI Bus
> > > 
> > > On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644
> > > > --- a/lib/librte_eal/common/eal_common_bus.c
> > > > +++ b/lib/librte_eal/common/eal_common_bus.c
> > > > @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =
> > > >  rte_bus_scan(void)
> > > >  {
> > > >         int ret;
> > > > -       struct rte_bus *bus = NULL;
> > > > +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;
> > > >
> > > >         TAILQ_FOREACH(bus, &rte_bus_list, next) {
> > > > +               if (!strcmp(bus->name, "ifpga")) {
> > > > +                       ifpga_bus = bus;
> > > > +                       continue;
> > > > +               }
> > > > +
> > > >                 ret = bus->scan();
> > > >                 if (ret)
> > > >                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > > >                                 bus->name);
> > > >         }
> > > >
> > > > +       if (ifpga_bus) {
> > > > +               ret = ifpga_bus->scan();
> > > > +               if (ret)
> > > > +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > > > +                               ifpga_bus->name);
> > > > +       }
> > > > +
> > > 
> > > You are doing this just so that PCI scans are completed *before* ifpga scans?
> > > Rosen: yes
> > > Well, I understand that this certainly is an issue that we can't yet define a priority ordering of bus scans.
> > > 
> > > But, I think what you are require is a simpler:
> > > 
> > > In the file ifpga_bus.c:
> > > 
> > > +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
> > > ...
> > > ...
> > > #define RTE_REGISTER_BUS(nm, bus) \ RTE_INIT_PRIO(businitfn_ ##nm, 
> > > 110); \
> > > 
> > > If you define your own version of RTE_REGISTER_BUS with the priority number higher, it would be inserted later in the bus list.
> > > rte_register_bus doesn't do any inherent ordering.
> > > This would save the changes you are doing in the lib/librte_eal/common/eal_common_bus.c file.
> > > 
> > > But I think there has to be a better provision of defining priority of bus scans - I am sure when new devices come in, there would be possibility of dependencies as in your case.
> > > Rosen: is the priority scan of bus is implemented?
> > 
> > No, there is no priority set for scanning order.
> > However, the order in which buses are registered, will modify the 
> > order in which scans are done.
> > 
> > Thus, if you change the priority of your registration, you should be 
> > able to ensure that your scan comes last.
> > 
> 
> Can we register the bus only when a PCI device match is found at 
> runtime, e.g. as part of the PCI driver instance initialization?
> 
> /Bruce

Technically, yes. You would append a new bus during rte_bus_probe, so the linked list would simply have a new node and you would then probe it. You would need to make sure you scan your bus first, so you would have some weird conditions (whether you are loaded during probe or naturally, you'd have to do your scan or not).

However, this seems like a terrible idea. You introduce an edge case that will need to be carried over in most of the bus API implementation.

This new bus seems like a specialization of the PCI bus. Why not directly use the PCI bus and have your driver linked to either a rawdev or a vdev, where you could store your metadata and expose a specialized interface?
Rosen: pls see my v1 patch, in that patch we don't need to modify rte_bus_scan(), the IFPGA Bus Scen is probed by FPGA PCI Driver.
             The reason wo don't directly use PCI bus is that:
              1. One FPGA PCI Device has more than one AFU bitstream;
              2. Each AFU is a hardware device viewed by DPDK;
              3. Acceleration Driver(like Eth/Crpt) bind to AFU dirver;
              4. We also need to hotplug AFU bitstream in runtime;
--
Gaëtan Rivet
6WIND
  
Xu, Rosen March 15, 2018, 1:29 a.m. UTC | #7
-----Original Message-----
From: Richardson, Bruce 
Sent: Tuesday, March 06, 2018 19:36
To: Ga?tan Rivet <gaetan.rivet@6wind.com>
Cc: Xu, Rosen <rosen.xu@intel.com>; Shreyansh Jain <shreyansh.jain@nxp.com>; dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>
Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel FPGA Bus Second Scan, it should be scanned after PCI Bus

On Tue, Mar 06, 2018 at 11:46:22AM +0100, Gaëtan Rivet wrote:
> On Tue, Mar 06, 2018 at 10:42:14AM +0000, Xu, Rosen wrote:
> > 
> > 
> > -----Original Message-----
> > From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
> > Sent: Tuesday, March 06, 2018 14:20
> > To: Xu, Rosen <rosen.xu@intel.com>
> > Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Zhang, 
> > Tianfei <tianfei.zhang@intel.com>
> > Subject: Re: [dpdk-dev] [RFC 3/4] lib/librte_eal/common: Add Intel 
> > FPGA Bus Second Scan, it should be scanned after PCI Bus
> > 
> > On Tue, Mar 6, 2018 at 7:13 AM, 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..74bfa15 100644
> > > --- a/lib/librte_eal/common/eal_common_bus.c
> > > +++ b/lib/librte_eal/common/eal_common_bus.c
> > > @@ -70,15 +70,27 @@ struct rte_bus_list rte_bus_list =
> > >  rte_bus_scan(void)
> > >  {
> > >         int ret;
> > > -       struct rte_bus *bus = NULL;
> > > +       struct rte_bus *bus = NULL, *ifpga_bus = NULL;
> > >
> > >         TAILQ_FOREACH(bus, &rte_bus_list, next) {
> > > +               if (!strcmp(bus->name, "ifpga")) {
> > > +                       ifpga_bus = bus;
> > > +                       continue;
> > > +               }
> > > +
> > >                 ret = bus->scan();
> > >                 if (ret)
> > >                         RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > >                                 bus->name);
> > >         }
> > >
> > > +       if (ifpga_bus) {
> > > +               ret = ifpga_bus->scan();
> > > +               if (ret)
> > > +                       RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
> > > +                               ifpga_bus->name);
> > > +       }
> > > +
> > 
> > You are doing this just so that PCI scans are completed *before* ifpga scans?
> > Rosen: yes
> > Well, I understand that this certainly is an issue that we can't yet define a priority ordering of bus scans.
> > 
> > But, I think what you are require is a simpler:
> > 
> > In the file ifpga_bus.c:
> > 
> > +RTE_REGISTER_BUS(IFPGA_BUS_NAME, rte_ifpga_bus.bus); <== this
> > ...
> > ...
> > #define RTE_REGISTER_BUS(nm, bus) \
> > RTE_INIT_PRIO(businitfn_ ##nm, 110); \
> > 
> > If you define your own version of RTE_REGISTER_BUS with the priority number higher, it would be inserted later in the bus list.
> > rte_register_bus doesn't do any inherent ordering.
> > This would save the changes you are doing in the lib/librte_eal/common/eal_common_bus.c file.
> > 
> > But I think there has to be a better provision of defining priority of bus scans - I am sure when new devices come in, there would be possibility of dependencies as in your case.
> > Rosen: is the priority scan of bus is implemented?
> 
> No, there is no priority set for scanning order.
> However, the order in which buses are registered, will modify the 
> order in which scans are done.
> 
> Thus, if you change the priority of your registration, you should be 
> able to ensure that your scan comes last.
> 

Can we register the bus only when a PCI device match is found at runtime, e.g. as part of the PCI driver instance initialization?
Rosen: yes, in my v1 patch it's called by FPGA PCI Driver(Rawdev)

/Bruce
  

Patch

diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
index 3e022d5..74bfa15 100644
--- a/lib/librte_eal/common/eal_common_bus.c
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -70,15 +70,27 @@  struct rte_bus_list rte_bus_list =
 rte_bus_scan(void)
 {
 	int ret;
-	struct rte_bus *bus = NULL;
+	struct rte_bus *bus = NULL, *ifpga_bus = NULL;
 
 	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		if (!strcmp(bus->name, "ifpga")) {
+			ifpga_bus = bus;
+			continue;
+		}
+		
 		ret = bus->scan();
 		if (ret)
 			RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
 				bus->name);
 	}
 
+	if (ifpga_bus) {
+		ret = ifpga_bus->scan();
+		if (ret)
+			RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n",
+				ifpga_bus->name);
+	}
+
 	return 0;
 }