From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v5 05/12] eal: add probe and remove support for rte_driver Date: Mon, 9 Jan 2017 11:58:27 +0530 Message-ID: <25261ee3-120f-7821-0217-35bfec3bc0fb@nxp.com> References: <1482756644-13726-1-git-send-email-shreyansh.jain@nxp.com> <5088350.GnaeIEgOPh@xps13> <7af56aaf-b4dc-4933-588d-d0c375d23844@nxp.com> <2232398.sT6R6nRinv@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 NAM02-SN1-obe.outbound.protection.outlook.com (mail-eopbgr730045.outbound.protection.outlook.com [40.107.73.45]) by dpdk.org (Postfix) with ESMTP id 2D59A1E2B for ; Mon, 9 Jan 2017 07:25:00 +0100 (CET) In-Reply-To: <2232398.sT6R6nRinv@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 Friday 06 January 2017 08:56 PM, Thomas Monjalon wrote: > 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? Ok. Makes sense as a standardized way of differentiating 'init' from 'probe'. > > If yes, the comment could be changed from "Probe the device" to > "Check and instantiate a device". Now that probe if removed from rte_driver, I think this would no longer be valid. [1] [1] http://dpdk.org/ml/archives/dev/2017-January/054140.html > >>> 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(). I have made some changes on similar lines. Will share them soon. Then we can discuss again. > > >> 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 >