From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by dpdk.org (Postfix) with ESMTP id 2ECC41B26E for ; Fri, 20 Oct 2017 22:08:35 +0200 (CEST) Received: by mail-wr0-f196.google.com with SMTP id j15so130419wre.8 for ; Fri, 20 Oct 2017 13:08:35 -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=b7WpV1GsnJNB9xyUvR+O/knUsz4RZ5ibc57QetczMV0=; b=v8dPzy48izO1gvvfENAgWg/91TBMaCXaAhXv5RWCepyynNStXMFps8leuaY6vMedtO 5uX9F27+/+Qh5Ky+rWSlYuyJid+SC1KaIT2WBwqpo8k3SdzeuBZ38hqNEvcit4uhTHm8 4130aH7K06i+oNhLcSwc40CTZJB8bA8s1ohRbykIazhfdbiRYEcQbwrktZOSsf0ZzbEn WyCZ7YLaMOAJokYjfaL/LOdlIlB+XDFuBdGjkS0osrg3uHlbUENdcOiEvIG87dhv1YZY 9W6dHncmvtAMM80kZF2bYbJ522dQyk4ShzVdLbvNaPyccOKpX7Ew8Xf/hOQES18h1b2c 0XAA== 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=b7WpV1GsnJNB9xyUvR+O/knUsz4RZ5ibc57QetczMV0=; b=bhVIHWvqHwmTbgCFWEkop3VAWs6+HmyAL7C65jDqGXmGvawSFpLENMvLGvRc5R3Eu4 ArmU9RwCiTG0Vg7eAtXF5u8vmtQcQZDPneR9cs+XbHb6PecSpcie+ZxfETK2St8TbJfg Wk/EzGGN0nBoyQd9N+P/AUpv5tiErujBUjdMiL/cpGnezlQhyePC+LVkocCTg8Bi094Z WthHzhPb7BPezGwhuEPNT6bZr8wmKAVvNqxgPQEk/07Nx51KTkA4XGFgZYtXmP3msjIZ +mfoXUZI7t/bwVr3Jhs6HQm8ZDu6ytO6octd2BnbftSDSRqu9mtgLIDLZ0fHPcCHri7m ZGwg== X-Gm-Message-State: AMCzsaXTKGb0WZZ0vcl7RiuBiFoLC4Tz/odxnNUj40GlS72py398P44T i7/yTDPXdEYbnTRMyISlFI52lGpC X-Google-Smtp-Source: ABhQp+TaCcb47ycDRm3eKj6D0wAzSO60MTkrYwoCG1E1Z5L4YxeDHICc+95MAA3dd4vF1/FiLBA6Nw== X-Received: by 10.223.196.199 with SMTP id o7mr5381126wrf.119.1508530114724; Fri, 20 Oct 2017 13:08:34 -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 p49sm5426854wrc.61.2017.10.20.13.08.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 Oct 2017 13:08:34 -0700 (PDT) Date: Fri, 20 Oct 2017 22:08:22 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: "Tan, Jianfeng" Cc: dev@dpdk.org, santosh.shukla@caviumnetworks.com, jerin.jacob@caviumnetworks.com, anatoly.burakov@intel.com Message-ID: <20171020200822.GJ3596@bidouze.vm.6wind.com> References: <1508411909-63954-1-git-send-email-jianfeng.tan@intel.com> <20171019114225.GF3596@bidouze.vm.6wind.com> <38a126ec-7a6a-b43f-88b1-67e3b6477369@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <38a126ec-7a6a-b43f-88b1-67e3b6477369@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] pci: fix check uio bind 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: Fri, 20 Oct 2017 20:08:35 -0000 On Sat, Oct 21, 2017 at 12:47:14AM +0800, Tan, Jianfeng wrote: > Hi Gaëtan, > > > On 10/19/2017 7:42 PM, Gaëtan Rivet wrote: > >Hi Jianfeng, > > > >On Thu, Oct 19, 2017 at 11:18:29AM +0000, Jianfeng Tan wrote: > >>When checking if any devices bound to uio, we did not exclud > >>those which are blacklisted (or in the case that a whitelist > >>is specified). > >> > >>This patch fixes it by only checking whitelisted devices. > >> > >>Fixes: 815c7deaed2d ("pci: get IOMMU class on Linux") > >> > >>Signed-off-by: Jianfeng Tan > >>--- > >> lib/librte_eal/linuxapp/eal/eal_pci.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >>diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c > >>index b4dbf95..2b23d67 100644 > >>--- a/lib/librte_eal/linuxapp/eal/eal_pci.c > >>+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c > >>@@ -516,8 +516,26 @@ static inline int > >> pci_one_device_bound_uio(void) > >> { > >> struct rte_pci_device *dev = NULL; > >>+ struct rte_devargs *devargs; > >>+ int check_all = 1; > >>+ int need_check; > >>+ > >>+ if (rte_pci_bus.bus.conf.scan_mode == RTE_BUS_SCAN_WHITELIST) > >>+ check_all = 0; > >> FOREACH_DEVICE_ON_PCIBUS(dev) { > >>+ devargs = dev->device.devargs; > >>+ > >>+ need_check = 0; > >>+ if (check_all) > >Unless I'm mistaken, you will check blacklisted devices as well here. > > Thank you for pointing out this. > > I was referring to rte_pci_probe(), which also only check "probe_all" and > (devargs && RTE_DEV_WHITELISTED); but turns out it double checks the > blacklisted devices in rte_pci_probe_one_driver(). > > I'll fix it. > > >The condition should be something like: > > > >if (check_all && devargs == NULL) > > >Which means that both ifs can be refactored as > > > >if ((check_all ^ (devargs != NULL)) == 0) > > continue; > > > >Removing need_check. But it can be hard to read. > > Yes, I prefer to make it easy to understand. Please let me know if you are > OK with below code (remove check_all): > > FOREACH_DEVICE_ON_PCIBUS(dev) { > devargs = dev->device.devargs; > > need_check = 0; > switch (rte_pci_bus.bus.conf.scan_mode) { > case RTE_BUS_SCAN_UNDEFINED: > need_check = 1; > break; > case RTE_BUS_SCAN_WHITELIST: > if (devargs && devargs->policy == > RTE_DEV_WHITELISTED) > need_check = 1; > break; > case RTE_BUS_SCAN_BLACKLIST: > if (!devargs || devargs->policy != > RTE_DEV_BLACKLISTED) > need_check = 1; > break; > } > > if (!need_check) > continue; > ... I like the switch, two remarks however: 1. The SCAN_UNDEFINED basically means blacklist mode for the PCI bus. This is the reason probe_all was set by testing for WHITELIST mode: either of the other too would thus trigger the blacklist behavior. Thus, I think you could write a fallthrough case for UNDEFINED, that would go into the BLACKLIST mode. 2. For pointers in general I would test against NULL instead of using the unary '!'. I think it is the general policy in DPDK to always explicitly check against the constant value, but I personally think that for booleans like need_check the "not" operator is ok. So I will only highlight the !devargs :) > > Thanks, > Jianfeng -- Gaëtan Rivet 6WIND