From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id 763922B96 for ; Wed, 7 Dec 2016 13:17:42 +0100 (CET) Received: by mail-wm0-f51.google.com with SMTP id g23so165175869wme.1 for ; Wed, 07 Dec 2016 04:17:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=KsaevOWYe6Ncp9mtCJYjRWpEp0a6iJHTiDm6J2wPcvA=; b=IuIsvEMaSrfNt3ZS5tj36gUtidSS/M9KhbbS3HL2F7NRmyz5QuUreUCtgXjRdkNNxB MRDfZdk+VOi58hawlbG64mxHk0Xei16PUL4st/vpz7s9tucB1EJhO/eOzqT0kNz3gOd+ BCdLYQ/m0wbhxYtjMppOtgqpDkwP4JMQCmhHCxSOwdU/8OhSEQcHfOt2kSAtU7rziBGZ 4T5sVKn2bzRxBdytcQYUNeEWPgwSd63wzz7J683bbai0CPmnVre1f7i+nu/BBhP6tTt3 sYDcZKP91GMlnjuvyBB8IpLnexxC9SWx1y5lLlp+IfXJhdihCOQl12aqocyqZqB+l6FV j4mg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=KsaevOWYe6Ncp9mtCJYjRWpEp0a6iJHTiDm6J2wPcvA=; b=Gx7faeAQfAQ4lk60YVY6WdPR7k5XM/Q7h1bU8k07WcO1B/YXGpHGP04UTeqqakYoaH llSFkT/k+3liAQM+uDAph1Ge+oes35W5NInv2yGupOBW2nD/pyEUxHP1A0EgskGJ/oUt oFTw4TRNsXVSi7azBYe6u/vmx1k1M7yhIjmAOM1WUPvJu4vgs038wFyAOw2TS9ZXchti UB8kIT4xfDkrK4jLyja6G6tUL1ZgTkxqvl6kkBDxO1jwL5xcnlLjhjVQeLEhl+A6XF/o CCxKmoi/lVIZXB9lN8SthTelUfVwjiSueP0WPTdGf4avDlNG+JzWZrqVxpo3+74uEKhl gubg== X-Gm-Message-State: AKaTC02e4k54buawRLnjPF1TH4+wsKjZvUyYFh/MZMv98F2Cv2Ao+aMQlYYlVKgAegn229jhOXUetnJ3w5sz/rea X-Received: by 10.28.146.201 with SMTP id u192mr2489567wmd.142.1481113062070; Wed, 07 Dec 2016 04:17:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.19.73 with HTTP; Wed, 7 Dec 2016 04:17:21 -0800 (PST) In-Reply-To: References: <1480846288-2517-1-git-send-email-shreyansh.jain@nxp.com> From: David Marchand Date: Wed, 7 Dec 2016 13:17:21 +0100 Message-ID: To: Shreyansh Jain Cc: "dev@dpdk.org" , Thomas Monjalon Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH 00/13] Introducing EAL Bus-Device-Driver Model X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Dec 2016 12:17:42 -0000 Hello Shreyansh, On Wed, Dec 7, 2016 at 10:55 AM, Shreyansh Jain wrote: > On Wednesday 07 December 2016 02:22 AM, David Marchand wrote: >>> 0002~0003: Introducing the basic Bus model and associated test case >>> 0005: Support insertion of device rather than addition to tail >> >> >> Patch 2 and 5 could be squashed. > > > I deliberately kept them separate. I intent to extend the Patch 5 for > hotplugging. But, if I don't end up adding support for that in this series, > I will merge these two. Fine. >> The constructor priority stuff seems unneeded as long as we use >> explicit reference to a global (or local, did not check) bus symbol >> rather than a runtime lookup. > > > I didn't understand your point here. > IMO, constructor priority (or some other way to handle this) is important. I > faced this issue while verifying it at my end when the drivers were getting > registered before the bus. > > Can you elaborate more on '..use explicit reference to a global...'? The drivers register themselves to a bus using this bus specific api. For pci, this is rte_eal_pci_register(). The pci_bus object must be moved to eal_common_pci.c (we can stil internally expose for bsd / linux specific implementations). Then, rte_eal_pci_register() can add the pci driver to the pci_bus drivers list even if this pci_bus object is not registered yet to the buses list. And no constructor order issue ? >> >> >>> 0004: Add scan and match callbacks for the Bus and updated test case >> >> >> Why do you push back the bus object in the 'scan' method ? >> This method is bus specific which means that the code "knows" the >> object registered with the callback. > > > This 'knows' is the grey area for me. > The bus (for example, PCI) after scanning needs to call > rte_eal_bus_add_device() to link the device in bus's device_list. > > Two options: > 1. Have a global reference to "pci" bus (rte_bus) somewhere in eal_pci.c > 2. Call rte_eal_get_bus() every time someone needs the reference. > 3. C++ style, 'this->'. > > I have taken the 3rd path. It simplifies my code to not assume a handle as > well as not allow for reference fetch calls every now and then. > > As a disadvantage: it means passing this as argument - and some cases > maintaining it as __rte_unused. > > Taking (1) or (2) is not advantageous than this approach. 1) is the simplest one. When you write a pci_scan method and embed it in you pci_bus object, but this pci_scan method still wonders which bus object it is supposed to work on, this is a bit like Schizophrenia ;-). >> Is is that you want to have a single scan method used by multiple buses ? > > > Yes, but only as a use case. For example, platform devices are of various > types - what if we have a south-bound bus over a platform bus. In which > case, a hierarchical bus layout is possible. > But, this is far-fetched idea for now. Well, if you have no usecase at the moment, let's keep it simple, please. >> >>> 0006: Integrate bus scan/match with EAL, without any effective >>> driver >> >> >> Hard to find a right balance in patch splittng, but patch 4 and 6 are >> linked, I would squash them into one. > > > Yes, it is hard and sometimes there is simply no strong rationale for > splitting or merging. This is one of those cases. > My idea was that one patch _only_ introduces Bus services (structures, > functions etc) and another should enable the calls to it from EAL. > In that sense, I still think 4 and 6 should remain separate, may be > consecutive, though. Ok, will see in next version of the patchset. >> >>> 0007: rte_pci_driver->probe replaced with rte_driver->probe >> >> >> This patch is too big, please separate in two patches: eal changes >> then ethdev/driver changes. > > > I don't think that can be done. One change is incomplete without the other. > > Changes to all files are only for rte_pci_driver->probe to rte_driver->probe > movement. EAL changes is to allow rte_eth_dev_pci_probe function after such > a change as rte_driver->probe has different arguments as compared to > rte_pci_driver->probe. The patches won't compile if I split. > > Am I missing something? >> >> Why do you push back the driver object in the 'probe' method ? (idem >> rte_bus->scan). > > > I am assuming you are referring to rte_driver->probe(). > This is being done so that implementations (specific to drivers on a > particular bus) can start extracting the rte_xxx_driver, if need be. > > For example, for e1000/em_ethdev.c, rte_driver->probe() have been set to > rte_eth_dev_pci_probe() which requires rte_pci_driver to work with. In > absence of the rte_driver object, this function cannot call > rte_pci_driver->probe (for example) for driver specific operations. Sorry, I am thinking a step ahead with eth_driver out of the picture. But once eth_driver disappears, I can see no reason to keep this driver in the probe method (Schizophrenia again). -- David Marchand