From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id CD5121B349 for ; Mon, 23 Oct 2017 05:20:54 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Oct 2017 20:20:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,420,1503385200"; d="scan'208";a="326374960" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 22 Oct 2017 20:20:52 -0700 Received: from fmsmsx124.amr.corp.intel.com (10.18.125.39) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 22 Oct 2017 20:20:52 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx124.amr.corp.intel.com (10.18.125.39) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 22 Oct 2017 20:20:52 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.152]) with mapi id 14.03.0319.002; Mon, 23 Oct 2017 11:20:50 +0800 From: "Tan, Jianfeng" To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , "santosh.shukla@caviumnetworks.com" , "jerin.jacob@caviumnetworks.com" , "Burakov, Anatoly" Thread-Topic: [PATCH] pci: fix check uio bind Thread-Index: AQHTSMvHcDnWbG30AECE92ztD1nbXqLqhvcAgAJtmwD//7IVAIAEIzOw Date: Mon, 23 Oct 2017 03:20:49 +0000 Message-ID: References: <1508411909-63954-1-git-send-email-jianfeng.tan@intel.com> <20171019114225.GF3596@bidouze.vm.6wind.com> <38a126ec-7a6a-b43f-88b1-67e3b6477369@intel.com> <20171020200822.GJ3596@bidouze.vm.6wind.com> In-Reply-To: <20171020200822.GJ3596@bidouze.vm.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Mon, 23 Oct 2017 03:20:55 -0000 Hi Ga=EBtan, > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Saturday, October 21, 2017 4:08 AM > To: Tan, Jianfeng > Cc: dev@dpdk.org; santosh.shukla@caviumnetworks.com; > jerin.jacob@caviumnetworks.com; Burakov, Anatoly > Subject: Re: [PATCH] pci: fix check uio bind >=20 > On Sat, Oct 21, 2017 at 12:47:14AM +0800, Tan, Jianfeng wrote: > > Hi Ga=EBtan, > > > > > > On 10/19/2017 7:42 PM, Ga=EBtan 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 =3D NULL; > > >>+ struct rte_devargs *devargs; > > >>+ int check_all =3D 1; > > >>+ int need_check; > > >>+ > > >>+ if (rte_pci_bus.bus.conf.scan_mode =3D=3D RTE_BUS_SCAN_WHITELIST) > > >>+ check_all =3D 0; > > >> FOREACH_DEVICE_ON_PCIBUS(dev) { > > >>+ devargs =3D dev->device.devargs; > > >>+ > > >>+ need_check =3D 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" a= nd > > (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 =3D=3D NULL) > > > > >Which means that both ifs can be refactored as > > > > > >if ((check_all ^ (devargs !=3D NULL)) =3D=3D 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 =3D dev->device.devargs; > > > > need_check =3D 0; > > switch (rte_pci_bus.bus.conf.scan_mode) { > > case RTE_BUS_SCAN_UNDEFINED: > > need_check =3D 1; > > break; > > case RTE_BUS_SCAN_WHITELIST: > > if (devargs && devargs->policy =3D=3D > > RTE_DEV_WHITELISTED) > > need_check =3D 1; > > break; > > case RTE_BUS_SCAN_BLACKLIST: > > if (!devargs || devargs->policy !=3D > > RTE_DEV_BLACKLISTED) > > need_check =3D 1; > > break; > > } > > > > if (!need_check) > > continue; > > ... >=20 > I like the switch, two remarks however: >=20 > 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. >=20 > Thus, I think you could write a fallthrough case for UNDEFINED, that > would go into the BLACKLIST mode. >=20 > 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 :) Make sense! Will send out a new version as per your above suggestions. Thanks, Jianfeng