From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v5 01/12] eal/bus: introduce bus abstraction Date: Fri, 6 Jan 2017 16:01:24 +0530 Message-ID: References: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com> <1482758645-23057-1-git-send-email-shreyansh.jain@nxp.com> <1482758645-23057-2-git-send-email-shreyansh.jain@nxp.com> <12579803.ZHBMo4GdSA@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Thomas Monjalon Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0073.outbound.protection.outlook.com [104.47.33.73]) by dpdk.org (Postfix) with ESMTP id 55F0A2C37 for ; Fri, 6 Jan 2017 11:27:56 +0100 (CET) In-Reply-To: <12579803.ZHBMo4GdSA@xps13> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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