Hi Gaëtan, > -----Original Message----- > From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com] > Sent: Saturday, October 21, 2017 4:08 AM > To: Tan, Jianfeng > Cc: dev@dpdk.org; santosh.shu...@caviumnetworks.com; > jerin.ja...@caviumnetworks.com; Burakov, Anatoly > Subject: Re: [PATCH] pci: fix check uio bind > > 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 <jianfeng....@intel.com> > > >>--- > > >> 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 :)
Make sense! Will send out a new version as per your above suggestions. Thanks, Jianfeng
