From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v6 5/8] eal: introduce bus scan and probe in EAL Date: Tue, 17 Jan 2017 10:33:14 +0530 Message-ID: <0d943156-b700-3964-3e4f-9d8083ebd783@nxp.com> References: <1484581107-2025-1-git-send-email-shreyansh.jain@nxp.com> <1484581107-2025-6-git-send-email-shreyansh.jain@nxp.com> <4721b3d8-7374-76e8-d859-61c86a88a314@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Ferruh Yigit , Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0085.outbound.protection.outlook.com [104.47.41.85]) by dpdk.org (Postfix) with ESMTP id 62A325323 for ; Tue, 17 Jan 2017 05:59:50 +0100 (CET) In-Reply-To: <4721b3d8-7374-76e8-d859-61c86a88a314@intel.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" On Tuesday 17 January 2017 01:28 AM, Ferruh Yigit wrote: > On 1/16/2017 3:38 PM, Shreyansh Jain wrote: >> Each bus implementation defines their own callbacks for scanning bus >> and probing devices available on the bus. Enable EAL to call bus specific >> scan and probe functions during DPDK initialization. >> >> Existing PCI scan/probe continues to exist. It will removed in subsequent >> patches. >> >> Signed-off-by: Shreyansh Jain > > <...> > >> +/* Scan all the buses for registering devices */ >> +int >> +rte_bus_scan(void) > > I hesitate to make this kind of (not really functional) comments in this > stage of the release, but please feel free to ignore them as your wish. No issues - any review comment is welcome. > > Previous patch is (4/8) for adding bus scan support, so why not this > function (also .map and .h file part of it) added in prev patch? Maybe I didn't get your point well, but this is my take: 4/8 is for introducing the scan callbacks into the Bus layer. There is no work done for EAL yet in 4/8. This patch, 5/8, adds functions (not callbacks) and modifications to EAL for plugging the bus (with its scan/probe) into EAL. > > And if there is a specific patch for scan, probe can be another one? I agree with this. > >> +{ >> + int ret; >> + struct rte_bus *bus = NULL; >> + >> + TAILQ_FOREACH(bus, &rte_bus_list, next) { >> + ret = bus->scan(); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n", >> + bus->name); >> + /* Error in scanning any bus stops the EAL init. */ >> + return ret; >> + } >> + } >> + >> + return 0; >> +} > > <...> > >> diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c >> index 0d799be..35da451 100644 > <...> >> + >> +/* Add a PCI device to PCI Bus */ >> +void >> +rte_eal_pci_add_device(struct rte_pci_bus *pci_bus, >> + struct rte_pci_device *pci_dev) > > I think more generic approach from previous version was better > (rte_eal_bus_add_device()), but I guess they sacrificed for less > modification. There was a lot of offline discussions after the reviews were posted until v5. From the gist of it, I gathered that smaller 'specific' changes are preferred as compared to complete generic approach without an upfront usecase. Also, this change became irrelevant once I moved out the device/driver lists from rte_bus to rte_xxx_bus. This is inline with existing model of (rte_pci_device->rte_device, rte_pci_driver->rte_driver, rte_pci_bus->rte_bus) and having pci_device_list/pci_driver_list private to PCI itself. I guess what is going to happen is that in near future when buses start moving to drivers/bus/*, this kind of generic layer would come in. > >> +{ >> + TAILQ_INSERT_TAIL(&pci_bus->device_list, pci_dev, next); >> + /* Update Bus references */ >> + pci_dev->device.bus = &pci_bus->bus; >> +} >> + > > <...> > >> >> +/** >> + * Structure describing the PCI bus >> + */ >> +struct rte_pci_bus { >> + struct rte_bus bus; /**< Inherit the generic class */ >> + struct rte_pci_device_list device_list; /**< List of PCI devices */ >> + struct rte_pci_driver_list driver_list; /**< List of PCI drivers */ > > Why bus device/driver lists moved from rte_bus? Each bus will need this? > Is it to change as less code as possible? Yes, to move towards a minimal change. Also, in sync with existing 'pci_device_list/pci_driver_list'. > > <...> > >> + >> +/** >> + * Insert a PCI device in the PCI Bus at a particular location in the device >> + * list. It also updates the PCI Bus reference of the new devices to be >> + * inserted. > > Minor issue in document compilation: > > - warning: argument 'pci_dev' of command @param is not found > > - parameter 'new_pci_dev' not documented > > Similar warnings exists for rte_eal_pci_remove_device() too. > > Also following in rte_bus_scan(void) and rte_bus_probe(void) > - warning: argument 'void' of command @param is not found I will fix and post v7 very shortly. > >> + * >> + * @param exist_pci_dev >> + * Existing PCI device in PCI Bus >> + * @param pci_dev >> + * PCI device to be added before exist_pci_dev >> + * @return void >> + */ >> +void rte_eal_pci_insert_device(struct rte_pci_device *exist_pci_dev, >> + struct rte_pci_device *new_pci_dev); >> + > > <...> > >