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