From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v5 05/12] eal: add probe and remove support for rte_driver Date: Fri, 06 Jan 2017 16:26:21 +0100 Message-ID: <2232398.sT6R6nRinv@xps13> References: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com> <5088350.GnaeIEgOPh@xps13> <7af56aaf-b4dc-4933-588d-d0c375d23844@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: david.marchand@6wind.com, dev@dpdk.org To: Shreyansh Jain Return-path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id EACAF214A for ; Fri, 6 Jan 2017 16:26:22 +0100 (CET) Received: by mail-wm0-f50.google.com with SMTP id t79so34126272wmt.0 for ; Fri, 06 Jan 2017 07:26:22 -0800 (PST) In-Reply-To: <7af56aaf-b4dc-4933-588d-d0c375d23844@nxp.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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