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

Andrew Rybchenko arybchenko at solarflare.com
Wed Aug 28 16:39:53 CEST 2019


@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 <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 [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.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.

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.


More information about the dev mailing list