From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 7C78F1B2F2 for ; Tue, 10 Oct 2017 18:00:52 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7FED425791; Tue, 10 Oct 2017 16:00:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7FED425791 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=aconole@redhat.com Received: from dhcp-25-97.bos.redhat.com (unknown [10.18.25.172]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8F4CA779D9; Tue, 10 Oct 2017 16:00:50 +0000 (UTC) From: Aaron Conole To: Shreyansh Jain Cc: Don Provan , Jan Blunck , Thomas Monjalon , dev , Hemant Agrawal References: <20170812102220.27773-1-shreyansh.jain@nxp.com> <2075457.Vvey9mxHue@xps> <10403057.Ll0Xg1E4J1@xps> <83422f57-4c0e-5806-c741-ed5ce10891b0@nxp.com> <83a3c6c6-8d50-8106-7c7f-9b5c8263ce96@nxp.com> Date: Tue, 10 Oct 2017 12:00:49 -0400 In-Reply-To: <83a3c6c6-8d50-8106-7c7f-9b5c8263ce96@nxp.com> (Shreyansh Jain's message of "Tue, 10 Oct 2017 10:30:36 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 10 Oct 2017 16:00:51 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] eal: bus scan and probe never fail 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: Tue, 10 Oct 2017 16:00:52 -0000 Shreyansh Jain writes: > Hello Don, > > On Monday 09 October 2017 11:51 PM, Don Provan wrote: >>> -----Original Message----- >>> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] >>> Sent: Monday, October 09, 2017 4:10 AM >>> To: Jan Blunck ; Thomas Monjalon >>> >>> Cc: dev ; Hemant Agrawal >>> Subject: Re: [dpdk-dev] [PATCH] eal: bus scan and probe never fail >>> >>> ... >>> This is where I have disagreement/doubt. >>> Reporting error code from rte_bus_scan would do two things: >>> >>> 1. rte_eal_init is not designed to ignore/log-only these errors - it >>> would quit initialization. (But, this can be changed) >>> 2. What should rte_eal_init do with this error? rte_bus_scan would have >>> already printed the problematic bus->scan() failure. >> >> These practical problems confirm to me that the failure of a bus >> scan is more of a strategic issue: when asking "which devices can >> I use?", "none" is a perfectly valid answer that does not seem >> like an error to me even when a failed bus scan is the reason for >> that answer. > > I agree with this. > >> >> From the application's point of view, the potential error here >> is that the device it wants to use isn't available. I don't see that >> either the init function or the probe function will have enough >> information to understand that application-level problem, so >> they should leave it to the application to detect it. > > I think I understand you comment but just want to cross check again: > Scan or probe error should simply be ignored by EAL layer and let the > application take stance when it detects that the device it was looking > for is missing. Is my understanding correct? > > I am trying to come a conclusion so that this patch can either be > modified or pushed as it is. If the above understanding is correct, I > don't see any changes required in the patch. Does it make sense to introduce a way to query the results of the various bus types for their status? That way we can give the relevant information to the application if it wants, and make the bus scanning code *always* succeed? This version shouldn't be an ABI breakage, either (confirm?). half-baked below (not tested or suitable - just an example): --- diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c index a30a898..cd1ef1e 100644 --- a/lib/librte_eal/common/eal_common_bus.c +++ b/lib/librte_eal/common/eal_common_bus.c @@ -38,9 +38,23 @@ #include "eal_private.h" +struct rte_bus_failure { + struct rte_bus *bus; + int err; +}; + struct rte_bus_list rte_bus_list = TAILQ_HEAD_INITIALIZER(rte_bus_list); +TAILQ_HEAD(rte_bus_scan_failure_list, rte_bus_failure); +struct rte_bus_scan_failure_list rte_bus_scan_failure_list = + TAILQ_HEAD_INITIALIZER(rte_bus_failure); + +TAILQ_HEAD(rte_bus_probe_failure_list, rte_bus_failure); +struct rte_bus_probe_failure_list rte_bus_probe_failure_list = + TAILQ_HEAD_INITIALIZER(rte_bus_failure); + + void rte_bus_register(struct rte_bus *bus) { @@ -64,6 +78,26 @@ rte_bus_unregister(struct rte_bus *bus) RTE_LOG(DEBUG, EAL, "Unregistered [%s] bus.\n", bus->name); } +static void +rte_bus_append_failed_scan(struct rte_bus *bus, int ret) +{ + struct rte_bus_failure *f = malloc(sizeof(struct rte_bus_failure)); + if (!f) abort(); + f->bus = bus; + f->ret = ret; + TAILQ_INSERT_TAIL(&rte_bus_scan_failure_list, f, next); +} + +static void +rte_bus_append_failed_scan(struct rte_bus *bus, int ret) +{ + struct rte_bus_failure *f = malloc(sizeof(struct rte_bus_failure)); + if (!f) abort(); + f->bus = bus; + f->ret = ret; + TAILQ_INSERT_TAIL(&rte_bus_probe_failure_list, f, next); +} + /* Scan all the buses for registered devices */ int rte_bus_scan(void) @@ -76,13 +110,33 @@ rte_bus_scan(void) if (ret) { RTE_LOG(ERR, EAL, "Scan for (%s) bus failed.\n", bus->name); - return ret; + rte_bus_append_failed_scan(bus, ret); } } return 0; } +/* Seek through scan failures */ +void +rte_bus_scan_errors(rte_bus_error_callback cb) +{ + struct rte_bus_failure *f = NULL; + TAILQ_FOREACH(f, &rte_bus_scan_failure_list, next) { + cb(f->bus, f->ret); + } +} + +/* Seek through probe failures */ +void +rte_bus_probe_errors(rte_bus_error_callback cb) +{ + struct rte_bus_failure *f = NULL; + TAILQ_FOREACH(f, &rte_bus_probe_failure_list, next) { + cb(f->bus, f->ret); + } +} + /* Probe all devices of all buses */ int rte_bus_probe(void) @@ -100,7 +154,7 @@ rte_bus_probe(void) if (ret) { RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", bus->name); - return ret; + rte_bus_append_failed_probe(bus, ret); } } @@ -109,7 +163,7 @@ rte_bus_probe(void) if (ret) { RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name); - return ret; + rte_bus_append_failed_probe(bus, ret); } } diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h index 6fb0834..daddb28 100644 --- a/lib/librte_eal/common/include/rte_bus.h +++ b/lib/librte_eal/common/include/rte_bus.h @@ -231,6 +231,20 @@ void rte_bus_register(struct rte_bus *bus); */ void rte_bus_unregister(struct rte_bus *bus); +typedef void (*rte_bus_error_callback)(struct rte_bus *bus, int err); + +/** + * Search through all buses, invoking cb for each bus which reports scan + * error. + */ +void rte_bus_scan_errors(rte_bus_error_callback cb); + +/** + * Search through all buses, invoking cb for each bus which reports scan + * error. + */ +void rte_bus_probe_errors(rte_bus_error_callback cb); + /** * Scan all the buses. * --