From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id B1456377E for ; Tue, 18 Jul 2017 10:39:44 +0200 (CEST) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jul 2017 01:39:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,377,1496127600"; d="scan'208";a="126372200" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.255.134.81]) ([10.255.134.81]) by orsmga005.jf.intel.com with ESMTP; 18 Jul 2017 01:39:40 -0700 To: Thomas Monjalon , =?UTF-8?Q?Ga=c3=abtan_Rivet?= Cc: dev@dpdk.org References: <20170712182812.18404-1-thomas@monjalon.net> <1868638.SUpGZpUFpc@xps> <20170713081439.GH11154@bidouze.vm.6wind.com> <2174554.9RIM1Tt4jP@xps> From: Ferruh Yigit Message-ID: Date: Tue, 18 Jul 2017 09:39:37 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <2174554.9RIM1Tt4jP@xps> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] net/failsafe: do not probe device if plugged out 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, 18 Jul 2017 08:39:46 -0000 On 7/13/2017 10:17 AM, Thomas Monjalon wrote: > 13/07/2017 10:14, Gaëtan Rivet: >> 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... > > No worries, it can be fixed in 17.11. > What is the status of the patch, is this patch rejected for this release, or will be merged into existing failsafe?