[dpdk-dev] [PATCH 01/51] ethdev: change rte_eth_dev_info_get() return value to int
viktorin at cesnet.cz
Wed Aug 28 13:26:38 CEST 2019
On Wed, 28 Aug 2019 13:09:51 +0300
Andrew Rybchenko <arybchenko at 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 at solarflare.com> wrote:
> >> From: Ivan Ilchenko <Ivan.Ilchenko at 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  was sent together with the patch.
>  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
> >> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko at oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <arybchenko at 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
mostly zeros and some always "known metadata" like device pointer,
> 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.
More information about the dev