Re: [dpdk-dev] [PATCH v5 05/12] eal: add probe and remove support for rte_driver

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

2017-01-06 17:14, Shreyansh Jain:
> On Wednesday 04 January 2017 03:35 AM, Thomas Monjalon wrote:
> > 2016-12-26 18:53, Shreyansh Jain:
> >> --- a/lib/librte_eal/common/include/rte_dev.h
> >> +++ b/lib/librte_eal/common/include/rte_dev.h
> >> @@ -152,6 +162,8 @@ struct rte_driver {
> >>  	struct rte_bus *bus;           /**< Bus serviced by this driver */
> >>  	const char *name;                   /**< Driver name. */
> >>  	const char *alias;              /**< Driver alias. */
> >> +	driver_probe_t *probe;         /**< Probe the device */
> >> +	driver_remove_t *remove;       /**< Remove/hotplugging the device */
> >>  };
> >
> > If I understand well, this probe function does neither scan nor match.
> > So it could be named init.
> 
> Current model is:
> 
> After scanning for devices and populating bus->device_list,
> Bus probe does:
>   `-> bus->match()
>   `-> rte_driver->probe() for matched driver
> 
> For PCI drivers, '.probe = rte_eal_pci_probe'.
> 
> For example, igb_ethdev.c:
> 
> --->8---
> static struct eth_driver rte_igb_pmd = {
>          .pci_drv = {
>                  .driver = {
>                          .probe = rte_eal_pci_probe,
>                          .remove = rte_eal_pci_remove,
>                  },
> ...
> --->8---

Yes
I'm just having some doubts about the naming "probe" compared to "init".
And yes I know I was advocating to unify naming to "probe" recently :)
I would like to be sure it is not confusing for anyone.
Do you agree that "init" refers to global driver initialization and
"probe" refers to instantiating a device?

If yes, the comment could be changed from "Probe the device" to
"Check and instantiate a device".

> > I think the probe (init) and remove ops must be specific to the bus.
> > We can have them in rte_bus, and as an example, the pci implementation
> > would call the pci probe and remove ops of rte_pci_driver.

I do not understand clearly what I was saying here :/

> So,
> ---
> After scanning for devices (bus->scan()):
> Bus probe (rte_eal_bus_probe()):
>   `-> bus->match()
>   `-> bus->init() - a new fn rte_bus_pci_init()

I suggest the naming bus->probe().
It is currently implemented in rte_eal_pci_probe_one_driver().

>       -> which calls rte_eal_pci_probe()

Not needed here, this function is converted into the PCI match function.

>       -> and rte_pci_driver->probe()

Yes, bus->probe() makes some processing and calls rte_pci_driver->probe().


> and remove rte_driver probe and remove callbacks because they are now
> redundant. (they were added in bus patches itself)
> ---
> 
> Is the above correct understanding of your statement?

I think we just need to move probe/remove in rte_pci_driver.

> Somehow I don't remember why I didn't do this in first place - it seems
> to be better option than introducing a rte_driver->probe()/remove()
> layer. I will change it (and think again why I rejected this idea in
> first place). Thanks.

Thanks