From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0041.outbound.protection.outlook.com [104.47.0.41]) by dpdk.org (Postfix) with ESMTP id 84CE52935 for ; Tue, 20 Sep 2016 08:46:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=O5Jpyzd9n0NQT1BuauOrC1iOE44P1h5WOoV9FbvPVh0=; b=G9rnfiwGtZEcC/9AQvu0HXto3Cbmf5otgpOThl3Hgn2F8Ob9/q/6M/cFqTJsGSNtAUaAOIJ04TmfzHv3t30+HodiT/4g+xfUh7IFbvfQ0R3+Ac2YP9+rI99EAMbDsOCytM+sop0jE3DKt4geAi4jNGFYyoRQK1CbJ+T0KxEQSXk= Received: from DB5PR0401MB2054.eurprd04.prod.outlook.com (10.166.11.137) by DB5PR04MB1607.eurprd04.prod.outlook.com (10.164.38.149) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.639.2; Tue, 20 Sep 2016 06:46:41 +0000 Received: from DB5PR0401MB2054.eurprd04.prod.outlook.com ([10.166.11.137]) by DB5PR0401MB2054.eurprd04.prod.outlook.com ([10.166.11.137]) with mapi id 15.01.0639.005; Tue, 20 Sep 2016 06:46:42 +0000 From: Shreyansh Jain To: Jan Viktorin CC: "dev@dpdk.org" , Hemant Agrawal Thread-Topic: [PATCH v3 06/15] eal/soc: implement probing of drivers Thread-Index: AQHSCnZykl4y9OdKlkWZpsSw7DGDTqB8FlWAgASpA6WAASW+UA== Date: Tue, 20 Sep 2016 06:46:31 +0000 Deferred-Delivery: Tue, 20 Sep 2016 06:45:11 +0000 Message-ID: References: <1451682326-5834-1-git-send-email-viktorin@rehivetech.com> <1473410639-10367-1-git-send-email-shreyansh.jain@nxp.com> <1473410639-10367-7-git-send-email-shreyansh.jain@nxp.com> <20160916142703.607722e7@pcviktorin.fit.vutbr.cz> <20160919133404.0f7fad3e@pcviktorin.fit.vutbr.cz> In-Reply-To: <20160919133404.0f7fad3e@pcviktorin.fit.vutbr.cz> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=shreyansh.jain@nxp.com; x-originating-ip: [122.177.155.70] x-ms-office365-filtering-correlation-id: 63447cb5-7c96-4f8a-4beb-08d3e121e168 x-microsoft-exchange-diagnostics: 1; DB5PR04MB1607; 6:Wi7vR9GjZLKX86U+QTxl1v3NdT6DYqjUA+LJZZh6xL6T6irskkbgBiDIjIO1ZGSwikc9SO2oq8KPwyb2Ej90qsNmsT/wO0gSCrI5AhwIA3zYDbyoTWRe/EsMt3p//8vABU9d44PuaBCWf2+1WzU319Tizlmd3NEXvH1pO9py8LKMls03vWlg/ylLGP8YFPXWOqQPmmzuWz3OdfrFKTpZP7iExAPGXAqdHdPimT6tgwCNr8WvqQ3d6Ies3jBNj3BjFZc6eAMYMRygO3Em5QnFepB71zZP+RDFv3Sk7AhdTganviLHT8O8hnBEw9bZBBh5JrG+45f3icfSEZRXG/5HqA==; 5:dgxDXZ9wOHuxU1N+7gFz717sRP5pgqDEQe7kmZ+t3ErFiLW2T8iRLk8FAYDdWB16UR3yTGVbgoUMFCAt9a6Ru1ryALFRh0A32yoDqoJe50YpjPYuxXtmUUVy97HQ6tSWHX1p0xvENmhZDhzt042Sjg==; 24:qw9ywhNjX8WBoIEoKi+P92WqzCLA4cTlQAao/hELczCmAp0GoDh4HdNw91tuB9d2SvcwMfP/+Uq1nSk6yYlCwh/5AG6tuIlG/lrhRmDcB8U=; 7:+Hsnue5yGUHjmlUXj4eFucpFNJljIWzQ5agcCRvibk1InB3OuBOLYYxcuoXBi3ogAYwNi++L8s/xySaC9y8PauH6lqHr5K3ZMYtKclvompvZTnWDjwYNL9ZzCHHDuxx0bAm0s384v1vD/xjg5AuMqRJyiIAjGJJWVzy1P52AcutWDum0v8TZGTIi7E7UEGhenv06fAlNpgzTdVlKMZ9+8m/2EPK6g2TGM9+ROv79Tr3r34cxlBQOdUjiyUlrt3DRVhvYURZNrc0cWbUVVP9epIBb0h3B9w3uD7Kp2VrMTo75p6WIGc2uobgA2NoJ2FYC x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DB5PR04MB1607; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(185117386973197); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040176)(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6055026); SRVR:DB5PR04MB1607; BCL:0; PCL:0; RULEID:; SRVR:DB5PR04MB1607; x-forefront-prvs: 0071BFA85B x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(979002)(6009001)(7916002)(252514010)(189002)(13464003)(199003)(377454003)(76104003)(24454002)(78114003)(51444003)(86362001)(50986999)(54356999)(8936002)(110136003)(9686002)(76176999)(93886004)(74316002)(7846002)(5660300001)(305945005)(33656002)(7696004)(8676002)(81156014)(101416001)(81166006)(3660700001)(189998001)(7736002)(3280700002)(5002640100001)(15395725005)(68736007)(122556002)(87936001)(76576001)(97736004)(15974865002)(66066001)(92566002)(2900100001)(3846002)(19580395003)(15975445007)(2906002)(77096005)(10400500002)(2950100001)(6116002)(106116001)(19580405001)(4326007)(586003)(106356001)(105586002)(102836003)(579004)(559001)(969003)(989001)(999001)(1009001)(1019001); DIR:OUT; SFP:1101; SCL:1; SRVR:DB5PR04MB1607; H:DB5PR0401MB2054.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Sep 2016 06:46:41.8908 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR04MB1607 Subject: Re: [dpdk-dev] [PATCH v3 06/15] eal/soc: implement probing of drivers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Sep 2016 06:46:44 -0000 Hi Jan, > -----Original Message----- > From: Jan Viktorin [mailto:viktorin@rehivetech.com] > Sent: Monday, September 19, 2016 5:04 PM > To: Shreyansh Jain > Cc: dev@dpdk.org; Hemant Agrawal > Subject: Re: [PATCH v3 06/15] eal/soc: implement probing of drivers >=20 > On Mon, 19 Sep 2016 12:17:53 +0530 > Shreyansh Jain wrote: >=20 > > Hi Jan, > > > > On Friday 16 September 2016 05:57 PM, Jan Viktorin wrote: > > > On Fri, 9 Sep 2016 14:13:50 +0530 > > > Shreyansh Jain wrote: > > > > > >> Each SoC PMD registers a set of callback for scanning its own bus/in= fra > and > > >> matching devices to drivers when probe is called. > > >> This patch introduces the infra for calls to SoC scan on > rte_eal_soc_init() > > >> and match on rte_eal_soc_probe(). > > >> > > >> Patch also adds test case for scan and probe. > > >> > > >> Signed-off-by: Jan Viktorin > > >> Signed-off-by: Shreyansh Jain > > >> Signed-off-by: Hemant Agrawal > > >> --- > > >> app/test/test_soc.c | 138 +++++++++++++= +- > > >> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 4 + > > >> lib/librte_eal/common/eal_common_soc.c | 215 > ++++++++++++++++++++++++ > > >> lib/librte_eal/common/include/rte_soc.h | 51 ++++++ > > >> lib/librte_eal/linuxapp/eal/eal.c | 5 + > > >> lib/librte_eal/linuxapp/eal/eal_soc.c | 16 ++ > > >> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 4 + > > >> 7 files changed, 432 insertions(+), 1 deletion(-) > > >> >=20 > [...] >=20 > > > > > > > >> +static void test_soc_scan_dev0_cb(void); > > > > > > Similar here, something like "match_by_name". > > > > > >> +static int test_soc_match_dev0_cb(struct rte_soc_driver *drv, > > >> + struct rte_soc_device *dev); > > > > > > I prefer an empty line here. > > > > Do we really place newlines in function declarations? That doesn't > > really help anything, until and unless some comments are added to those= . > > Anyways, rather than added blank lines, I will add some comments - thos= e > > are indeed misssing. >=20 > It took me a while to parse those lines... If they are logically grouped, > it'd be ok. Comments might be helpful. However, here these are forward > declarations so it's a question whether to put comments here or to the > implementations below. Ok. I will keep this mind in v4. >=20 > > > > > > > > > > > ditto... > > > > Will add comments. > > > > > > > >> +static void test_soc_scan_dev1_cb(void); > > > > > > ditto... > > > > Same here, I prefer comment rather than blank line. > > > > > >=20 > [...] >=20 > > >> > > >> +/* Test Probe (scan and match) functionality */ > > >> +static int > > >> +test_soc_init_and_probe(void) > > > > > > You say to test scan and match. I'd prefer to reflect this in the nam= e > > > of the test. Otherwise, it seems you are testing init and probe which > > > is not true, I think. > > > > I agree. I will update the name of the function. > > > > > > > > Do you test that "match principle works" or that "match functions are= OK" > > > or "match functions are called as expected", ...? > > > > "match functions are called as expected" >=20 > OK, but there is no assert that says "yes, the match function has been > called". > In other words, it is not an automatic test and it does not help to verif= y > that the code is working. >=20 > I think that you should test that a particular match function succeeds or > not. > So again, I don't consider this to be a test. It does not verify anything= . =20 Agree with you. This is not a 'test' in real sense. I will revisit and see = what can be done about this. Probably your method of 'match' succeeds with = 'always_find_device0' style function. >=20 > > 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 >=20 > 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. >=20 > 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. =20 Ok. 'match_always' called after 'always_find_device0' like scan function. S= o, 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 ca= se would explain the user what the intention of the test is, rather than it= s outcome. Is this what you are suggesting?=20 >=20 > > test case simply confirms that a SoC based PMD would be able to >=20 > It does not confirm anything from my point of view. You *always* print > "successful" > at the end of this test (see below). >=20 > > 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. >=20 > So, I'd comment this as something like: >=20 > "mimic rte_eal_soc_init to prepare for the rte_eal_soc_probe" =20 Agree. >=20 > > >> + */ > > >> + 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<=3D>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 tes= t? > > > 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 i= s > > simply a verification of EAL's ability to link the PMD with it > > (scan/match function pointers). >=20 > I am sorry, I disagree. You always print "successful". The only way to fa= il > here is a SIGSEGV or other very serious system failure. But we test > rte_eal_soc_probe > and not system failures. =20 Ok. I am not yet clear how the test case would be created, except what I ha= ve mentioned above that rather than being functional, testcase only explain= s 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 irrelev= ant 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 togethe= r. It doesn't serve any purpose, which you have rightly pointed out. >=20 > > > > > > > >> + return 0; > > >> +} > > >> + > > >> /* save real devices and drivers until the tests finishes */ >=20 > [...] >=20 > > >> diff --git a/lib/librte_eal/common/eal_common_soc.c > b/lib/librte_eal/common/eal_common_soc.c > > >> index 5dcddc5..bb87a67 100644 > > >> --- a/lib/librte_eal/common/eal_common_soc.c > > >> +++ b/lib/librte_eal/common/eal_common_soc.c > > >> @@ -36,6 +36,8 @@ > > >> #include > > >> > > >> #include > > >> +#include > > >> +#include > > >> > > >> #include "eal_private.h" > > >> > > >> @@ -45,6 +47,213 @@ struct soc_driver_list soc_driver_list =3D > > >> struct soc_device_list soc_device_list =3D > > >> TAILQ_HEAD_INITIALIZER(soc_device_list); > > >> > > >> +/* Default SoC device<->Driver match handler function */ > > > > > > I think this comment is redundant. All this is already said in the > rte_soc.h. > > > > Ok. I will remove it from here and if need be, update the rte_soc.h to > > have elaborate comments. > > > > > > > >> +int > > >> +rte_eal_soc_match(struct rte_soc_driver *drv, struct rte_soc_device > *dev) > > >> +{ > > >> + int i, j; > > >> + > > >> + RTE_VERIFY(drv !=3D NULL && drv->id_table !=3D NULL); > > >> + RTE_VERIFY(dev !=3D NULL && dev->id !=3D NULL); > > >> + > > >> + for (i =3D 0; drv->id_table[i].compatible; ++i) { > > >> + const char *drv_compat =3D drv->id_table[i].compatible; > > >> + > > >> + for (j =3D 0; dev->id[j].compatible; ++j) { > > >> + const char *dev_compat =3D dev->id[j].compatible; > > >> + > > >> + if (!strcmp(drv_compat, dev_compat)) > > >> + return 0; > > >> + } > > >> + } > > >> + > > >> + return 1; > > >> +} > > >> + >=20 > A redundant empty line here... Yes, I will remove this.=20 >=20 > > >> + > > >> +static int > > >> +rte_eal_soc_probe_one_driver(struct rte_soc_driver *drv, > > >> + struct rte_soc_device *dev) > > >> +{ > > >> + int ret =3D 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. >=20 > Well, yes. It seems to be redundant. However, it would emphesize the fact > that this function expects that match_fn is set. >=20 > In the rte_eal_soc_register, the RTE_VERIFY says "The API requires those"= . >=20 > 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. >=20 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 wheth= er 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. > > > > > > > >> + ret =3D 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 do= es > not match". > > > > Problem would be about '' 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 '' 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 > > '' means. Anyways, it is better to discuss these things in that > > other email. >=20 > I am not sure which thread do you mean... Can you point me there, please? =20 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 go= od assumption for a SoC driver or not. Based on that '' (or any other= identifier, like 'compatible'), the log can be printed as you suggested ab= ove. >=20 > > > > > > > >> + return ret; >=20 > [...] >=20 > > >> + > > >> +int > > >> +rte_eal_soc_probe_one(const struct rte_soc_addr *addr) > > >> +{ > > >> + struct rte_soc_device *dev =3D NULL; > > >> + int ret =3D 0; > > >> + > > >> + if (addr =3D=3D NULL) > > >> + return -1; > > >> + > > >> + /* unlike pci, in case of soc, it the responsibility of the soc > driver > > >> + * to check during init whether device has been updated since > last add. > > > > > > Why? Can you give a more detailed explanation? > > > > For this patch, I have _not_ assumed anything for a SoC's > > bus/driver/device model. In absence of a proper standard, each SoC is > > unique - categorizing all SoC under a platform bus, for example, would > > only mean assuming platform bus is a standard. > > Best judge for the layout of SoC devices is the SoC PMD (which is also > > like a bus driver, other than being a device driver). > > > > Once again, if the discussion in other thread comes to a logical > > conclusion, this would get updated. >=20 > Again, I am not sure which thread discusses this topic. Copying the link again: http://dpdk.org/ml/archives/dev/2016-September/0469= 33.html >=20 > I just don't like the idea to leave update responsibility on PMDs. > Maybe, there can be a callback update in rte_soc_device (set or not-set > by the custom scan function) that is to be called here. =20 And for me, the very idea of 'leave update responsibility to PMD' is appeal= ing in case of SoC. Though, I think your suggestion of 'callback' is pretty good. Just to clari= fy, is my understanding correct: PMD registers a callback for 'update' and = EAL would call that when it reaches this code path (of update device inform= ation during probe). The callback can be passed the device found so that PM= D can update that. Is that what you too had in mind? >=20 > > > > > > > >> + */ > > >> + > > >> + TAILQ_FOREACH(dev, &soc_device_list, next) { > > >> + if (rte_eal_compare_soc_addr(&dev->addr, addr)) > > >> + continue; > > >> + > > >> + ret =3D soc_probe_all_drivers(dev); > > >> + if (ret < 0) > > >> + goto err_return; > > >> + return 0; > > >> + } > > >> + return -1; > > >> + > > >> +err_return: > > >> + RTE_LOG(WARNING, EAL, > > >> + "Requested device %s cannot be used\n", addr->name); > > >> + return -1; > > >> +} > > >> + > > >> +/* > > >> + * Scan the SoC devices and call the devinit() function for all > registered > > >> + * drivers that have a matching entry in its id_table for discovere= d > devices. > > >> + */ > > > > > > Should be in header. Here it is redundant. > > > > Ok. I will move to rte_soc.h. > > > > > > > >> +int > > >> +rte_eal_soc_probe(void) > > >> +{ > > >> + struct rte_soc_device *dev =3D NULL; > > >> + int ret =3D 0; > > >> + > > >> + TAILQ_FOREACH(dev, &soc_device_list, next) { > > >> + ret =3D soc_probe_all_drivers(dev); > > >> + if (ret < 0) > > >> + rte_exit(EXIT_FAILURE, "Requested device %s" > > >> + " cannot be used\n", dev->addr.name); > > >> + } > > >> + > > >> + return 0; > > >> +} > > >> + > > >> /* dump one device */ > > >> static int > > >> soc_dump_one_device(FILE *f, struct rte_soc_device *dev) > > >> @@ -79,6 +288,12 @@ rte_eal_soc_dump(FILE *f) > > >> void > > >> rte_eal_soc_register(struct rte_soc_driver *driver) > > >> { > > >> + /* For a valid soc driver, match and scan function > > >> + * should be provided. > > >> + */ > > > > > > This comment should be in the header file. > > > > Actually there is no valueable addition made by this comment. RTE_VERIF= Y > > is self explanatory. I will remove the comment all together. >=20 > No, the comment must be present for the rte_eal_soc_register function as > its documentation. The RTE_VERIFY is not an excuse, it just verifies the > fact that the caller understands the documentation and that she didn't ma= ke > a mistake. Ok. It doesn't hurt to have a comment. I will add that.=20 >=20 > > > > > > > >> + RTE_VERIFY(driver !=3D NULL); > > >> + RTE_VERIFY(driver->match_fn !=3D NULL); > > >> + RTE_VERIFY(driver->scan_fn !=3D NULL); > > >> TAILQ_INSERT_TAIL(&soc_driver_list, driver, next); > > >> } > > >> > > >> diff --git a/lib/librte_eal/common/include/rte_soc.h > b/lib/librte_eal/common/include/rte_soc.h > > >> index c6f98eb..bfb49a2 100644 > > >> --- a/lib/librte_eal/common/include/rte_soc.h > > >> +++ b/lib/librte_eal/common/include/rte_soc.h > > >> @@ -97,6 +97,16 @@ typedef int (soc_devinit_t)(struct rte_soc_driver= *, > struct rte_soc_device *); > > >> typedef int (soc_devuninit_t)(struct rte_soc_device *); > > >> > > >> /** > > >> + * SoC device scan callback, called from rte_eal_soc_init. > > > > > > Can you explain what is the goal of the callback? > > > What is the expected behaviour. > > > > EAL would call the scan of each registered SoC PMD > > (DRIVER_REGISTER_SOC). This scan is responsible for finding devices on > > SoC's specific bus and add them to SoC device_list. This is a callback > > because SoC don't have a generalization like PCI. A SoC is not > > necessarily a platform bus either (what original patch series assumed). >=20 > In doc comment... >=20 > > > > > > > > It returns void so it seems it can never fail. Is this correct? > > > I can image that to scan for devices, I need to check some file-syste= m > > > structure which can be unavailable... > > > > This is what I had in mind: > > That is true, it never fails. It is expected that scan function simply > > ignores (logs error) and moves ahead. A local error for a particular So= C > > (I agree, there might not be more than one SoC) doesn't necessarily mea= n > > that complete DPDK Application should quit. It only means that > > application user should get some error/warning/message about failure. >=20 > I understand, this is OK then. >=20 > > > > > > > >> + */ > > >> +typedef void (soc_scan_t)(void); > > > > > > You are missing the '*' in (*soc_scan_t). > > > > That was put in the definition in the rte_soc_driver - but, I see you > > have already commented there. I will add the '*' here and remove from > there. > > > > > > > >> + > > >> +/** > > >> + * Custom device<=3D>driver match callback for SoC > > > > > > Can you explain the semantics (return values), please? > > > > rte_soc.h already has explanation on the expected semantics over > > rte_eal_soc_match - the default implementation. But, I agree, it should > > be above this declaration. >=20 > True ;). >=20 > > > > > > > >> + */ > > >> +typedef int (soc_match_t)(struct rte_soc_driver *, struct > rte_soc_device *); > > > >=20 > [...] >=20 > > > > > > > > I think, we should tell the users that scan_fn and match_fn must be > always set > > > to something. > > > > How? I think it would be part of documentation, isn't it? >=20 > Yes. It should be documented in the comment for rte_eal_soc_init. > My comment was misplaced a bit... =20 Ok. >=20 > > Also, rte_eal_soc_init() already enforces this check with RTE_VERIFY. > > > > > > > >> const struct rte_soc_id *id_table; /**< ID table, NULL terminated > */ > > >> }; > > >> > > >> @@ -146,6 +158,45 @@ rte_eal_compare_soc_addr(const struct rte_soc_a= ddr > *a0, > > >> } > > >> > > >> /** > > >> + * Default function for matching the Soc driver with device. Each > driver can > > >> + * either use this function or define their own soc matching functi= on. > > >> + * This function relies on the compatible string extracted from sys= fs. > But, > > >> + * a SoC might have different way of identifying its devices. Such = SoC > can > > >> + * override match_fn. > > >> + * > > >> + * @return > > >> + * 0 on success > > >> + * -1 when no match found > > >> + */ > > >> +int > > >> +rte_eal_soc_match(struct rte_soc_driver *drv, struct rte_soc_device > *dev); > > > > > > What about naming it > > > > > > rte_eal_soc_match_default > > > > Ok. > > > > > > > > or maybe better > > > > > > rte_eal_soc_match_compatible > > > > > > what do you think? > > > > From what I had in mind - the discussion about SoC not necessarily > > being a Platform bus - 'compatible' doesn't look fine to me. But again, > > it is still open debate so - I will wait until that is conlcuded. >=20 > Why? The current implementation works this way: >=20 > int > rte_eal_soc_match(struct rte_soc_driver *drv, struct rte_soc_device *dev) > { > int i, j; >=20 > RTE_VERIFY(drv !=3D NULL && drv->id_table !=3D NULL); > RTE_VERIFY(dev !=3D NULL && dev->id !=3D NULL); >=20 > for (i =3D 0; drv->id_table[i].compatible; ++i) { > const char *drv_compat =3D drv->id_table[i].compatible; >=20 > for (j =3D 0; dev->id[j].compatible; ++j) { > const char *dev_compat =3D dev->id[j].compatible; >=20 > if (!strcmp(drv_compat, dev_compat)) > return 0; > } > } >=20 > return 1; > } >=20 > It checks for compatible. So why not to name it that way? If you provide > a match testing the name of devices then it can be named *_match_name. =20 I think all the confusion is about whether match on compatible string shoul= d be kept as default or not. My opinion: No. Because I have a usecase where the SoC PMD is not a platform bus. [1] By keeping a 'compatible' string based match, it would be obvious to have a= scan for /sys/bus/platform. That in itself is based on assumption that a S= oC is platform bus and would adhere to generally known semantics of a platf= orm bus. Other the other hand, as Hemant has suggested in [1], plan is to have a ske= letal code for SoC framework - one which allows any SoC PMD to be plugged i= n irrespective of the bus/device design. Thereafter, when more PMDs become = part of the DPDK framework, generalize functions (which is where your imple= mentations of scan/match over /sys/bus/platform fit pretty well). [1] http://dpdk.org/ml/archives/dev/2016-September/046967.html >=20 > > > > > > > >> + > > >> +/** > > >> + * Probe SoC devices for registered drivers. > > >> + */ > > >> +int rte_eal_soc_probe(void); > > >> + > > >> +/** > > >> + * Probe the single SoC device. > > >> + */ > > >> +int rte_eal_soc_probe_one(const struct rte_soc_addr *addr); > > >> + > > >> +/** > > >> + * Close the single SoC device. > > >> + * > > >> + * Scan the SoC devices and find the SoC device specified by the So= C > > >> + * address, then call the devuninit() function for registered drive= r > > >> + * that has a matching entry in its id_table for discovered device. > > >> + * > > >> + * @param addr > > >> + * The SoC address to close. > > >> + * @return > > >> + * - 0 on success. > > >> + * - Negative on error. > > >> + */ > > >> +int rte_eal_soc_detach(const struct rte_soc_addr *addr); > > >> + > > >> +/** > > >> * Dump discovered SoC devices. > > >> */ > > >> void rte_eal_soc_dump(FILE *f); > > >> diff --git a/lib/librte_eal/linuxapp/eal/eal.c > b/lib/librte_eal/linuxapp/eal/eal.c > > >> index 15c8c3d..147b601 100644 > > >> --- a/lib/librte_eal/linuxapp/eal/eal.c > > >> +++ b/lib/librte_eal/linuxapp/eal/eal.c > > >> @@ -70,6 +70,7 @@ > > >> #include > > >> #include > > >> #include > > >> +#include > > >> #include > > >> #include > > >> #include > > >> @@ -881,6 +882,10 @@ rte_eal_init(int argc, char **argv) > > >> if (rte_eal_pci_probe()) > > >> rte_panic("Cannot probe PCI\n"); > > >> > > >> + /* Probe & Initialize SoC devices */ > > >> + if (rte_eal_soc_probe()) > > >> + rte_panic("Cannot probe SoC\n"); > > >> + > > >> rte_eal_mcfg_complete(); > > >> > > >> return fctret; > > >> diff --git a/lib/librte_eal/linuxapp/eal/eal_soc.c > b/lib/librte_eal/linuxapp/eal/eal_soc.c > > >> index 04848b9..5f961c4 100644 > > >> --- a/lib/librte_eal/linuxapp/eal/eal_soc.c > > >> +++ b/lib/librte_eal/linuxapp/eal/eal_soc.c > > >> @@ -52,5 +52,21 @@ > > >> int > > >> rte_eal_soc_init(void) > > >> { > > >> + struct rte_soc_driver *drv; > > >> + > > >> + /* for debug purposes, SoC can be disabled */ > > >> + if (internal_config.no_soc) > > >> + return 0; > > >> + > > >> + /* For each registered driver, call their scan routine to perform > any > > >> + * custom scan for devices (for example, custom buses) > > >> + */ > > >> + TAILQ_FOREACH(drv, &soc_driver_list, next) { > > > > > > Is it possible to have drv->scan_fn =3D=3D NULL? I suppose, this is i= nvalid. > > > I'd prefer to have RTE_VERIFY for this check. > > > > rte_eal_soc_init() has this check already. Driver wouldn't even be > > registered in case scan/match are not implemented. >=20 > True, but when reviewing or refactoring, you cannot see always all this > context. > It is more defensive to explain here "don't worry, the scan_fn is always = set > here". =20 Ok. I will add this. >=20 > > > > > > > >> + 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. >=20 > ;) =20 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 m= atch/scan', 'platform or not platform dependent SoC' etc? I think it would = save a lot of ping-pong effort.=20 (For today (20/Sep), I might not be available, but I am OK any other time). >=20 > > > > - > > Shreyansh >=20 >=20 >=20 > -- > Jan Viktorin E-mail: Viktorin@RehiveTech.com > System Architect Web: www.RehiveTech.com > RehiveTech > Brno, Czech Republic Thanks a lot. - Shreyansh