[dpdk-dev,v5,01/12] eal/bus: introduce bus abstraction

Message ID 1482758645-23057-2-git-send-email-shreyansh.jain@nxp.com (mailing list archive)
State Superseded, archived
Headers

Checks

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

Commit Message

Shreyansh Jain Dec. 26, 2016, 1:23 p.m. UTC
  This patch introduces the rte_bus abstraction for devices and drivers in
EAL framework. The model is:
 - One or more buses are connected to a CPU (or core)
 - One or more devices are conneted to a Bus
 - Drivers are running instances which manage one or more devices
 - Bus is responsible for identifying devices (and interrupt propogation)
 - Driver is responsible for initializing the device

This patch adds a 'rte_bus' class which rte_driver and rte_device refer.
This way, each device (rte_xxx_device) would have reference to the bus
it is based on. As well as, each driver (rte_xxx_driver) would have link
to the bus and devices on it for servicing.

                                  __ rte_bus_list
                                 /
                     +----------'---+
                     |rte_bus       |
                     | driver_list------> List of rte_bus specific
                     | device_list----    devices
                     |              | `-> List of rte_bus associated
                     |              |     drivers
                     +--|------|----+
              _________/        \_________
    +--------/----+                     +-\---------------+
    |rte_device   |                     |rte_driver       |
    | rte_bus     |                     | rte_bus         |
    | rte_driver  |                     | ...             |
    | ...         |                     +---------...-----+
    |             |                               |||
    +---||--------+                               |||
        ||                                        |||
        | \                                        \\\
        |  \_____________                           \\\
        |                \                          |||
 +------|---------+ +----|----------+               |||
 |rte_pci_device  | |rte_xxx_device |               |||
 | ....           | | ....          |               |||
 +----------------+ +---------------+              / | \
                                                  /  |  \
                            _____________________/  /    \
                           /                    ___/      \
            +-------------'--+    +------------'---+    +--'------------+
            |rte_pci_driver  |    |rte_vdev_driver |    |rte_xxx_driver |
            | ....           |    | ....           |    | ....          |
            +----------------+    +----------------+    +---------------+

This patch only enables the bus references on rte_driver and rte_driver.
EAL wide global device and driver list continue to exist until an instance
of bus is added in subsequent patches.

This patch also introduces RTE_REGISTER_BUS macro on the lines of
RTE_PMD_REGISTER_XXX. Key difference is that the constructor priority has
been explicitly set to 101 so as to execute bus registration before PMD.

Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 lib/librte_eal/bsdapp/eal/Makefile              |   1 +
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  16 ++
 lib/librte_eal/common/Makefile                  |   2 +-
 lib/librte_eal/common/eal_common_bus.c          | 190 ++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_bus.h         | 177 ++++++++++++++++++++++
 lib/librte_eal/common/include/rte_dev.h         |   2 +
 lib/librte_eal/linuxapp/eal/Makefile            |   1 +
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  16 ++
 8 files changed, 404 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_eal/common/eal_common_bus.c
 create mode 100644 lib/librte_eal/common/include/rte_bus.h
  

Comments

Thomas Monjalon Jan. 3, 2017, 9:52 p.m. UTC | #1
2016-12-26 18:53, Shreyansh Jain:
> +DPDK_17.02 {
> +	global:
> +
> +	rte_bus_list;
> +	rte_eal_bus_add_device;
> +	rte_eal_bus_add_driver;
> +	rte_eal_bus_get;
> +	rte_eal_bus_dump;
> +	rte_eal_bus_register;
> +	rte_eal_bus_insert_device;
> +	rte_eal_bus_remove_device;
> +	rte_eal_bus_remove_driver;
> +	rte_eal_bus_unregister;

I think the prefix can be just rte_bus_ instead of rte_eal_bus_.

> +/** Double linked list of buses */
> +TAILQ_HEAD(rte_bus_list, rte_bus);
> +
> +/* Global Bus list */
> +extern struct rte_bus_list rte_bus_list;

Why the bus list is public?

> +/**
> + * A structure describing a generic bus.
> + */
> +struct rte_bus {
> +	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
> +	struct rte_driver_list driver_list;
> +				     /**< List of all drivers on bus */
> +	struct rte_device_list device_list;
> +				     /**< List of all devices on bus */
> +	const char *name;            /**< Name of the bus */
> +};

I am not convinced we should link a generic bus to drivers and devices.
What do you think of having rte_pci_bus being a rte_bus and linking
with rte_pci_device and rte_pci_driver lists?

I'm thinking to something like that:

struct rte_bus {
	TAILQ_ENTRY(rte_bus) next;
	const char *name;
	rte_bus_scan_t scan;
	rte_bus_match_t match;
};
struct rte_pci_bus {
	struct rte_bus bus;
	struct rte_pci_driver_list pci_drivers;
	struct rte_pci_device_list pci_devices;
};

> +/** Helper for Bus registration. The constructor has higher priority than
> + * PMD constructors
> + */
> +#define RTE_REGISTER_BUS(nm, bus) \
> +static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \
> +{\
> +	(bus).name = RTE_STR(nm);\
> +	rte_eal_bus_register(&bus); \
> +}

By removing the lists from rte_bus as suggested above, do you still need
a priority for this constructor?

>  struct rte_device {
>  	TAILQ_ENTRY(rte_device) next; /**< Next device */
> +	struct rte_bus *bus;          /**< Device connected to this bus */
>  	const struct rte_driver *driver;/**< Associated driver */
>  	int numa_node;                /**< NUMA node connection */
>  	struct rte_devargs *devargs;  /**< Device user arguments */
> @@ -148,6 +149,7 @@ void rte_eal_device_remove(struct rte_device *dev);
>   */
>  struct rte_driver {
>  	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> +	struct rte_bus *bus;           /**< Bus serviced by this driver */
>  	const char *name;                   /**< Driver name. */
>  	const char *alias;              /**< Driver alias. */
>  };

Do we need to know the bus associated to a driver in rte_driver?
Bus and driver are already associated in rte_device.
  
Shreyansh Jain Jan. 6, 2017, 10:31 a.m. UTC | #2
On Wednesday 04 January 2017 03:22 AM, Thomas Monjalon wrote:
> 2016-12-26 18:53, Shreyansh Jain:
>> +DPDK_17.02 {
>> +	global:
>> +
>> +	rte_bus_list;
>> +	rte_eal_bus_add_device;
>> +	rte_eal_bus_add_driver;
>> +	rte_eal_bus_get;
>> +	rte_eal_bus_dump;
>> +	rte_eal_bus_register;
>> +	rte_eal_bus_insert_device;
>> +	rte_eal_bus_remove_device;
>> +	rte_eal_bus_remove_driver;
>> +	rte_eal_bus_unregister;
>
> I think the prefix can be just rte_bus_ instead of rte_eal_bus_.

Ok. I will change these.
I thought as these symbols are part of librte_eal/*, naming should 
reflect that.

>
>> +/** Double linked list of buses */
>> +TAILQ_HEAD(rte_bus_list, rte_bus);
>> +
>> +/* Global Bus list */
>> +extern struct rte_bus_list rte_bus_list;
>
> Why the bus list is public?

I will revisit this. When I made it public, intention was that some
element external to EAL (e.g. drivers/bus) might require to iterate
over this list. But, as of now I don't think any such user/case exists
(except test*).

>
>> +/**
>> + * A structure describing a generic bus.
>> + */
>> +struct rte_bus {
>> +	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
>> +	struct rte_driver_list driver_list;
>> +				     /**< List of all drivers on bus */
>> +	struct rte_device_list device_list;
>> +				     /**< List of all devices on bus */
>> +	const char *name;            /**< Name of the bus */
>> +};
>
> I am not convinced we should link a generic bus to drivers and devices.
> What do you think of having rte_pci_bus being a rte_bus and linking
> with rte_pci_device and rte_pci_driver lists?

This is different from what I had in mind.
You are saying:

  Class: rte_bus
       `-> No object instantiated for this class
  Class: rte_pci_bus inheriting rte_bus
       `-> object instantiated for this class.

Here, rte_bus is being treated as an abstract class which is only 
inherited and rte_pci_bus is the base class which is instantiated.

And I was thinking:

  Class: rte_bus
       `-> Object: pci_bus (defined in */eal/eal_pci.c)

Here, rte_bus is that base class which is instantiated.

I agree that what you are suggesting is inline with current model:
  rte_device -> abstract class (no one instantiates it)
   `-> rte_pci_device -> Base class which inherits rte_device and
                         is instantiated.

When I choose not to create rte_pci_bus, it was because I didn't want 
another indirection in form of rte_bus->rte_pci_bus->object.
There were no 'non-generic' bus functions which were only applicable for 
rte_pci_bus. Eventually, rte_pci_bus ended up being a direct inheritance 
of rte_bus.

>
> I'm thinking to something like that:
>
> struct rte_bus {
> 	TAILQ_ENTRY(rte_bus) next;
> 	const char *name;
> 	rte_bus_scan_t scan;
> 	rte_bus_match_t match;
> };
> struct rte_pci_bus {
> 	struct rte_bus bus;
> 	struct rte_pci_driver_list pci_drivers;
> 	struct rte_pci_device_list pci_devices;
> };

if we go by rte_bus->rte_pci_bus->(instance of rte_pci_bus), above is 
fine. Though, I am in favor of rte_bus->(instance of rte_bus for PCI) 
because I don't see any 'non-generic' information in rte_pci_bus which 
can't be put in rte_bus.

>
>> +/** Helper for Bus registration. The constructor has higher priority than
>> + * PMD constructors
>> + */
>> +#define RTE_REGISTER_BUS(nm, bus) \
>> +static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \
>> +{\
>> +	(bus).name = RTE_STR(nm);\
>> +	rte_eal_bus_register(&bus); \
>> +}
>
> By removing the lists from rte_bus as suggested above, do you still need
> a priority for this constructor?

I think yes.
Even if we have rte_pci_bus as a class, object of rte_bus should be part 
of Bus list *before* registration of a driver (because, driver 
registration searches for bus by name).

(This is assuming that no global PCI/VDEV/XXX bus object is created 
which is directly used within all PCI specific bus operations).

There was another suggestion on list which was to check for existence of 
bus _within_ each driver registration and create/instantiate an object 
in case no bus is registered. I didn't like the approach so I didn't use 
it. From David [1], and me [2]

[1] http://dpdk.org/ml/archives/dev/2016-December/051689.html
[2] http://dpdk.org/ml/archives/dev/2016-December/051698.html


>
>>  struct rte_device {
>>  	TAILQ_ENTRY(rte_device) next; /**< Next device */
>> +	struct rte_bus *bus;          /**< Device connected to this bus */
>>  	const struct rte_driver *driver;/**< Associated driver */
>>  	int numa_node;                /**< NUMA node connection */
>>  	struct rte_devargs *devargs;  /**< Device user arguments */
>> @@ -148,6 +149,7 @@ void rte_eal_device_remove(struct rte_device *dev);
>>   */
>>  struct rte_driver {
>>  	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>> +	struct rte_bus *bus;           /**< Bus serviced by this driver */
>>  	const char *name;                   /**< Driver name. */
>>  	const char *alias;              /**< Driver alias. */
>>  };
>
> Do we need to know the bus associated to a driver in rte_driver?
> Bus and driver are already associated in rte_device.
>
>

Two reasons:
1/ A driver should be associated with a bus so that if required, all bus 
can be directly extracted - even when probing has not been done.
2/ device->driver would only be updated after probe. device->driver->bus 
would not be valid in such cases, if required.

Overall, I don't have objections for rte_bus->rte_pci_bus=>object as 
compared to rte_bus=>PCI-object. But, I would still like to get a final 
confirmation of a more preferred way.

Meanwhile, I will make changes to accommodate this change to save time 
in case rte_pci_bus class is final/preferred method.

-
Shreyansh
  
Thomas Monjalon Jan. 6, 2017, 2:55 p.m. UTC | #3
2017-01-06 16:01, Shreyansh Jain:
> On Wednesday 04 January 2017 03:22 AM, Thomas Monjalon wrote:
> > 2016-12-26 18:53, Shreyansh Jain:
> >> +/**
> >> + * A structure describing a generic bus.
> >> + */
> >> +struct rte_bus {
> >> +	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
> >> +	struct rte_driver_list driver_list;
> >> +				     /**< List of all drivers on bus */
> >> +	struct rte_device_list device_list;
> >> +				     /**< List of all devices on bus */
> >> +	const char *name;            /**< Name of the bus */
> >> +};
> >
> > I am not convinced we should link a generic bus to drivers and devices.
> > What do you think of having rte_pci_bus being a rte_bus and linking
> > with rte_pci_device and rte_pci_driver lists?
> 
> This is different from what I had in mind.
> You are saying:
> 
>   Class: rte_bus
>        `-> No object instantiated for this class
>   Class: rte_pci_bus inheriting rte_bus
>        `-> object instantiated for this class.
> 
> Here, rte_bus is being treated as an abstract class which is only 
> inherited and rte_pci_bus is the base class which is instantiated.
> 
> And I was thinking:
> 
>   Class: rte_bus
>        `-> Object: pci_bus (defined in */eal/eal_pci.c)
> 
> Here, rte_bus is that base class which is instantiated.
> 
> I agree that what you are suggesting is inline with current model:
>   rte_device -> abstract class (no one instantiates it)
>    `-> rte_pci_device -> Base class which inherits rte_device and
>                          is instantiated.

Yes

> When I choose not to create rte_pci_bus, it was because I didn't want 
> another indirection in form of rte_bus->rte_pci_bus->object.
> There were no 'non-generic' bus functions which were only applicable for 
> rte_pci_bus. Eventually, rte_pci_bus ended up being a direct inheritance 
> of rte_bus.
> 
> > I'm thinking to something like that:
> >
> > struct rte_bus {
> > 	TAILQ_ENTRY(rte_bus) next;
> > 	const char *name;
> > 	rte_bus_scan_t scan;
> > 	rte_bus_match_t match;
> > };
> > struct rte_pci_bus {
> > 	struct rte_bus bus;
> > 	struct rte_pci_driver_list pci_drivers;
> > 	struct rte_pci_device_list pci_devices;
> > };
> 
> if we go by rte_bus->rte_pci_bus->(instance of rte_pci_bus), above is 
> fine. Though, I am in favor of rte_bus->(instance of rte_bus for PCI) 
> because I don't see any 'non-generic' information in rte_pci_bus which 
> can't be put in rte_bus.

The lists of drivers and devices are specific to the bus.
Your proposal was to list them as generic rte_driver/rte_device and
cast them. I'm just saying we can directly declare them with the right type,
e.g. rte_pci_driver/rte_pci_device.

In the same logic, the functions probe/remove are specifics for the bus and
should be declared in rte_pci_driver instead of the generic rte_driver.


> >> +/** Helper for Bus registration. The constructor has higher priority than
> >> + * PMD constructors
> >> + */
> >> +#define RTE_REGISTER_BUS(nm, bus) \
> >> +static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \
> >> +{\
> >> +	(bus).name = RTE_STR(nm);\
> >> +	rte_eal_bus_register(&bus); \
> >> +}
> >
> > By removing the lists from rte_bus as suggested above, do you still need
> > a priority for this constructor?
> 
> I think yes.
> Even if we have rte_pci_bus as a class, object of rte_bus should be part 
> of Bus list *before* registration of a driver (because, driver 
> registration searches for bus by name).
> 
> (This is assuming that no global PCI/VDEV/XXX bus object is created 
> which is directly used within all PCI specific bus operations).
> 
> There was another suggestion on list which was to check for existence of 
> bus _within_ each driver registration and create/instantiate an object 
> in case no bus is registered. I didn't like the approach so I didn't use 
> it. From David [1], and me [2]
> 
> [1] http://dpdk.org/ml/archives/dev/2016-December/051689.html
> [2] http://dpdk.org/ml/archives/dev/2016-December/051698.html

OK, we can keep your approach of prioritize bus registrations.
If we see an issue later, we could switch to a bus registration done
when a first driver is registered on the bus.


> >>  struct rte_device {
> >>  	TAILQ_ENTRY(rte_device) next; /**< Next device */
> >> +	struct rte_bus *bus;          /**< Device connected to this bus */
> >>  	const struct rte_driver *driver;/**< Associated driver */
> >>  	int numa_node;                /**< NUMA node connection */
> >>  	struct rte_devargs *devargs;  /**< Device user arguments */
> >> @@ -148,6 +149,7 @@ void rte_eal_device_remove(struct rte_device *dev);
> >>   */
> >>  struct rte_driver {
> >>  	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
> >> +	struct rte_bus *bus;           /**< Bus serviced by this driver */
> >>  	const char *name;                   /**< Driver name. */
> >>  	const char *alias;              /**< Driver alias. */
> >>  };
> >
> > Do we need to know the bus associated to a driver in rte_driver?
> > Bus and driver are already associated in rte_device.
> 
> Two reasons:
> 1/ A driver should be associated with a bus so that if required, all bus 
> can be directly extracted - even when probing has not been done.

I do not understand this need.

> 2/ device->driver would only be updated after probe. device->driver->bus 
> would not be valid in such cases, if required.

We can update device->driver on match.

Please let's do not over-engineer if not needed.
In this case, I think we can skip rte_driver->bus.


> Overall, I don't have objections for rte_bus->rte_pci_bus=>object as 
> compared to rte_bus=>PCI-object. But, I would still like to get a final 
> confirmation of a more preferred way.
> 
> Meanwhile, I will make changes to accommodate this change to save time 
> in case rte_pci_bus class is final/preferred method.

It looks more natural to me to avoid class casting and use specialized classes
when possible. So yes I prefer instantiating rte_pci_bus.
However, I could be wrong, and will consider any argument.

Thanks
  
Shreyansh Jain Jan. 9, 2017, 6:24 a.m. UTC | #4
On Friday 06 January 2017 08:25 PM, Thomas Monjalon wrote:
> 2017-01-06 16:01, Shreyansh Jain:
>> On Wednesday 04 January 2017 03:22 AM, Thomas Monjalon wrote:
>>> 2016-12-26 18:53, Shreyansh Jain:
>>>> +/**
>>>> + * A structure describing a generic bus.
>>>> + */
>>>> +struct rte_bus {
>>>> +	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
>>>> +	struct rte_driver_list driver_list;
>>>> +				     /**< List of all drivers on bus */
>>>> +	struct rte_device_list device_list;
>>>> +				     /**< List of all devices on bus */
>>>> +	const char *name;            /**< Name of the bus */
>>>> +};
>>>
>>> I am not convinced we should link a generic bus to drivers and devices.
>>> What do you think of having rte_pci_bus being a rte_bus and linking
>>> with rte_pci_device and rte_pci_driver lists?
>>
>> This is different from what I had in mind.
>> You are saying:
>>
>>   Class: rte_bus
>>        `-> No object instantiated for this class
>>   Class: rte_pci_bus inheriting rte_bus
>>        `-> object instantiated for this class.
>>
>> Here, rte_bus is being treated as an abstract class which is only
>> inherited and rte_pci_bus is the base class which is instantiated.
>>
>> And I was thinking:
>>
>>   Class: rte_bus
>>        `-> Object: pci_bus (defined in */eal/eal_pci.c)
>>
>> Here, rte_bus is that base class which is instantiated.
>>
>> I agree that what you are suggesting is inline with current model:
>>   rte_device -> abstract class (no one instantiates it)
>>    `-> rte_pci_device -> Base class which inherits rte_device and
>>                          is instantiated.
>
> Yes
>
>> When I choose not to create rte_pci_bus, it was because I didn't want
>> another indirection in form of rte_bus->rte_pci_bus->object.
>> There were no 'non-generic' bus functions which were only applicable for
>> rte_pci_bus. Eventually, rte_pci_bus ended up being a direct inheritance
>> of rte_bus.
>>
>>> I'm thinking to something like that:
>>>
>>> struct rte_bus {
>>> 	TAILQ_ENTRY(rte_bus) next;
>>> 	const char *name;
>>> 	rte_bus_scan_t scan;
>>> 	rte_bus_match_t match;
>>> };
>>> struct rte_pci_bus {
>>> 	struct rte_bus bus;
>>> 	struct rte_pci_driver_list pci_drivers;
>>> 	struct rte_pci_device_list pci_devices;
>>> };
>>
>> if we go by rte_bus->rte_pci_bus->(instance of rte_pci_bus), above is
>> fine. Though, I am in favor of rte_bus->(instance of rte_bus for PCI)
>> because I don't see any 'non-generic' information in rte_pci_bus which
>> can't be put in rte_bus.
>
> The lists of drivers and devices are specific to the bus.
> Your proposal was to list them as generic rte_driver/rte_device and
> cast them. I'm just saying we can directly declare them with the right type,
> e.g. rte_pci_driver/rte_pci_device.

Ok. I get your point. Already changing the code to reflect this.

>
> In the same logic, the functions probe/remove are specifics for the bus and
> should be declared in rte_pci_driver instead of the generic rte_driver.

Yes, I agree with this after above argument.

>
>
>>>> +/** Helper for Bus registration. The constructor has higher priority than
>>>> + * PMD constructors
>>>> + */
>>>> +#define RTE_REGISTER_BUS(nm, bus) \
>>>> +static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \
>>>> +{\
>>>> +	(bus).name = RTE_STR(nm);\
>>>> +	rte_eal_bus_register(&bus); \
>>>> +}
>>>
>>> By removing the lists from rte_bus as suggested above, do you still need
>>> a priority for this constructor?
>>
>> I think yes.
>> Even if we have rte_pci_bus as a class, object of rte_bus should be part
>> of Bus list *before* registration of a driver (because, driver
>> registration searches for bus by name).
>>
>> (This is assuming that no global PCI/VDEV/XXX bus object is created
>> which is directly used within all PCI specific bus operations).
>>
>> There was another suggestion on list which was to check for existence of
>> bus _within_ each driver registration and create/instantiate an object
>> in case no bus is registered. I didn't like the approach so I didn't use
>> it. From David [1], and me [2]
>>
>> [1] http://dpdk.org/ml/archives/dev/2016-December/051689.html
>> [2] http://dpdk.org/ml/archives/dev/2016-December/051698.html
>
> OK, we can keep your approach of prioritize bus registrations.
> If we see an issue later, we could switch to a bus registration done
> when a first driver is registered on the bus.

Thanks for confirmation.

>
>
>>>>  struct rte_device {
>>>>  	TAILQ_ENTRY(rte_device) next; /**< Next device */
>>>> +	struct rte_bus *bus;          /**< Device connected to this bus */
>>>>  	const struct rte_driver *driver;/**< Associated driver */
>>>>  	int numa_node;                /**< NUMA node connection */
>>>>  	struct rte_devargs *devargs;  /**< Device user arguments */
>>>> @@ -148,6 +149,7 @@ void rte_eal_device_remove(struct rte_device *dev);
>>>>   */
>>>>  struct rte_driver {
>>>>  	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
>>>> +	struct rte_bus *bus;           /**< Bus serviced by this driver */
>>>>  	const char *name;                   /**< Driver name. */
>>>>  	const char *alias;              /**< Driver alias. */
>>>>  };
>>>
>>> Do we need to know the bus associated to a driver in rte_driver?
>>> Bus and driver are already associated in rte_device.
>>
>> Two reasons:
>> 1/ A driver should be associated with a bus so that if required, all bus
>> can be directly extracted - even when probing has not been done.
>
> I do not understand this need.

For example, Looping over all drivers for plugging them out. We need to 
know which bus a driver is on so that we can unplug the devices 
associated with the driver on that bus.

>
>> 2/ device->driver would only be updated after probe. device->driver->bus
>> would not be valid in such cases, if required.
>
> We can update device->driver on match.

Yes, we can.

>
> Please let's do not over-engineer if not needed.
> In this case, I think we can skip rte_driver->bus.

Hm, Ok. This was more of prospective step. We can avoid it without much 
impact. I will change the code.

>
>
>> Overall, I don't have objections for rte_bus->rte_pci_bus=>object as
>> compared to rte_bus=>PCI-object. But, I would still like to get a final
>> confirmation of a more preferred way.
>>
>> Meanwhile, I will make changes to accommodate this change to save time
>> in case rte_pci_bus class is final/preferred method.
>
> It looks more natural to me to avoid class casting and use specialized classes
> when possible. So yes I prefer instantiating rte_pci_bus.
> However, I could be wrong, and will consider any argument.

Ok. I will go with your argument - mostly because I am OK either way and 
we can always come back if framework changes are stable.

>
> Thanks
>
  
Ferruh Yigit Jan. 9, 2017, 3:22 p.m. UTC | #5
On 12/26/2016 1:23 PM, Shreyansh Jain wrote:

<...>

> +
> +DPDK_17.02 {
> +	global:
> +
> +	rte_bus_list;
> +	rte_eal_bus_add_device;
> +	rte_eal_bus_add_driver;
> +	rte_eal_bus_get;
> +	rte_eal_bus_dump;
> +	rte_eal_bus_register;

> +	rte_eal_bus_insert_device;

This function added in patch 3/12, it can be good to add this function
into .map file in that patch.

<...>
  
Shreyansh Jain Jan. 10, 2017, 4:07 a.m. UTC | #6
Hello Ferruh,

On Monday 09 January 2017 08:52 PM, Ferruh Yigit wrote:
> On 12/26/2016 1:23 PM, Shreyansh Jain wrote:
>
> <...>
>
>> +
>> +DPDK_17.02 {
>> +	global:
>> +
>> +	rte_bus_list;
>> +	rte_eal_bus_add_device;
>> +	rte_eal_bus_add_driver;
>> +	rte_eal_bus_get;
>> +	rte_eal_bus_dump;
>> +	rte_eal_bus_register;
>
>> +	rte_eal_bus_insert_device;
>
> This function added in patch 3/12, it can be good to add this function
> into .map file in that patch.

Yes, I caught this while rebasing for v6. In fact, now this function 
itself has been removed.
Nevertheless, thanks for reviewing.

>
> <...>
>
>

-
Shreyansh
  

Patch

diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index a15b762..cce99f7 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -78,6 +78,7 @@  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_cpuflags.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_string_fns.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_devargs.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_bus.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_dev.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_options.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_common_thread.c
diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index 2f81f7c..51115f4 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -174,3 +174,19 @@  DPDK_16.11 {
 	rte_eal_vdrv_unregister;
 
 } DPDK_16.07;
+
+DPDK_17.02 {
+	global:
+
+	rte_bus_list;
+	rte_eal_bus_add_device;
+	rte_eal_bus_add_driver;
+	rte_eal_bus_get;
+	rte_eal_bus_dump;
+	rte_eal_bus_register;
+	rte_eal_bus_insert_device;
+	rte_eal_bus_remove_device;
+	rte_eal_bus_remove_driver;
+	rte_eal_bus_unregister;
+
+} DPDK_16.11;
diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index a92c984..0c39414 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -38,7 +38,7 @@  INC += rte_per_lcore.h rte_random.h
 INC += rte_tailq.h rte_interrupts.h rte_alarm.h
 INC += rte_string_fns.h rte_version.h
 INC += rte_eal_memconfig.h rte_malloc_heap.h
-INC += rte_hexdump.h rte_devargs.h rte_dev.h rte_vdev.h
+INC += rte_hexdump.h rte_devargs.h rte_bus.h rte_dev.h rte_vdev.h
 INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
 INC += rte_malloc.h rte_keepalive.h rte_time.h
 
diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
new file mode 100644
index 0000000..01730f8
--- /dev/null
+++ b/lib/librte_eal/common/eal_common_bus.c
@@ -0,0 +1,190 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 NXP
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of NXP nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <inttypes.h>
+#include <sys/queue.h>
+
+#include <rte_bus.h>
+#include <rte_devargs.h>
+#include <rte_debug.h>
+
+#include "eal_private.h"
+
+struct rte_bus_list rte_bus_list =
+	TAILQ_HEAD_INITIALIZER(rte_bus_list);
+
+/** @internal
+ * Add a device to a bus.
+ */
+void
+rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(dev);
+
+	TAILQ_INSERT_TAIL(&bus->device_list, dev, next);
+	dev->bus = bus;
+}
+
+/** @internal
+ * Remove a device from its bus.
+ */
+void
+rte_eal_bus_remove_device(struct rte_device *dev)
+{
+	struct rte_bus *bus;
+
+	RTE_VERIFY(dev);
+	RTE_VERIFY(dev->bus);
+
+	bus = dev->bus;
+	TAILQ_REMOVE(&bus->device_list, dev, next);
+	dev->bus = NULL;
+}
+
+/** @internal
+ * Associate a driver with a bus.
+ */
+void
+rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(drv);
+
+	TAILQ_INSERT_TAIL(&bus->driver_list, drv, next);
+	drv->bus = bus;
+}
+
+/** @internal
+ * Disassociate a driver from bus.
+ */
+void
+rte_eal_bus_remove_driver(struct rte_driver *drv)
+{
+	struct rte_bus *bus;
+
+	RTE_VERIFY(drv);
+	RTE_VERIFY(drv->bus);
+
+	bus = drv->bus;
+	TAILQ_REMOVE(&bus->driver_list, drv, next);
+	drv->bus = NULL;
+}
+
+/**
+ * Get the bus handle using its name
+ */
+struct rte_bus *
+rte_eal_bus_get(const char *bus_name)
+{
+	struct rte_bus *bus;
+
+	RTE_VERIFY(bus_name);
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		RTE_VERIFY(bus->name);
+
+		if (!strcmp(bus_name, bus->name))
+			return bus;
+	}
+
+	/* Unable to find bus requested */
+	return NULL;
+}
+
+/* register a bus */
+void
+rte_eal_bus_register(struct rte_bus *bus)
+{
+	RTE_VERIFY(bus);
+	RTE_VERIFY(bus->name && strlen(bus->name));
+
+	/* Initialize the driver and device list associated with the bus */
+	TAILQ_INIT(&(bus->driver_list));
+	TAILQ_INIT(&(bus->device_list));
+
+	TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
+	RTE_LOG(INFO, EAL, "Registered [%s] bus.\n", bus->name);
+}
+
+/* unregister a bus */
+void
+rte_eal_bus_unregister(struct rte_bus *bus)
+{
+	/* All devices and drivers associated with the bus should have been
+	 * 'device->uninit' and 'driver->remove()' already.
+	 */
+	RTE_VERIFY(TAILQ_EMPTY(&(bus->driver_list)));
+	RTE_VERIFY(TAILQ_EMPTY(&(bus->device_list)));
+
+	/* TODO: For each device, call its rte_device->driver->remove()
+	 * and rte_eal_bus_remove_driver()
+	 */
+
+	TAILQ_REMOVE(&rte_bus_list, bus, next);
+	RTE_LOG(INFO, EAL, "Unregistered [%s] bus.\n", bus->name);
+}
+
+/* dump one bus info */
+static int
+bus_dump_one(FILE *f, struct rte_bus *bus)
+{
+	int ret;
+
+	/* For now, dump only the bus name */
+	ret = fprintf(f, " %s\n", bus->name);
+
+	/* Error in case of inability in writing to stream */
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+void
+rte_eal_bus_dump(FILE *f)
+{
+	int ret;
+	struct rte_bus *bus;
+
+	TAILQ_FOREACH(bus, &rte_bus_list, next) {
+		ret = bus_dump_one(f, bus);
+		if (ret) {
+			RTE_LOG(ERR, EAL, "Unable to write to stream (%d)\n",
+				ret);
+			break;
+		}
+	}
+}
diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
new file mode 100644
index 0000000..ad2873c
--- /dev/null
+++ b/lib/librte_eal/common/include/rte_bus.h
@@ -0,0 +1,177 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2016 NXP
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of NXP nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_BUS_H_
+#define _RTE_BUS_H_
+
+/**
+ * @file
+ *
+ * RTE PMD Bus Abstraction interfaces
+ *
+ * This file exposes APIs and Interfaces for Bus Abstraction over the devices
+ * drivers in EAL.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdio.h>
+#include <sys/queue.h>
+
+#include <rte_log.h>
+#include <rte_dev.h>
+
+/** Double linked list of buses */
+TAILQ_HEAD(rte_bus_list, rte_bus);
+
+/* Global Bus list */
+extern struct rte_bus_list rte_bus_list;
+
+/**
+ * A structure describing a generic bus.
+ */
+struct rte_bus {
+	TAILQ_ENTRY(rte_bus) next;   /**< Next bus object in linked list */
+	struct rte_driver_list driver_list;
+				     /**< List of all drivers on bus */
+	struct rte_device_list device_list;
+				     /**< List of all devices on bus */
+	const char *name;            /**< Name of the bus */
+};
+
+/** @internal
+ * Add a device to a bus.
+ *
+ * @param bus
+ *	Bus on which device is to be added
+ * @param dev
+ *	Device handle
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev);
+
+/** @internal
+ * Remove a device from its bus.
+ *
+ * @param dev
+ *	Device handle to remove
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_remove_device(struct rte_device *dev);
+
+/** @internal
+ * Associate a driver with a bus.
+ *
+ * @param bus
+ *	Bus on which driver is to be added
+ * @param dev
+ *	Driver handle
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv);
+
+/** @internal
+ * Disassociate a driver from its bus.
+ *
+ * @param dev
+ *	Driver handle to remove
+ * @return
+ *	None
+ */
+void
+rte_eal_bus_remove_driver(struct rte_driver *drv);
+
+/**
+ * Register a Bus handler.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be registered.
+ */
+void rte_eal_bus_register(struct rte_bus *bus);
+
+/**
+ * Unregister a Bus handler.
+ *
+ * @param bus
+ *   A pointer to a rte_bus structure describing the bus
+ *   to be unregistered.
+ */
+void rte_eal_bus_unregister(struct rte_bus *bus);
+
+/**
+ * Obtain handle for bus given its name.
+ *
+ * @param bus_name
+ *	Name of the bus handle to search
+ * @return
+ *	Pointer to Bus object if name matches any registered bus object
+ *	NULL, if no matching bus found
+ */
+struct rte_bus *rte_eal_bus_get(const char *bus_name);
+
+/**
+ * Dump information of all the buses registered with EAL.
+ *
+ * @param f
+ *	A valid and open output stream handle
+ *
+ * @return
+ *	 0 in case of success
+ *	!0 in case there is error in opening the output stream
+ */
+void rte_eal_bus_dump(FILE *f);
+
+/** Helper for Bus registration. The constructor has higher priority than
+ * PMD constructors
+ */
+#define RTE_REGISTER_BUS(nm, bus) \
+static void __attribute__((constructor(101), used)) businitfn_ ##nm(void) \
+{\
+	(bus).name = RTE_STR(nm);\
+	rte_eal_bus_register(&bus); \
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_BUS_H */
diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index b17791f..8ac09e0 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -122,6 +122,7 @@  struct rte_driver;
  */
 struct rte_device {
 	TAILQ_ENTRY(rte_device) next; /**< Next device */
+	struct rte_bus *bus;          /**< Device connected to this bus */
 	const struct rte_driver *driver;/**< Associated driver */
 	int numa_node;                /**< NUMA node connection */
 	struct rte_devargs *devargs;  /**< Device user arguments */
@@ -148,6 +149,7 @@  void rte_eal_device_remove(struct rte_device *dev);
  */
 struct rte_driver {
 	TAILQ_ENTRY(rte_driver) next;  /**< Next in list. */
+	struct rte_bus *bus;           /**< Bus serviced by this driver */
 	const char *name;                   /**< Driver name. */
 	const char *alias;              /**< Driver alias. */
 };
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 4e206f0..aa874a5 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -87,6 +87,7 @@  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_cpuflags.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_string_fns.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_devargs.c
+SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_bus.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_dev.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_options.c
 SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += eal_common_thread.c
diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
index 83721ba..abfe93e 100644
--- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
@@ -178,3 +178,19 @@  DPDK_16.11 {
 	rte_eal_vdrv_unregister;
 
 } DPDK_16.07;
+
+DPDK_17.02 {
+	global:
+
+	rte_bus_list;
+	rte_eal_bus_add_device;
+	rte_eal_bus_add_driver;
+	rte_eal_bus_get;
+	rte_eal_bus_dump;
+	rte_eal_bus_register;
+	rte_eal_bus_insert_device;
+	rte_eal_bus_remove_device;
+	rte_eal_bus_remove_driver;
+	rte_eal_bus_unregister;
+
+} DPDK_16.11;