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... See below. > > >> 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. 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
