Re: [PATCH v2 02/18] eal: remove generic devtype

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hello Aaron,

On Tue, Oct 17, 2017 at 02:16:59PM -0400, Aaron Conole wrote:
> Gaetan Rivet <gaetan.rivet@6wind.com> 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 <gaetan.rivet@6wind.com>
> > ---
< ... >
> > @@ -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