Re: [dpdk-dev] [PATCH 01/51] ethdev: change rte_eth_dev_info_get() return value to int

[Jan Viktorin <viktorin@cesnet.cz>]

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