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 15:43:06 +0530 Message-ID: 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 NAM02-BL2-obe.outbound.protection.outlook.com (mail-bl2nam02on0040.outbound.protection.outlook.com [104.47.38.40]) by dpdk.org (Postfix) with ESMTP id C77FF14EC for ; Tue, 17 Jan 2017 11:09:43 +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" Just an update on things fixed/updated in v7 against these comments: 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. > > 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? > > And if there is a specific patch for scan, probe can be another one? v7 Contains a split patch for introducing probe handler and introducing scan and probe in EAL. > >> +{ >> + 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. > >> +{ >> + 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? > > <...> > >> + >> +/** >> + * 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 Pushed v7 to ML. 'make doc' is not reporting any warn/error now. Thanks for pointing it out. > >> + * >> + * @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); >> + > > <...> > >