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

[Andrew Rybchenko]

On 9/2/19 12:33 PM, Ferruh Yigit wrote:
> On 8/29/2019 5:56 PM, Thomas Monjalon wrote:
>> 28/08/2019 16:39, Andrew Rybchenko:
>>> On 8/28/19 2:26 PM, Jan Viktorin wrote:
>>>> Andrew Rybchenko <arybchenko at solarflare.com> wrote:
>>>>> On 8/28/19 12:51 PM, Jan Viktorin wrote:
>>>>>> Andrew Rybchenko <arybchenko at solarflare.com> wrote:
>>>>>>> From: Ivan Ilchenko <Ivan.Ilchenko at oktetlabs.ru>
>>>>>>> + * @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 like the idea of making some functions mandatory.
>> If we need to provide a default callback, why not.
>>
>> I'm also thinking we need to better enforce a standardization
>> of basic features to be implemented. It would make DPDK more mature.
>>
>>
> +1 to make some dev_ops mandatory. At first I can think of:
> dev_infos_get
> dev_configure
> rx_queue_setup
> tx_queue_setup
> dev_start
> dev_stop

+1 as well, but I think it is a separate story.
I really don't want to complicate so big patchset by introducing
more logic here.

As far as I can see dev_infos_get callback is implemented in
all network PMDs. So, I think the topic is not gating the patchset.

> specific to 'dev_infos_get', it is to get device info, not sure a default
> callback is good idea for it.
>
> And overall agree that 'rte_eth_dev_info_get()' can be documented more/better ...

Me too