From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id DEFC3A0613 for <public@inbox.dpdk.org>; Wed, 28 Aug 2019 13:25:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 258041C19E; Wed, 28 Aug 2019 13:25:32 +0200 (CEST) Received: from office2.cesnet.cz (office2.cesnet.cz [195.113.144.244]) by dpdk.org (Postfix) with ESMTP id 442C51BFEB for <dev@dpdk.org>; Wed, 28 Aug 2019 13:25:30 +0200 (CEST) Received: from coaster.localdomain (unknown [IPv6:2001:67c:1220:80e:a9:edd0:2e93:a6c4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by office2.cesnet.cz (Postfix) with ESMTPSA id 88E9C40006F; Wed, 28 Aug 2019 13:25:29 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cesnet.cz; s=office2; t=1566991530; bh=qGTEDswU0cO/rEiAdodkpHup1xuYlZeuVr31zq/70d4=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=oOz8aKP5hR5U6tG78fM0xF03rnUu3cR85uWwF1uXTdWczUcQvvh7G8UK2bUtdmkJs VY8pKGFxz4A0sK4TPXpXwdxIOn2rztiY9Z5wJaqA2Xdj+h3WjKy9i6A3WdnCOKXjb4 eHUTFUufDELYi3yOw/hPhiC1CsiwK3atLD7FwUTA= Date: Wed, 28 Aug 2019 13:26:38 +0200 From: Jan Viktorin <viktorin@cesnet.cz> To: Andrew Rybchenko <arybchenko@solarflare.com> Cc: Neil Horman <nhorman@tuxdriver.com>, John McNamara <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>, Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit <ferruh.yigit@intel.com>, <dev@dpdk.org>, Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru> Message-ID: <20190828132638.24193266@coaster.localdomain> In-Reply-To: <b5ee78eb-46cb-cda2-d1ae-31fab00bd04f@solarflare.com> References: <1566915962-5472-2-git-send-email-arybchenko@solarflare.com> <20190828115146.5812afa1@coaster.localdomain> <b5ee78eb-46cb-cda2-d1ae-31fab00bd04f@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 01/51] ethdev: change rte_eth_dev_info_get() return value to int X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> On Wed, 28 Aug 2019 13:09:51 +0300 Andrew Rybchenko <arybchenko@solarflare.com> wrote: > On 8/28/19 12:51 PM, Jan Viktorin wrote: > > On Tue, 27 Aug 2019 15:25:12 +0100 > > Andrew Rybchenko <arybchenko@solarflare.com> wrote: > > > >> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru> > >> > >> Change rte_eth_dev_info_get() return value from void to int and > >> return negative errno values in case of error conditions. > >> Modify rte_eth_dev_info_get() usage across the ethdev according > >> to new return type. > > Hello Andrew, > > > > I didn't find any cover letter describing the true intentions of > > this patchset. Anyway, see below a short comment... > > The cover letter [1] was sent together with the patch. > > [1] http://mails.dpdk.org/archives/dev/2019-August/141593.html Thanks. So, the goal is "just" to replace void by int. This is what I was missing... See below. > > >> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru> > >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com> > >> --- > >> doc/guides/rel_notes/deprecation.rst | 1 - > >> doc/guides/rel_notes/release_19_11.rst | 5 ++- > >> lib/librte_ethdev/rte_ethdev.c | 71 > >> ++++++++++++++++++++++++---------- lib/librte_ethdev/rte_ethdev.h > >> | 6 ++- 4 files changed, 60 insertions(+), 23 deletions(-) > > [...] > > > >> b/lib/librte_ethdev/rte_ethdev.h index dc6596b..09c278d 100644 > >> --- a/lib/librte_ethdev/rte_ethdev.h > >> +++ b/lib/librte_ethdev/rte_ethdev.h > >> @@ -2366,8 +2366,12 @@ int > >> rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id, > >> * @param dev_info > >> * A pointer to a structure of type *rte_eth_dev_info* to be > >> filled with > >> * the contextual information of the Ethernet device. > >> + * @return > >> + * - (0) if successful. > >> + * - (-ENOTSUP) if support for dev_infos_get() does not exist > >> for the device. > > I believe that allowing PMDs to return -ENOTSUP is not a good idea. > > At the moment, all PMDs provides this kind of information. It is not > > always very reliable piece of information but for me, it is a piece > > of gold I would not like to loose when configuring devices. > > > > I think it should be mandatory for all PMDs to provide this > > function. Another possible way, give a sane default contents of > > this structure. But, please, do not return -ENOTSUP. > > I agree that dev_infos_get() callback should be mandatory, but > what the function should do if the callback is not provided? One way would be to fail to register a PMD without that callback. Such PMD driver would be simply wrong. This is what I mean by saying "mandatory" - the callback MUST be provided. DPDK could be so nice to provide a default callback named like default_dev_infos_get_when_you_are_incompetent_pmd_author() returning mostly zeros and some always "known metadata" like device pointer, driver_name, ... > I think that a sane default contents is more harm than an error > (basically that's what we had before the patch). Well, the dev info API is not in the best possible condition. And I believe that this is not a secret. Especially, I am missing a kind of specification of the structure contents (API doc is not satisfactory at the moment). E.g. what does it mean when dev_info.max_rx_queues == 65535? Similarly max_rx_pktlen == 65535. IMHO, this is the source of harm - no spec. Let's return back to the thread topic. For me, at the application level, I need to get at least identification of the device - bus name, driver name, device ID. The dev info structure gives me those. If there is a better way to retrieve these info by port_id then I have no objections to allow to return -ENOTSUP. However, the only well-defined way seems to be rte_eth_dev_info_get(). If a PMD does not give it to me, such PMD becomes simply useless. I am currently experiencing 2 different PMDs and both provide slightly different semantics of the dev info structure. That is bad, of course. However, I can stand it if I know some info about the device - as I've already mentioned: ID, driver and bus. > Since the function may return error, caller should take it into > account anyway. Yes, some error codes could have special > handling, but default error handling is required in any case. > You are absolutely right and I support such changes. Regards Jan