From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH] net/failsafe: do not probe device if plugged out Date: Thu, 13 Jul 2017 10:14:39 +0200 Message-ID: <20170713081439.GH11154@bidouze.vm.6wind.com> References: <20170712182812.18404-1-thomas@monjalon.net> <20170712203957.GG11154@bidouze.vm.6wind.com> <1868638.SUpGZpUFpc@xps> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: Thomas Monjalon Return-path: Received: from mail-wm0-f46.google.com (mail-wm0-f46.google.com [74.125.82.46]) by dpdk.org (Postfix) with ESMTP id 8D6042935 for ; Thu, 13 Jul 2017 10:14:49 +0200 (CEST) Received: by mail-wm0-f46.google.com with SMTP id i127so17262723wma.0 for ; Thu, 13 Jul 2017 01:14:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1868638.SUpGZpUFpc@xps> 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 Thu, Jul 13, 2017 at 08:52:33AM +0200, Thomas Monjalon wrote: > 12/07/2017 22:39, Gaëtan Rivet: > > Hi Thomas, > > > > Nice idea. A few remarks below: > > > > On Wed, Jul 12, 2017 at 08:28:12PM +0200, Thomas Monjalon wrote: > > > FOREACH_SUBDEV(sdev, i, dev) { > > > if (sdev->state != DEV_PARSED) > > > continue; > > > da = &sdev->devargs; > > > + > > > > Superfluous line. > > I don't think so :) It is isolating the "skip" block with its comment. > > > > + /* skip plugged out devices */ > > > + if (! first_init > > > + && sdev->cmdline == NULL > > > + && strcmp(da->bus->name, "vdev") != 0) { > > > > Use first_init == false instead of negation. > > && should be at the end of the line instead of the start of the next > > one. > > Yes > > > Indentation is wrong. > > No, the coding style is to put 2 tabs for continuation lines. > > > > + da->bus->scan(); > > > + if (bus->find_device(NULL, cmp_dev_name, da->name) == NULL) > > > + continue; /* device not found */ > > > > da->bus->find_device instead of bus->find_device. > > This function cannot find the device back currently on the PCI bus, > > blocking the plugging of VF. > > > > The PCI bus will scan the VF while no rte_devargs exists to > > describe it within the global list. If the device exists, it will > > detect it, allocate it and then set its name. > > Without any rte_devargs, the name of a PCI device falls back to its > > canonical name (DomBDF instead of BDF). The name comparison with > > da->name can only succeed if the slave was declared using the DomBDF > > format. > > > > The fix is to do a deep copy of the rte_devargs (the API has been > > sent previously with the rte_devargs rework but I have since removed > > it) and insert it using rte_eal_devargs_insert(). This is essentially > > the solution I used for the rte_eal_hotplug_add() fix[1]. > > > > The alternative fix is to propose an API for buses to transform device > > names into their canonical form on demand... And it would certainly only > > be useful for the PCI bus. > > > > The issue is discussed there: > > [1]: http://dpdk.org/ml/archives/dev/2017-July/071155.html > > OK, I was not aware of this exact issue. > So I will wait above fix. The fix above will not solve this issue. If you are waiting for a proper, general fix, I don't think it will arrive anytime soon. I have the rte_eal_devargs_clone and a working version that I can send in-reply-to if you want. But that's an API extension while in RC2 so... -- Gaëtan Rivet 6WIND