[dpdk-dev] [PATCH v3 06/15] eal/soc: implement probing of drivers

[Jan Viktorin]

On Tue, 20 Sep 2016 06:46:31 +0000
Shreyansh Jain <shreyansh.jain at nxp.com> wrote:

> Hi Jan,
> 
> > -----Original Message-----
> > From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> > Sent: Monday, September 19, 2016 5:04 PM
> > To: Shreyansh Jain <shreyansh.jain at nxp.com>
> > Cc: dev at dpdk.org; Hemant Agrawal <hemant.agrawal at nxp.com>
> > Subject: Re: [PATCH v3 06/15] eal/soc: implement probing of drivers
> > 
> > On Mon, 19 Sep 2016 12:17:53 +0530
> > Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
> >   
> > > Hi Jan,

[...]

> 
> >   
> > > The model for the patchset was to allow PMDs to write their own match
> > > and hence, verifying a particular match is not definitive. Rather, the  
> > 
> > If you want to verify a particular match implementation then there should
> > be a particular test verifying that implementation, e.g.
> > test_match_compatible(),
> > test_match_proprietary, test_match_by_name.
> > 
> > However, this is testing the rte_eal_soc_probe (at least, I understand it
> > that way).
> > The probe iterates over devices and drivers and matches them. Thus, the
> > argument
> > "a particular match is not definitive" seems to be irrelevant here. You
> > should build
> > a testing match function like "match_always" that verifies the probe is
> > working. Not
> > that the "match" is working.  
>  
> Ok. 'match_always' called after 'always_find_device0' like scan function. So, essentially rather than a functional match, the testcase only checks if these handlers can be called or not. The naming of such handlers in test case would explain the user what the intention of the test is, rather than its outcome. Is this what you are suggesting? 

Yes, it seems to be ;).

> 
> >   
> > > test case simply confirms that a SoC based PMD would be able to  
> > 
> > It does not confirm anything from my point of view. You *always* print
> > "successful"
> > at the end of this test (see below).
> >   
> > > implement its own match/scan and these would be called from EAL as  
> > expected.  
> > >  
> > > >  
> > > >> +{
> > > >> +	struct rte_soc_driver *drv;
> > > >> +
> > > >> +	/* Registering dummy drivers */
> > > >> +	rte_eal_soc_register(&empty_pmd0.soc_drv);
> > > >> +	rte_eal_soc_register(&empty_pmd1.soc_drv);
> > > >> +	/* Assuming that test_register_unregister is working, not  
> > verifying  
> > > >> +	 * that drivers are indeed registered
> > > >> +	*/
> > > >> +
> > > >> +	/* rte_eal_soc_init is called by rte_eal_init, which in turn  
> > calls the  
> > > >> +	 * scan_fn of each driver.  
> > 
> > So, I'd comment this as something like:
> > 
> > "mimic rte_eal_soc_init to prepare for the rte_eal_soc_probe"  
>  
> Agree.
> 
> >   
> > > >> +	 */
> > > >> +	TAILQ_FOREACH(drv, &soc_driver_list, next) {
> > > >> +		if (drv && drv->scan_fn)
> > > >> +			drv->scan_fn();
> > > >> +	}  
> > > >
> > > > Here, I suppose you mimic the rte_eal_soc_init?  
> > >
> > > Yes.
> > >  
> > > >  
> > > >> +
> > > >> +	/* rte_eal_init() would perform other inits here */
> > > >> +
> > > >> +	/* Probe would link the SoC devices<=>drivers */
> > > >> +	rte_eal_soc_probe();
> > > >> +
> > > >> +	/* Unregistering dummy drivers */
> > > >> +	rte_eal_soc_unregister(&empty_pmd0.soc_drv);
> > > >> +	rte_eal_soc_unregister(&empty_pmd1.soc_drv);
> > > >> +
> > > >> +	free(empty_pmd0.soc_dev.addr.name);
> > > >> +
> > > >> +	printf("%s has been successful\n", __func__);  
> > > >
> > > > How you detect it is unsuccessful? Is it possible to fail in this test?
> > > > A test that can never fail is in fact not a test :).  
> > >
> > > The design assumption for SoC patcheset was: A PMDs scan is called to
> > > find devices on its bus (PMD ~ bus). Whether devices are found or not,
> > > is irrelevant to EAL - whether that is because of error or actually no
> > > devices were available.
> > > With the above logic, no 'success/failure' is checked in the test. It is
> > > simply a verification of EAL's ability to link the PMD with it
> > > (scan/match function pointers).  
> > 
> > I am sorry, I disagree. You always print "successful". The only way to fail
> > here is a SIGSEGV or other very serious system failure. But we test
> > rte_eal_soc_probe
> > and not system failures.  
>  
> Ok. I am not yet clear how the test case would be created, except what I have mentioned above that rather than being functional, testcase only explains the case through function naming.
> The premise on which match/scan are based is that they would be implemented by the respective PMD. Which makes testing of such function either irrelevant or simply a help to user to understand how SoC PMD should be created.
> If this persists, probably I would rather remove this test case all together. It doesn't serve any purpose, which you have rightly pointed out.

I'd vote to have this test to check for regressions. See the test_pci.c, e.g. test_pci_blacklist(). It is not
ideal but it works. You can check how many devices have been matched or seen.

Unfortunately, I am not familiar with DPAA2 API. I assume that it works via /sys as well as platform_device
but with different properties. You can create a fake /sys sub-tree (see app/test/test_pci_sysfs/) and test
whether your scan function reads data successfully and fills all the structures as expected. I think, there
is a lot of space in this area.

Next, is it possible to write a generic DPAA2 backend for the SoC Framework? I mean to not introduce the DPAA2
into PMDs (this looks like mixing of responsibilities) but have it in EAL as an alternative to UIO, VFIO, etc.
I suppose that as DPAA2 is a kind of API, it should be possible to go this way.

> 
> >   
> > >  
> > > >  
> > > >> +	return 0;
> > > >> +}
> > > >> +
> > > >>  /* save real devices and drivers until the tests finishes */  
> > 

[...]

> > >  
> > > >  
> > > >> +int
> > > >> +rte_eal_soc_match(struct rte_soc_driver *drv, struct rte_soc_device  
> > *dev)  
> > > >> +{
> > > >> +	int i, j;
> > > >> +
> > > >> +	RTE_VERIFY(drv != NULL && drv->id_table != NULL);
> > > >> +	RTE_VERIFY(dev != NULL && dev->id != NULL);
> > > >> +
> > > >> +	for (i = 0; drv->id_table[i].compatible; ++i) {
> > > >> +		const char *drv_compat = drv->id_table[i].compatible;
> > > >> +
> > > >> +		for (j = 0; dev->id[j].compatible; ++j) {
> > > >> +			const char *dev_compat = dev->id[j].compatible;
> > > >> +
> > > >> +			if (!strcmp(drv_compat, dev_compat))
> > > >> +				return 0;
> > > >> +		}
> > > >> +	}
> > > >> +
> > > >> +	return 1;
> > > >> +}
> > > >> +  
> > 
> > A redundant empty line here...  
> 
> Yes, I will remove this. 
> 
> >   
> > > >> +
> > > >> +static int
> > > >> +rte_eal_soc_probe_one_driver(struct rte_soc_driver *drv,
> > > >> +			     struct rte_soc_device *dev)
> > > >> +{
> > > >> +	int ret = 1;
> > > >> +  
> > > >
> > > > I think, the RTE_VERIFY(dev->match_fn) might be good here.
> > > > It avoids any doubts about the validity of the pointer.  
> > >
> > > That has already been done in rte_eal_soc_register which is called when
> > > PMDs are registering themselves through DRIVER_REGISTER_SOC. That would
> > > prevent any PMD leaking through to this stage without a proper
> > > match_fn/scan_fn.  
> > 
> > Well, yes. It seems to be redundant. However, it would emphesize the fact
> > that this function expects that match_fn is set.
> > 
> > In the rte_eal_soc_register, the RTE_VERIFY says "The API requires those".
> > 
> > But when I review I do not always see all the context. It is not safe for
> > me to assume that there was probably some RTE_VERIFY in the path... It is
> > not a fast path so it does not hurt the performance in anyway.
> >   
> 
> You mean RTE_VERIFY should be duplicated so that code readability increases?
> I am not really convinced on that.
> My opinion: rte_eal_soc_*probe* series shouldn't actually worry about whether a valid PMD has been plugged in or. It should simply assume that having reached here, all is well on the PMDs definition side.
> 
> On the contrary, placing a RTE_VERIFY here would only state the reader that it is possible to reach this code path without a valid match function.

Yes, it would state that the match function must always be valid here. That's the point.

> 
> > >  
> > > >  
> > > >> +	ret = drv->match_fn(drv, dev);
> > > >> +	if (ret) {
> > > >> +		RTE_LOG(DEBUG, EAL,
> > > >> +			" match function failed, skipping\n");  
> > > >
> > > > Is this a failure? I think it is not. Failure would be if the match
> > > > function cannot execute correctly. This is more like "no-match".  
> > >
> > > The log message is misleading. This is _not_ a failure but simply a
> > > 'no-match'. I will update this.
> > >  
> > > >
> > > > When debugging, I'd like to see more a message like "driver <name> does  
> > not match".  
> > >
> > > Problem would be about '<name>' of a driver. There is already another
> > > discussion about SoC capability/platform bus definitions - probably I
> > > will wait for that so as to define what a '<name>' for a driver and
> > > device is.
> > > In this case, the key reason for not adding such a message was because
> > > it was assumed PMDs are black boxes with EAL not even assuming what
> > > '<name>' means. Anyways, it is better to discuss these things in that
> > > other email.  
> > 
> > I am not sure which thread do you mean... Can you point me there, please?  
>  
> Sorry, somehow I forgot to add that:
> http://dpdk.org/ml/archives/dev/2016-September/046933.html
> 
> One of the discussion point in above thread is whether platform bus is a good assumption for a SoC driver or not. Based on that '<name>' (or any other identifier, like 'compatible'), the log can be printed as you suggested above.

Quite frankly, I don't know. The platform bus seemed the obvious starting point to me when I started working
on this topic. There was no DPAA2 (as far as I know), nothing more suitable. I write custom kernel drivers
as platform devices.

However, if you look into PCI infrastructure you can see that the UIO, VFIO and custom approaches are available.
And there is no PMD-specific scan function. I'd rather go the way it is already done to not diverge a lot from
the base code. Otherwise, we can easily come to a point like "drop DPDK and write again" :).

> 
> >   
> > >  
> > > >  

[...]

> 
> >   
> > >  
> > > >  
> > > >> +		if (drv && drv->scan_fn) {
> > > >> +			drv->scan_fn();
> > > >> +			/* Ignore all errors from this */
> > > >> +		}  
> > > >  
> > > >> +	}
> > > >> +
> > > >>  	return 0;
> > > >>  }
> > > >> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map  
> > b/lib/librte_eal/linuxapp/eal/rte_eal_version.map  
> > > >> index b9d1932..adcfe7d 100644
> > > >> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > > >> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > > >> @@ -179,5 +179,9 @@ DPDK_16.11 {
> > > >>  	rte_eal_soc_register;
> > > >>  	rte_eal_soc_unregister;
> > > >>  	rte_eal_soc_dump;
> > > >> +	rte_eal_soc_match;
> > > >> +	rte_eal_soc_detach;
> > > >> +	rte_eal_soc_probe;
> > > >> +	rte_eal_soc_probe_one;
> > > >>
> > > >>  } DPDK_16.07;  
> > > >
> > > > Regards
> > > > Jan
> > > >  
> > >
> > > I hope I have covered all your comments. That was an exhaustive review.
> > > Thanks a lot for your time.
> > >
> > > Lets work to resolve the architectural issues revolving around SoC
> > > scan/match.  
> > 
> > ;)  
>  
> Phew!
> I am already loosing track of things you have suggested.
> What if we can chat on IRC and resolve the issue surrounding the 'default match/scan', 'platform or not platform dependent SoC' etc? I think it would save a lot of ping-pong effort. 
> (For today (20/Sep), I might not be available, but I am OK any other time).

Sure... write me when you are available. I maybe temporarily away.

> 
> >   
> > >
> > > -
> > > Shreyansh  
> > 
> > 
> > 
> > --
> >    Jan Viktorin                  E-mail: Viktorin at RehiveTech.com
> >    System Architect              Web:    www.RehiveTech.com
> >    RehiveTech
> >    Brno, Czech Republic  
> 
> Thanks a lot.
> 
> -
> Shreyansh



-- 
   Jan Viktorin                  E-mail: Viktorin at RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic