[01/51] ethdev: change rte_eth_dev_info_get() return value to int
Checks
Commit Message
From: Ivan Ilchenko <Ivan.Ilchenko@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.
Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@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(-)
Comments
On Tue, 27 Aug 2019 15:25:12 +0100
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@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...
>
> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@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.
Regards
Jan
> + * - (-ENODEV) if *port_id* invalid.
> */
> -void rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info
> *dev_info); +int rte_eth_dev_info_get(uint16_t port_id, struct
> rte_eth_dev_info *dev_info);
> /**
> * Retrieve the firmware version of a device.
On 8/28/19 12:51 PM, Jan Viktorin wrote:
> On Tue, 27 Aug 2019 15:25:12 +0100
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> From: Ivan Ilchenko <Ivan.Ilchenko@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
>> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@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?
I think that a sane default contents is more harm than an error
(basically that's what we had before the patch).
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.
On Wed, 28 Aug 2019 13:09:51 +0300
Andrew Rybchenko <arybchenko@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@solarflare.com> wrote:
> >
> >> From: Ivan Ilchenko <Ivan.Ilchenko@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.Ilchenko@oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <arybchenko@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
@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@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@solarflare.com> wrote:
>>>
>>>> From: Ivan Ilchenko <Ivan.Ilchenko@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@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@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.
28/08/2019 16:39, Andrew Rybchenko:
> On 8/28/19 2:26 PM, Jan Viktorin wrote:
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >> On 8/28/19 12:51 PM, Jan Viktorin wrote:
> >>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >>>> From: Ivan Ilchenko <Ivan.Ilchenko@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.
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@solarflare.com> wrote:
>>>> On 8/28/19 12:51 PM, Jan Viktorin wrote:
>>>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>>>> From: Ivan Ilchenko <Ivan.Ilchenko@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
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 ...
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@solarflare.com> wrote:
>>>>> On 8/28/19 12:51 PM, Jan Viktorin wrote:
>>>>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>>>>> From: Ivan Ilchenko <Ivan.Ilchenko@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
On 9/3/2019 1:09 PM, Andrew Rybchenko wrote:
> 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@solarflare.com> wrote:
>>>>>> On 8/28/19 12:51 PM, Jan Viktorin wrote:
>>>>>>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>>>>>>> From: Ivan Ilchenko <Ivan.Ilchenko@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.
Agreed it is a separate story and not a blocker for this set
>
>> 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
>
@@ -88,7 +88,6 @@ Deprecation Notices
negative errno values to indicate various error conditions (e.g.
invalid port ID, unsupported operation, failed operation):
- - ``rte_eth_dev_info_get``
- ``rte_eth_promiscuous_enable`` and ``rte_eth_promiscuous_disable``
- ``rte_eth_allmulticast_enable`` and ``rte_eth_allmulticast_disable``
- ``rte_eth_link_get`` and ``rte_eth_link_get_nowait``
@@ -85,6 +85,9 @@ API Changes
Also, make sure to start the actual text at the margin.
=========================================================
+* ethdev: changed ``rte_eth_dev_infos_get`` return value from ``void`` to
+ ``int`` to provide a way to report various error conditions.
+
ABI Changes
-----------
@@ -136,7 +139,7 @@ The libraries prepended with a plus sign were incremented in this version.
librte_distributor.so.1
librte_eal.so.11
librte_efd.so.1
- librte_ethdev.so.12
+ + librte_ethdev.so.13
librte_eventdev.so.7
librte_flow_classify.so.1
librte_gro.so.1
@@ -1125,7 +1125,6 @@ struct rte_eth_dev *
dev = &rte_eth_devices[port_id];
- RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
if (dev->data->dev_started) {
@@ -1144,7 +1143,9 @@ struct rte_eth_dev *
*/
memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
- rte_eth_dev_info_get(port_id, &dev_info);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
/* If number of queues specified by application for both Rx and Tx is
* zero, use driver preferred values. This cannot be done individually
@@ -1406,6 +1407,7 @@ struct rte_eth_dev *
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
int diag;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1420,7 +1422,9 @@ struct rte_eth_dev *
return 0;
}
- rte_eth_dev_info_get(port_id, &dev_info);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
/* Lets restore MAC now if device does not support live change */
if (*dev_info.dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR)
@@ -1584,7 +1588,6 @@ struct rte_eth_dev *
return -EINVAL;
}
- RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_setup, -ENOTSUP);
/*
@@ -1592,7 +1595,10 @@ struct rte_eth_dev *
* This value must be provided in the private data of the memory pool.
* First check that the memory pool has a valid private data.
*/
- rte_eth_dev_info_get(port_id, &dev_info);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
+
if (mp->private_data_size < sizeof(struct rte_pktmbuf_pool_private)) {
RTE_ETHDEV_LOG(ERR, "%s private_data_size %d < %d\n",
mp->name, (int)mp->private_data_size,
@@ -1703,6 +1709,7 @@ struct rte_eth_dev *
struct rte_eth_dev_info dev_info;
struct rte_eth_txconf local_conf;
void **txq;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1712,10 +1719,11 @@ struct rte_eth_dev *
return -EINVAL;
}
- RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_setup, -ENOTSUP);
- rte_eth_dev_info_get(port_id, &dev_info);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
/* Use default specified by driver, if nb_tx_desc is zero */
if (nb_tx_desc == 0) {
@@ -2540,7 +2548,7 @@ struct rte_eth_dev *
fw_version, fw_size));
}
-void
+int
rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
{
struct rte_eth_dev *dev;
@@ -2558,7 +2566,7 @@ struct rte_eth_dev *
*/
memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
- RTE_ETH_VALID_PORTID_OR_RET(port_id);
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
dev = &rte_eth_devices[port_id];
dev_info->rx_desc_lim = lim;
@@ -2567,13 +2575,15 @@ struct rte_eth_dev *
dev_info->min_mtu = RTE_ETHER_MIN_MTU;
dev_info->max_mtu = UINT16_MAX;
- RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
(*dev->dev_ops->dev_infos_get)(dev, dev_info);
dev_info->driver_name = dev->device->driver->name;
dev_info->nb_rx_queues = dev->data->nb_rx_queues;
dev_info->nb_tx_queues = dev->data->nb_tx_queues;
dev_info->dev_flags = &dev->data->dev_flags;
+
+ return 0;
}
int
@@ -2643,7 +2653,10 @@ struct rte_eth_dev *
* which relies on dev->dev_ops->dev_infos_get.
*/
if (*dev->dev_ops->dev_infos_get != NULL) {
- rte_eth_dev_info_get(port_id, &dev_info);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
+
if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu)
return -EINVAL;
}
@@ -2991,10 +3004,15 @@ struct rte_eth_dev *
{
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info = { .flow_type_rss_offloads = 0, };
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
+
dev = &rte_eth_devices[port_id];
- rte_eth_dev_info_get(port_id, &dev_info);
if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
dev_info.flow_type_rss_offloads) {
RTE_ETHDEV_LOG(ERR,
@@ -3100,9 +3118,13 @@ struct rte_eth_dev *
struct rte_eth_dev_info dev_info;
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
unsigned i;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
- rte_eth_dev_info_get(port_id, &dev_info);
+
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
for (i = 0; i < dev_info.max_mac_addrs; i++)
if (memcmp(addr, &dev->data->mac_addrs[i],
@@ -3233,8 +3255,14 @@ struct rte_eth_dev *
struct rte_eth_dev_info dev_info;
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
unsigned i;
+ int ret;
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
- rte_eth_dev_info_get(port_id, &dev_info);
if (!dev->data->hash_mac_addrs)
return -1;
@@ -3319,11 +3347,15 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
struct rte_eth_link link;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
+
dev = &rte_eth_devices[port_id];
- rte_eth_dev_info_get(port_id, &dev_info);
link = dev->data->dev_link;
if (queue_idx > dev_info.max_tx_queues) {
@@ -4363,15 +4395,14 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
uint16_t *nb_rx_desc,
uint16_t *nb_tx_desc)
{
- struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
+ int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
- dev = &rte_eth_devices[port_id];
- RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
-
- rte_eth_dev_info_get(port_id, &dev_info);
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
if (nb_rx_desc != NULL)
rte_eth_dev_adjust_nb_desc(nb_rx_desc, &dev_info.rx_desc_lim);
@@ -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.
+ * - (-ENODEV) if *port_id* invalid.
*/
-void rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
+int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
/**
* Retrieve the firmware version of a device.