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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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.