From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id EED59A051D; Wed, 10 Jun 2020 14:06:49 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6FFA35F2F; Wed, 10 Jun 2020 14:06:49 +0200 (CEST) Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by dpdk.org (Postfix) with ESMTP id D015E2B83 for ; Wed, 10 Jun 2020 14:06:47 +0200 (CEST) Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 2DE6F20000F; Wed, 10 Jun 2020 12:06:45 +0000 (UTC) Date: Wed, 10 Jun 2020 14:06:41 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Maxime Coquelin Cc: dev@dpdk.org, david.marchand@redhat.com, wenzhuo.lu@intel.com, beilei.xing@intel.com, bernard.iremonger@intel.com Message-ID: <20200610120641.xtfonyyjahx5wiqo@u256.net> References: <20200608155330.163874-1-maxime.coquelin@redhat.com> <20200608155330.163874-3-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200608155330.163874-3-maxime.coquelin@redhat.com> Subject: Re: [dpdk-dev] [PATCH v20.11 2/2] eal: improve device probing API 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Maxime, On 08/06/20 17:53 +0200, Maxime Coquelin wrote: > This patch makes rte_dev_probe() to return the > rte_device pointer on success and NULL on error > instead of returning 0 on success and negative > value on error. > > The goal is to avoid that the calling application > iterates the devices list afterwards to retrieve > the pointer. Retrieving the pointer is required > for calling rte_dev_remove() later. > I agree with the idea. I recall starting to do it on the legacy functions (rte_eal_hotplug_*), but it was scrapped for API compat. > Signed-off-by: Maxime Coquelin > --- > app/test-pmd/testpmd.c | 2 +- > drivers/net/failsafe/failsafe.c | 5 +++-- > lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++-------- > lib/librte_eal/include/rte_dev.h | 4 ++-- > 4 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 4989d22ca8..f777f449a8 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -2764,7 +2764,7 @@ attach_port(char *identifier) > return; > } > > - if (rte_dev_probe(identifier) < 0) { > + if (rte_dev_probe(identifier) == NULL) { > TESTPMD_LOG(ERR, "Failed to attach port %s\n", identifier); > return; > } > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c > index 72362f35de..e32effdef2 100644 > --- a/drivers/net/failsafe/failsafe.c > +++ b/drivers/net/failsafe/failsafe.c > @@ -341,6 +341,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev) > struct rte_eth_dev *eth_dev; > struct sub_device *sdev; > struct rte_devargs devargs; > + struct rte_device *dev; > uint8_t i; > int ret; > > @@ -378,8 +379,8 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev) > continue; > } > if (!devargs_already_listed(&devargs)) { > - ret = rte_dev_probe(devargs.name); > - if (ret < 0) { > + dev = rte_dev_probe(devargs.name); > + if (dev == NULL) { > ERROR("Failed to probe devargs %s", > devargs.name); > continue; > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c > index 9e4f09d83e..72baae2e48 100644 > --- a/lib/librte_eal/common/eal_common_dev.c > +++ b/lib/librte_eal/common/eal_common_dev.c > @@ -120,7 +120,9 @@ rte_eal_hotplug_add(const char *busname, const char *devname, > if (ret != 0) > return ret; > > - ret = rte_dev_probe(devargs); > + if (rte_dev_probe(devargs) == NULL) > + ret = -1; > + > free(devargs); > > return ret; > @@ -192,7 +194,7 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev) > return ret; > } > > -int > +struct rte_device * > rte_dev_probe(const char *devargs) > { > struct eal_dev_mp_req req; > @@ -212,12 +214,12 @@ rte_dev_probe(const char *devargs) > if (ret != 0) { > RTE_LOG(ERR, EAL, > "Failed to send hotplug request to primary\n"); > - return -ENOMSG; > + return NULL; Is is a problem to keep the ENOMSG through rte_errno? > } > if (req.result != 0) > RTE_LOG(ERR, EAL, > "Failed to hotplug add device\n"); > - return req.result; > + return NULL; > } > > /* attach a shared device from primary start from here: */ > @@ -236,7 +238,7 @@ rte_dev_probe(const char *devargs) > * process. > */ > if (ret != -EEXIST) > - return ret; > + return dev; > } > > /* primary send attach sync request to secondary. */ > @@ -261,11 +263,11 @@ rte_dev_probe(const char *devargs) > > /* for -EEXIST, we don't need to rollback. */ > if (ret == -EEXIST) > - return ret; > + return dev; > goto rollback; > } > > - return 0; > + return dev; > > rollback: > req.t = EAL_DEV_REQ_TYPE_ATTACH_ROLLBACK; > @@ -282,7 +284,7 @@ rte_dev_probe(const char *devargs) > "Failed to rollback device attach on primary." > "Devices in secondary may not sync with primary\n"); > > - return ret; > + return NULL; > } > > int > diff --git a/lib/librte_eal/include/rte_dev.h b/lib/librte_eal/include/rte_dev.h > index c8d985fb5c..9cf7c7fd71 100644 > --- a/lib/librte_eal/include/rte_dev.h > +++ b/lib/librte_eal/include/rte_dev.h > @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname, > * @param devargs > * Device arguments including bus, class and driver properties. > * @return > - * 0 on success, negative on error. > + * Generic device pointer on success, NULL on error. If rte_errno is set, mapping codes to meanings would be helpful here. Acked-by: Gaetan Rivet -- Gaëtan