From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jianbo Liu Subject: Re: [PATCH 00/13] Introducing EAL Bus-Device-Driver Model Date: Mon, 12 Dec 2016 22:35:43 +0800 Message-ID: References: <1480846288-2517-1-git-send-email-shreyansh.jain@nxp.com> <1697fe66-962d-0848-5e68-615249b52dad@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: David Marchand , "dev@dpdk.org" , Thomas Monjalon To: Shreyansh Jain Return-path: Received: from mail-yb0-f171.google.com (mail-yb0-f171.google.com [209.85.213.171]) by dpdk.org (Postfix) with ESMTP id ECE6837A6 for ; Mon, 12 Dec 2016 15:35:44 +0100 (CET) Received: by mail-yb0-f171.google.com with SMTP id d128so3240554ybh.2 for ; Mon, 12 Dec 2016 06:35:44 -0800 (PST) In-Reply-To: <1697fe66-962d-0848-5e68-615249b52dad@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" Hi Shreyansh, On 7 December 2016 at 21:10, Shreyansh Jain wrote: > On Wednesday 07 December 2016 05:47 PM, David Marchand wrote: >> >> 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. > > > So, in eal_common_bus.c > > --->8--- > > struct rte_bus *global_ptr_to_pci_bus = NULL; > > struct rte_bus pci_bus = { ... }; > > rte_eal_pci_register() { > if (global_ptr_to_pci_bus == NULL) > rte_eal_bus_register(&pci_bus) > else > // continue as if PCI bus is registered > } > > --->8--- > > so, no RTE_REGISTER_BUS()? > > If yes, then RTE_REGISTER_BUS() should also check for an existing > registration for duplication. > > I was banking on a model where bus handlers (or bus drivers) are independent > entities, just like PMDs. So, we have a bus XYZ without any drivers > necessarily based on it. > > By making registration dependent on driver registration, it becomes implicit > that buses don't exist without drivers. > I am not in favor of this - or maybe I lack enough reason for this (about > how it will make framework/PMD life better). > >> >> 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 ;-). > > > :) > This now is linked to the above issue of constructor priority and having a > global bus reference. I don't personally prefer it. > I will still give this a serious thought, though. > I'm also in favor of (3). >> >> >>>> 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. >> How to express the hierarchical bus layout as the bus in your design is more like independent objects to hold drivers and their devices? >> >> Well, if you have no usecase at the moment, let's keep it simple, please. >> > > Ok. > >> >>>> >>>>> 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. > > > Is there anything specific that you are looking for in patchset v2? > I was thinking of: > 0. fixing BSD compilation issue reported by CI > 1. improving the test_pci.c > 2. hotplugging > 3. trying to move PCI to drives/bus/pci/linux/* and resolving how drivers > link to it, and how EAL resources like devargs are consumed. > > Anything else? > >> >> >>>> >>>>> 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). > > > When eth_driver disappears, i was thinking of accomodating the eth_dev_init > into the rte_pci_driver->probe/init. > But, this is still a nascent thought. > I am yet to start working on eth_driver. > >> >> >> >