@Thomas, @Ferruh, please, see below.
On 8/28/19 2:26 PM, Jan Viktorin wrote:
On Wed, 28 Aug 2019 13:09:51 +0300
Andrew Rybchenko <arybche...@solarflare.com> wrote:
On 8/28/19 12:51 PM, Jan Viktorin wrote:
On Tue, 27 Aug 2019 15:25:12 +0100
Andrew Rybchenko <arybche...@solarflare.com> wrote:
From: Ivan Ilchenko <ivan.ilche...@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...
Got it. Will try to improve it.
See below.
See below as well.
Signed-off-by: Ivan Ilchenko <ivan.ilche...@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybche...@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.
Typically callbacks are set on probe and
rte_eth_dev_pci_generic_probe() and similar functions could
be updated to reject devices with missing dev_infos_get callback.
However, there is a secondary process cases where dev_infos_get
is not essential since control path may be unsupported in secondary
process.
Anyway, I think it is a separate story.
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, ...
Thomas, Ferruh, what do you think?
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.
I see.
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.
Thanks,
Andrew.
