From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH v2 02/18] eal: remove generic devtype Date: Wed, 18 Oct 2017 10:20:18 +0200 Message-ID: <20171018082018.GA3596@bidouze.vm.6wind.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: Aaron Conole Return-path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 07A1B1B5DD for ; Wed, 18 Oct 2017 10:20:35 +0200 (CEST) Received: by mail-wm0-f44.google.com with SMTP id m72so8370153wmc.1 for ; Wed, 18 Oct 2017 01:20:35 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Aaron, On Tue, Oct 17, 2017 at 02:16:59PM -0400, Aaron Conole wrote: > Gaetan Rivet writes: > > > The devtype is now entirely defined by the device bus. As such, it is > > already characterized by the bus identifier within an rte_devargs. > > > > The rte_devtype enum can disappear, along with crutches added during > > this transition. > > > > rte_eal_devargs_type_count becomes useless and is removed. > > > > Signed-off-by: Gaetan Rivet > > --- < ... > > > @@ -380,11 +371,8 @@ rte_pci_probe(void) > > probed++; > > > > devargs = dev->device.devargs; > > - /* probe all or only whitelisted devices */ > > - if (probe_all) > > - ret = pci_probe_all_drivers(dev); > > - else if (devargs != NULL && > > - devargs->policy == RTE_DEV_WHITELISTED) > > + /* probe all or only declared devices */ > > + if (probe_all ^ (devargs != NULL)) > > What is the intent of this? If probe_all is true, and devargs != null, > I think this branch isn't taken. > > Shouldn't this be ||? Maybe I missed something? > Here are the possible choices: probe_all \ devargs !NULL NULL --------------------------+----------------------+----------------------------+ true |blacklist and the |blacklist mode and no | |devargs describes a |devargs, meaning the device | |blacklisted device |is not blacklisted | |--> do not probe |--> do probe | --------------------------+----------------------+----------------------------+ false |whitelist mode and |whitelist mode and no | |whitelisted device |devargs, device has been | |using a devargs |scanned but not whitelisted | |--> do probe |--> do not probe | --------------------------+----------------------+----------------------------+ A XOR is thus the logical expression of this decision. What I could be doubtful about here is that using a xor anywhere can be confusing for some people and it could all be expressed in two simpler ifs. Also, probe_all could be renamed as "blacklist_mode" for example to be more descriptive. But unless I'm mistaken, the logic should be correct. > > ret = pci_probe_all_drivers(dev); > > if (ret < 0) { > > RTE_LOG(ERR, EAL, "Requested device " PCI_PRI_FMT < ... > -- Gaëtan Rivet 6WIND