From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 879AB3254 for ; Tue, 5 Sep 2017 11:13:02 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 10DDC20B5B; Tue, 5 Sep 2017 05:13:02 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Tue, 05 Sep 2017 05:13:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=4YEy8z/4Wm01pxb G74uu5p7AT4VViTYbaymBHAbvjfY=; b=UXPgayFiwdKhJNrhr8ptcKmeIGEnO06 SzwUZnA8L/zhfSpr49RsQLczTzIpjkp9FIhOqnfgo21b12B0dzBKT459+f02sTsk 1zOeGOCAemNzo5ghTYuvV5oOtTCcyW/bR0RPeZ6sJZ1hiBf7WiwlfRxx/bjDPjRq JJDnkjGVPCxY= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=4YEy8z/4Wm01pxbG74uu5p7AT4VViTYbaymBHAbvjfY=; b=T3TBkOuo dqE6PdS6iE+ONazLqjRJcRRHoLKpU1Rl46MCQzEbT/qfiJbL/UITrCoPqb3CBH89 bjF/70lcZhZHyF/gcA6gMKU//Mt9sYvK9DhGGfMEbigjZFa9WxQ2Mopjh88661Rr C7CjpAfQvptWC5wQGYIsyI8W6vMYqK58KtIROdP8NdsYDIOY7dpZobPe0AVcX9th KectBDuqiyptjV3B9neyQ/7WHph0ilBRMkW7w/oog+Dqia0kdzMUpwkr5mDfKYRP 89qb8sld0q+5R1vSwK2qf6y+HjPLPssxMjTegM7Ve1QOooaXtC897bnVLw43fa8T AJWnaMOzuJ910w== X-ME-Sender: X-Sasl-enc: Ll1WLqn5M62Vsru7Ruvt9kdCwqdQJl91J3Zmrx5pcLYP 1504602781 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id AD2632418B; Tue, 5 Sep 2017 05:13:01 -0400 (EDT) From: Thomas Monjalon To: gowrishankar muthukrishnan Cc: dev@dpdk.org, =?ISO-8859-1?Q?Ga=EBtan?= Rivet , Declan Doherty , Ferruh Yigit , rasland@mellanox.com Date: Tue, 05 Sep 2017 11:13:00 +0200 Message-ID: <19317053.0GOQUN8yPN@xps> In-Reply-To: <20170731143458.GL11154@bidouze.vm.6wind.com> References: <20170731143458.GL11154@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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: Tue, 05 Sep 2017 09:13:02 -0000 Ping - any news? 31/07/2017 16:34, Ga=EBtan Rivet: > Hi Gowrishankar, Declan, >=20 > On Mon, Jul 10, 2017 at 12:02:24PM +0530, gowrishankar muthukrishnan wrot= e: > > 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 l= ike > > >>mlx5 as it is neither UIO nor VFIO based and hence PMD driver is unkn= own > > >>to find_port_id_by_pci_addr(), as below. > > >> > > >>testpmd --vdev 'net_bonding0,mode=3D1,slave=3D,socket= _id=3D0' > > >> > > >>PMD: bond_ethdev_parse_slave_port_kvarg(150) - Invalid slave port val= ue > > >> () specified > > >>EAL: Failed to parse slave ports for bonded device net_bonding0 > > >> > > >>This patch fixes parsing PCI ID from bonding device params by verifyi= ng > > >>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. > >=20 > > > > > >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, a= nd > > >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_na= me > > >API. > > > >=20 > I agree that it would be better to be able to use the ether API for > this. >=20 > 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. >=20 > 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). >=20 > The matching must be refined. >=20 > >=20 > > 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 f= rom > > any blacklisted PCI ids (as we test with -b or -w). > >=20 >=20 > 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. >=20 > 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. >=20 > static int > pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr) > { > struct rte_pci_device *pdev; > char *addr =3D _pci_addr; > struct rte_pci_addr paddr; > static struct rte_bus *pci_bus =3D NULL; >=20 > if (pci_bus =3D=3D NULL) > pci_bus =3D rte_bus_find_by_name("pci"); >=20 > if (pci_bus->parse(addr, &paddr) !=3D 0) { > /* Invalid PCI addr given as input. */ > return -1; > } > pdev =3D RTE_DEV_TO_PCI(dev); > return rte_eal_compare_pci_addr(&pdev->addr, &paddr); > } >=20 > Then verify that you are able to get a device by using it as follows: >=20 > { > struct rte_bus *pci_bus; > struct rte_device *dev; >=20 > pci_bus =3D rte_bus_find_by_name("pci"); > if (pci_bus =3D=3D NULL) { > RTE_LOG(ERR, PMD, "Unable to find PCI bus\n"); > return -1; > } > dev =3D pci_bus->find_device(NULL, pci_addr_cmp, devname); > if (dev =3D=3D NULL) { > RTE_LOG(ERR, PMD, "Unable to find the device %s to enslave.\n", > devname); > return -EINVAL; > } > } >=20 > I hope it's clear enough. You can find examples of use for this API in > lib/librte_eal/common/eal_common_dev.c >=20 > It's a quick implementation to outline the possible direction, I > haven't compiled it. It should be refined. >=20 > 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... >=20 > But the logic should work. >=20 > Best regards, >=20