From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f169.google.com (mail-wr0-f169.google.com [209.85.128.169]) by dpdk.org (Postfix) with ESMTP id 66931998C for ; Mon, 31 Jul 2017 16:35:09 +0200 (CEST) Received: by mail-wr0-f169.google.com with SMTP id 12so180381012wrb.1 for ; Mon, 31 Jul 2017 07:35:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=qvWmgrLlfD/eIzhGjeHtDPjHjeGvmQHbZbKTFkbu+CE=; b=TIBq99lPcSnbOZz7S6pIbLxdpcLm68sHtGvvMEBuws7Hs1LxHqHX5MHnv6e/RfMrAI GEHT9kLMpLiMP8HiLSGzML8g1FGc8zUTTEHY6u0xsw0pcGQw76KbGvr8eKV9l/nFY0yb o5XOSQHIv2bs8DbJ29TKVzi/GuAeq27VX5k2qIkKJ4z802S60232HduToitkRDvHH5G4 kvRAoOjuerah+dvJ6fsB87coopgBqwxjE0dvxAu/NWM0ylHbE+zeKTZ+aig2vgVL+v04 3OwluWNMXNuxOu3sZBdduBqNSPjEF4aaf/pv8roVvWSTHLb9O1svANeGmRyYrxn26fXC 5zGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=qvWmgrLlfD/eIzhGjeHtDPjHjeGvmQHbZbKTFkbu+CE=; b=d74ui4WiQwJkdestv5hyrxXmoX1RJR5VWEs3oogNc28/d0tPuzksebZ/RaHMDUHUBG ch0dtu6liNLHqxvF3keH5HHso/zEnnLAVmiNLb1D+UgyJ+BNcJTpHrmU8cEGRIbL3+1E mIDJ27ldH3Q2gU2aehamL5NpYPnxceYuWNtanAWxv+SAQEJ+bamrJewTX1zTPmFEOPIO PSE5l4KOf+ywKXCiqJtJcdGVTnbjVg1ZEDM3bisadcXGsxztkPNA5vS7eLcBbDylnXJO jOUKXYgCtAOtQp5/V0lGCE/YVXNeaxOz4oRTVytJqXu4aE6Y88IOQYls7Rae3rswg6Jp 2C6w== X-Gm-Message-State: AIVw110gpDUYRGO7NK7HKfJ5SYwu/QbMKuZ+RHPqgL/VOZDF/t1j0QPX ald83tZ54ZQcKwbr X-Received: by 10.223.178.85 with SMTP id y21mr12013558wra.92.1501511708898; Mon, 31 Jul 2017 07:35:08 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id v44sm36394211wrb.53.2017.07.31.07.35.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 31 Jul 2017 07:35:07 -0700 (PDT) Date: Mon, 31 Jul 2017 16:34:59 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: gowrishankar muthukrishnan Cc: Declan Doherty , dev@dpdk.org, Thomas Monjalon , Ferruh Yigit Message-ID: <20170731143458.GL11154@bidouze.vm.6wind.com> References: <266cd54d289bfd6e9535a173c9607f0234f8b1b7.1499167396.git.gowrishankar.m@linux.vnet.ibm.com> <2115e096-6c7f-08a1-bec2-1860b5ff11ae@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev 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: Mon, 31 Jul 2017 14:35:09 -0000 Hi Gowrishankar, Declan, On Mon, Jul 10, 2017 at 12:02:24PM +0530, gowrishankar muthukrishnan wrote: > On Friday 07 July 2017 09:08 PM, Declan Doherty wrote: > >On 04/07/2017 12:57 PM, Gowrishankar wrote: > >>From: Gowrishankar Muthukrishnan > >> > >>At present, creating bonding devices using --vdev is broken for PMD like > >>mlx5 as it is neither UIO nor VFIO based and hence PMD driver is unknown > >>to find_port_id_by_pci_addr(), as below. > >> > >>testpmd --vdev 'net_bonding0,mode=1,slave=,socket_id=0' > >> > >>PMD: bond_ethdev_parse_slave_port_kvarg(150) - Invalid slave port value > >> () specified > >>EAL: Failed to parse slave ports for bonded device net_bonding0 > >> > >>This patch fixes parsing PCI ID from bonding device params by verifying > >>it in RTE PCI bus, rather than checking dev->kdrv. > >> > >>Changes: > >> v2 - revisit fix by iterating rte_pci_bus > >> > >>Signed-off-by: Gowrishankar Muthukrishnan > >> > >>--- > >... > >> > > > >Hey Gowrishankar, > > > >I was having a look at this patch and there is the following checkpatch > >error. > > > >_coding style issues_ > > > > > >WARNING:AVOID_EXTERNS: externs should be avoided in .c files > >#48: FILE: drivers/net/bonding/rte_eth_bond_args.c:43: > >+extern struct rte_pci_bus rte_pci_bus; > > > Hi Declan, > Thank you for your review. > Yes, but I also saw some references like above in older code. > > > > >Looking at bit closer at the issue I think there is a simpler solution, > >the bonding driver really shouldn't be parsing the PCI bus directly, and > >since PCI devices use the PCI DBF as their name we can simply replace the > >all the scanning code with a simple call to rte_eth_dev_get_port_by_name > >API. > > I agree that it would be better to be able to use the ether API for this. The issue is that PCI devices are inconsistent regarding their names. The possibility is given to the user to employ the simplified BDF format for PCI device name, instead of the DomBDF format. Unfortunately, the default device name for a PCI device is in the DomBDF format. This means that the name won't match if the device was probed by using the PCI blacklist mode (the default PCI mode). The matching must be refined. > > But you are removing an option to mention ports by PCI addresses right (as > I see parse_port_id() completely removed in your patch) ?. > IMO, we just need to check if given eth pci id (incase we mention ports ib > PCI ID) is one of what EAL scanned in PCI. Also, slaves should not be from > any blacklisted PCI ids (as we test with -b or -w). > Declan is right about the iteration of PCI devices. The device list for the PCI bus is private, the extern declaration to the rte_pci_bus is the telltale sign that there is something wrong in the approach here. In order to respect the new rte_bus logic, I think what you want to achieve can be done by using the rte_bus->find_device with the correct device comparison function. static int pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr) { struct rte_pci_device *pdev; char *addr = _pci_addr; struct rte_pci_addr paddr; static struct rte_bus *pci_bus = NULL; if (pci_bus == NULL) pci_bus = rte_bus_find_by_name("pci"); if (pci_bus->parse(addr, &paddr) != 0) { /* Invalid PCI addr given as input. */ return -1; } pdev = RTE_DEV_TO_PCI(dev); return rte_eal_compare_pci_addr(&pdev->addr, &paddr); } Then verify that you are able to get a device by using it as follows: { struct rte_bus *pci_bus; struct rte_device *dev; pci_bus = rte_bus_find_by_name("pci"); if (pci_bus == NULL) { RTE_LOG(ERR, PMD, "Unable to find PCI bus\n"); return -1; } dev = pci_bus->find_device(NULL, pci_addr_cmp, devname); if (dev == NULL) { RTE_LOG(ERR, PMD, "Unable to find the device %s to enslave.\n", devname); return -EINVAL; } } I hope it's clear enough. You can find examples of use for this API in lib/librte_eal/common/eal_common_dev.c It's a quick implementation to outline the possible direction, I haven't compiled it. It should be refined. For example, the PCI address validation should not be happening in the comparison function, the pci_bus could be matched once instead of twice, etc... But the logic should work. Best regards, -- Gaëtan Rivet 6WIND