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
