[dpdk-dev,1/5] ethdev: add firmware version get

  This patch added API for 'rte_eth_dev_fwver_get'

void rte_eth_dev_fwver_get(uint8_t port_id,
char *fw_version, int fw_length);

Signed-off-by: Qiming Yang <qiming.yang@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 12 ++++++++++++
 lib/librte_ether/rte_ethdev.h          | 18 ++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++++++
 3 files changed, 37 insertions(+)
  

2016-11-17 17:42, Qiming Yang:
> This patch added API for 'rte_eth_dev_fwver_get'
> 
> void rte_eth_dev_fwver_get(uint8_t port_id,
> char *fw_version, int fw_length);

Copying some code here doesn't help really help.
Could you describe what we can expect in this string?
How can we compare this version number across different
devices of the same driver?
How does it apply to FPGA devices?
What is the potential use?

[...]
>  /**
> + * Retrieve the firmware version of an Ethernet device.

We should stop talking about Ethernet device.
Networking device is more appropriate.
And in this case, "device" is enough.

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param fw_version
> + *   A pointer the firmware version of an Ethernet device
> + * @param fw_length
> + *   The size of the firmware version, which should be large enough to store
> + *   the firmware version of the device.

How do we know that the length is too small?

> + */
> +void rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length);

Why not returning some errors?

You forgot to remove the deprecation notice in this patch.

PS: the series is broken as some patches are not numbered and this one is
not the first one. Please use a cover-letter to introduce such series.
  

On 17/11/2016 17:42, Qiming Yang wrote:
> This patch added API for 'rte_eth_dev_fwver_get'
>
> void rte_eth_dev_fwver_get(uint8_t port_id,
> char *fw_version, int fw_length);

Suggest description such as:

This patch adds ethdev API for fetching firmware version.

Also see Thomas's comments. Looks like some stale patches got picked up 
by 'git send-mail *.patch'..

..Remy
  

Hi, Thomas

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Thursday, November 17, 2016 9:37 PM
To: Yang, Qiming <qiming.yang@intel.com>
Cc: dev@dpdk.org; Horton, Remy <remy.horton@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Chen, Jing D <jing.d.chen@intel.com>
Subject: Re: [dpdk-dev] [PATCH 1/5] ethdev: add firmware version get

2016-11-17 17:42, Qiming Yang:
> This patch added API for 'rte_eth_dev_fwver_get'
> 
> void rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int 
> fw_length);

Copying some code here doesn't help really help.
Could you describe what we can expect in this string?
How can we compare this version number across different devices of the same driver?
How does it apply to FPGA devices?
What is the potential use?

Qiming: I'll add more describes in the next version.

[...]
>  /**
> + * Retrieve the firmware version of an Ethernet device.

We should stop talking about Ethernet device.
Networking device is more appropriate.
And in this case, "device" is enough.

Qiming: Got it.

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param fw_version
> + *   A pointer the firmware version of an Ethernet device
> + * @param fw_length
> + *   The size of the firmware version, which should be large enough to store
> + *   the firmware version of the device.

How do we know that the length is too small?

Qiming: The length is a number large enough to store the firmware version. Original I use a fixed number 20, but in order to be more flexible, I replace it into a variable. 

> + */
> +void rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int 
> +fw_length);

Why not returning some errors?

Qiming: It's a good advice.

You forgot to remove the deprecation notice in this patch.

PS: the series is broken as some patches are not numbered and this one is not the first one. Please use a cover-letter to introduce such series.

Qiming: It's my fault. Thank you for your reminder, I will pay attention next time.
  

Yes, that's my fault. I have set their statuses to 'suspended'

Qiming

-----Original Message-----
From: Horton, Remy 
Sent: Friday, November 18, 2016 9:10 AM
To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org
Cc: Wu, Jingjing <jingjing.wu@intel.com>; Chen, Jing D <jing.d.chen@intel.com>
Subject: Re: [PATCH 1/5] ethdev: add firmware version get


On 17/11/2016 17:42, Qiming Yang wrote:
> This patch added API for 'rte_eth_dev_fwver_get'
>
> void rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int 
> fw_length);

Suggest description such as:

This patch adds ethdev API for fetching firmware version.

Also see Thomas's comments. Looks like some stale patches got picked up by 'git send-mail *.patch'..

..Remy
  

Now, the example ethtool can only show the driver information. From
customers' point of view, it should be better if we can have the same
way that the Linux kernel ethtool does to show the bus-info and
firmware-version.

These five patches add a new API to fetch firmware version and implement
the display in example ethtool.

Qiming Yang (5):
  ethdev: add firmware version get
  net/e1000: add firmware version get
  net/ixgbe: add firmware version get
  net/i40e: add firmware version get
  ethtool: dispaly bus info and firmware version

 drivers/net/e1000/igb_ethdev.c         | 46 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c         | 15 +++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c       | 18 +++++++++++++
 examples/ethtool/ethtool-app/ethapp.c  |  2 ++
 examples/ethtool/lib/rte_ethtool.c     |  3 +++
 lib/librte_ether/rte_ethdev.c          | 12 +++++++++
 lib/librte_ether/rte_ethdev.h          | 18 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  7 ++++++
 8 files changed, 121 insertions(+)
  

On 06/12/2016 15:16, Qiming Yang wrote:
[..]
> Qiming Yang (5):
>   ethdev: add firmware version get
>   net/e1000: add firmware version get
>   net/ixgbe: add firmware version get
>   net/i40e: add firmware version get
>   ethtool: dispaly bus info and firmware version

s/dispaly/display

doc/guides/rel_notes/release_17_02.rst ought to be updated as well. Code 
itself looks ok though..

Acked-by: Remy Horton <remy.horton@intel.com>
  

Hi, Remy
I'll add this update in v3 patch, thank you for your remind.
BTW, It looks like Yigit don't agree the implement we used, he think we'd better not add a new ops to get this info.

-----Original Message-----
From: Horton, Remy 
Sent: Thursday, December 8, 2016 4:34 PM
To: Yang, Qiming <qiming.yang@intel.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 0/5] example/ethtool: add bus info and fw version get


On 06/12/2016 15:16, Qiming Yang wrote:
[..]
> Qiming Yang (5):
>   ethdev: add firmware version get
>   net/e1000: add firmware version get
>   net/ixgbe: add firmware version get
>   net/i40e: add firmware version get
>   ethtool: dispaly bus info and firmware version

s/dispaly/display

doc/guides/rel_notes/release_17_02.rst ought to be updated as well. Code itself looks ok though..

Acked-by: Remy Horton <remy.horton@intel.com>
  

2016-12-08 16:34, Remy Horton:
> 
> On 06/12/2016 15:16, Qiming Yang wrote:
> [..]
> > Qiming Yang (5):
> >   ethdev: add firmware version get
> >   net/e1000: add firmware version get
> >   net/ixgbe: add firmware version get
> >   net/i40e: add firmware version get
> >   ethtool: dispaly bus info and firmware version
> 
> s/dispaly/display
> 
> doc/guides/rel_notes/release_17_02.rst ought to be updated as well. Code 
> itself looks ok though..
> 
> Acked-by: Remy Horton <remy.horton@intel.com>

It must be a feature in the table (doc/guides/nics/features/).
The deprecation notice must be removed also.

I think it is OK to add a new dev_ops and a new API function for firmware
query. Generally speaking, it is a good thing to avoid putting all
informations in the same structure (e.g. rte_eth_dev_info). However, there
is a balance to find. Could we plan to add more info to this new query?
Instead of
	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length)
could it fill a struct?
	rte_eth_dev_fw_info_get(uint8_t port_id, struct rte_eth_dev_fw_info *fw_info)

We already have
	rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info)
with
	uint32_t version; /**< Device version */

There are also these functions (a bit related):
	rte_eth_dev_get_eeprom_length(uint8_t port_id)
	rte_eth_dev_get_eeprom(uint8_t port_id, struct rte_dev_eeprom_info *info)
  

On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
> 2016-12-08 16:34, Remy Horton:
>>
>> On 06/12/2016 15:16, Qiming Yang wrote:
>> [..]
>>> Qiming Yang (5):
>>>   ethdev: add firmware version get
>>>   net/e1000: add firmware version get
>>>   net/ixgbe: add firmware version get
>>>   net/i40e: add firmware version get
>>>   ethtool: dispaly bus info and firmware version
>>
>> s/dispaly/display
>>
>> doc/guides/rel_notes/release_17_02.rst ought to be updated as well. Code 
>> itself looks ok though..
>>
>> Acked-by: Remy Horton <remy.horton@intel.com>
> 
> It must be a feature in the table (doc/guides/nics/features/).
> The deprecation notice must be removed also.
> 
> I think it is OK to add a new dev_ops and a new API function for firmware
> query. Generally speaking, it is a good thing to avoid putting all
> informations in the same structure (e.g. rte_eth_dev_info). 

OK.

> However, there
> is a balance to find. Could we plan to add more info to this new query?
> Instead of
> 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length)

Here there is another problem, the content and the format of the string
is not defined. In this patchset it is not same for different PMDs.
This is OK for just printing the data, but not good for an API. How can
the application know what to expect.

> could it fill a struct?
> 	rte_eth_dev_fw_info_get(uint8_t port_id, struct rte_eth_dev_fw_info *fw_info)

I believe this is better. But the problem we are having with this usage
is: ABI breakage.

Since this struct will be a public structure, in the future if we want
to add a new field to the struct, it will break the ABI, and just this
change will cause a new version for whole ethdev library!

When all required fields received via arguments, one by one, instead of
struct, at least ABI versioning can be done on the API when new field
added, and can be possible to escape from ABI breakage. But this will be
ugly when number of arguments increased.

Or any other opinion on how to define API to reduce ABI breakage?

> 
> We already have
> 	rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info)
> with
> 	uint32_t version; /**< Device version */
> 
> There are also these functions (a bit related):
> 	rte_eth_dev_get_eeprom_length(uint8_t port_id)
> 	rte_eth_dev_get_eeprom(uint8_t port_id, struct rte_dev_eeprom_info *info)
>
  

2016-12-22 14:36, Ferruh Yigit:
> On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
> > I think it is OK to add a new dev_ops and a new API function for firmware
> > query. Generally speaking, it is a good thing to avoid putting all
> > informations in the same structure (e.g. rte_eth_dev_info). 
> 
> OK.
> 
> > However, there
> > is a balance to find. Could we plan to add more info to this new query?
> > Instead of
> > 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length)
[...]
> > could it fill a struct?
> > 	rte_eth_dev_fw_info_get(uint8_t port_id, struct rte_eth_dev_fw_info *fw_info)
> 
> I believe this is better. But the problem we are having with this usage
> is: ABI breakage.
> 
> Since this struct will be a public structure, in the future if we want
> to add a new field to the struct, it will break the ABI, and just this
> change will cause a new version for whole ethdev library!
> 
> When all required fields received via arguments, one by one, instead of
> struct, at least ABI versioning can be done on the API when new field
> added, and can be possible to escape from ABI breakage. But this will be
> ugly when number of arguments increased.
> 
> Or any other opinion on how to define API to reduce ABI breakage?

You're right.
But I don't think we should have a function per data. Just because it would
be ugly :)
I hope the ABI could become stable with time.
  

On 12/22/2016 2:47 PM, Thomas Monjalon wrote:
> 2016-12-22 14:36, Ferruh Yigit:
>> On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
>>> I think it is OK to add a new dev_ops and a new API function for firmware
>>> query. Generally speaking, it is a good thing to avoid putting all
>>> informations in the same structure (e.g. rte_eth_dev_info). 
>>
>> OK.
>>
>>> However, there
>>> is a balance to find. Could we plan to add more info to this new query?
>>> Instead of
>>> 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length)
> [...]
>>> could it fill a struct?
>>> 	rte_eth_dev_fw_info_get(uint8_t port_id, struct rte_eth_dev_fw_info *fw_info)
>>
>> I believe this is better. But the problem we are having with this usage
>> is: ABI breakage.
>>
>> Since this struct will be a public structure, in the future if we want
>> to add a new field to the struct, it will break the ABI, and just this
>> change will cause a new version for whole ethdev library!
>>
>> When all required fields received via arguments, one by one, instead of
>> struct, at least ABI versioning can be done on the API when new field
>> added, and can be possible to escape from ABI breakage. But this will be
>> ugly when number of arguments increased.
>>
>> Or any other opinion on how to define API to reduce ABI breakage?
> 
> You're right.
> But I don't think we should have a function per data. Just because it would
> be ugly :)

I am no suggesting function per data, instead something like:

rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);

And in the future if we need etrack_id too, we can have both in
versioned manner:

rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);

rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min,
	uint32_t etrack_id);

So my concern was if the number of the arguments becomes too many by time.


> I hope the ABI could become stable with time.
>
  

2016-12-22 15:05, Ferruh Yigit:
> On 12/22/2016 2:47 PM, Thomas Monjalon wrote:
> > 2016-12-22 14:36, Ferruh Yigit:
> >> On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
> >>> I think it is OK to add a new dev_ops and a new API function for firmware
> >>> query. Generally speaking, it is a good thing to avoid putting all
> >>> informations in the same structure (e.g. rte_eth_dev_info). 
> >>
> >> OK.
> >>
> >>> However, there
> >>> is a balance to find. Could we plan to add more info to this new query?
> >>> Instead of
> >>> 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length)
> > [...]
> >>> could it fill a struct?
> >>> 	rte_eth_dev_fw_info_get(uint8_t port_id, struct rte_eth_dev_fw_info *fw_info)
> >>
> >> I believe this is better. But the problem we are having with this usage
> >> is: ABI breakage.
> >>
> >> Since this struct will be a public structure, in the future if we want
> >> to add a new field to the struct, it will break the ABI, and just this
> >> change will cause a new version for whole ethdev library!
> >>
> >> When all required fields received via arguments, one by one, instead of
> >> struct, at least ABI versioning can be done on the API when new field
> >> added, and can be possible to escape from ABI breakage. But this will be
> >> ugly when number of arguments increased.
> >>
> >> Or any other opinion on how to define API to reduce ABI breakage?
> > 
> > You're right.
> > But I don't think we should have a function per data. Just because it would
> > be ugly :)
> 
> I am no suggesting function per data, instead something like:
> 
> rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
> 
> And in the future if we need etrack_id too, we can have both in
> versioned manner:
> 
> rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
> 
> rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min,
> 	uint32_t etrack_id);

Oh I see. So it can be versioned with compat macros.

> So my concern was if the number of the arguments becomes too many by time.

It looks to be a good proposal. We should not have a dozen of arguments.
  

On 12/22/2016 3:31 PM, Thomas Monjalon wrote:
> 2016-12-22 15:05, Ferruh Yigit:
>> On 12/22/2016 2:47 PM, Thomas Monjalon wrote:
>>> 2016-12-22 14:36, Ferruh Yigit:
>>>> On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
>>>>> I think it is OK to add a new dev_ops and a new API function for firmware
>>>>> query. Generally speaking, it is a good thing to avoid putting all
>>>>> informations in the same structure (e.g. rte_eth_dev_info). 
>>>>
>>>> OK.
>>>>
>>>>> However, there
>>>>> is a balance to find. Could we plan to add more info to this new query?
>>>>> Instead of
>>>>> 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int fw_length)
>>> [...]
>>>>> could it fill a struct?
>>>>> 	rte_eth_dev_fw_info_get(uint8_t port_id, struct rte_eth_dev_fw_info *fw_info)
>>>>
>>>> I believe this is better. But the problem we are having with this usage
>>>> is: ABI breakage.
>>>>
>>>> Since this struct will be a public structure, in the future if we want
>>>> to add a new field to the struct, it will break the ABI, and just this
>>>> change will cause a new version for whole ethdev library!
>>>>
>>>> When all required fields received via arguments, one by one, instead of
>>>> struct, at least ABI versioning can be done on the API when new field
>>>> added, and can be possible to escape from ABI breakage. But this will be
>>>> ugly when number of arguments increased.
>>>>
>>>> Or any other opinion on how to define API to reduce ABI breakage?
>>>
>>> You're right.
>>> But I don't think we should have a function per data. Just because it would
>>> be ugly :)
>>
>> I am no suggesting function per data, instead something like:
>>
>> rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
>>
>> And in the future if we need etrack_id too, we can have both in
>> versioned manner:
>>
>> rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
>>
>> rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min,
>> 	uint32_t etrack_id);
> 
> Oh I see. So it can be versioned with compat macros.
> 
>> So my concern was if the number of the arguments becomes too many by time.
> 
> It looks to be a good proposal. We should not have a dozen of arguments.
> 

So, I suggest trying this approach in this API.


Overall, change request for the patch becomes:
1- Change API, is following arguments good enough to start with?:
- FW_major_number
- FW_minor_number
- FW_patch_number
- Etrack_id

If so, API becomes:
rte_eth_dev_fw_version_get(uint8_t port_id, uint32_t *fw_major,
	uint32_t *fw_minor, uint32_t *fw_patch, uint32_t *etrack_id);

! Note, I have renamed API to rte_eth_dev_fw_version_get() from
rte_eth_dev_fw_info_get() mentioned above, to narrow the scope of API.

and dev_ops name keeps same: fw_version_get

2- Add new feature in feature table (doc/guides/nics/features/), first
patch can add to the default one, and each driver patch implements this
feature should update its feature table.
Feature name can be "FW version"

3- Remove deprecation notice in the first patch.


Thanks,
ferruh
  

These four patches added a new function ``rte_eth_dev_fwver_get()``
to fetch firmware related information by a given device.
Information include major firmware version, minor firmware
version, patch number and etrack id.

Qiming Yang (4):
  ethdev: add firmware information get
  net/e1000: add firmware version get
  net/ixgbe: add firmware version get
  net/i40e: add firmware version get

 doc/guides/nics/features/default.ini   |  1 +
 doc/guides/nics/features/i40e.ini      |  1 +
 doc/guides/nics/features/igb.ini       |  1 +
 doc/guides/nics/features/ixgbe.ini     |  1 +
 doc/guides/rel_notes/release_17_02.rst |  4 ++++
 drivers/net/e1000/igb_ethdev.c         | 43 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c         | 15 ++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c       | 17 ++++++++++++++
 lib/librte_ether/rte_ethdev.c          | 14 +++++++++++
 lib/librte_ether/rte_ethdev.h          | 23 ++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 11 files changed, 121 insertions(+)
  

Now, the example ethtool can only show the driver information. From
customers' point of view, it should be better if we can have the same
way that the Linux kernel ethtool does to show firmware-version.

These five patches added a new function ``rte_eth_dev_fw_version_get()``
to fetch firmware version related information and implement the
display in example ethtool.

Qiming Yang (5):
  ethdev: add firmware version get
  net/e1000: add firmware version get
  net/ixgbe: add firmware version get
  net/i40e: add firmware version get
  ethtool: dispaly firmware version

 doc/guides/nics/features/default.ini   |  1 +
 doc/guides/nics/features/i40e.ini      |  1 +
 doc/guides/nics/features/igb.ini       |  1 +
 doc/guides/nics/features/ixgbe.ini     |  1 +
 doc/guides/rel_notes/deprecation.rst   |  4 ----
 doc/guides/rel_notes/release_17_02.rst |  3 +++
 drivers/net/e1000/igb_ethdev.c         | 43 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c         | 15 ++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c       | 17 ++++++++++++++
 examples/ethtool/ethtool-app/ethapp.c  |  1 +
 examples/ethtool/lib/rte_ethtool.c     | 12 ++++++++++
 lib/librte_ether/rte_ethdev.c          | 14 +++++++++++
 lib/librte_ether/rte_ethdev.h          | 23 ++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 14 files changed, 133 insertions(+), 4 deletions(-)
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Thursday, December 22, 2016 11:31 PM
> To: Yigit, Ferruh
> Cc: Yang, Qiming; dev@dpdk.org; Horton, Remy
> Subject: Re: [dpdk-dev] [PATCH v2 0/5] example/ethtool: add bus info and fw
> version get
> 
> 2016-12-22 15:05, Ferruh Yigit:
> > On 12/22/2016 2:47 PM, Thomas Monjalon wrote:
> > > 2016-12-22 14:36, Ferruh Yigit:
> > >> On 12/22/2016 11:07 AM, Thomas Monjalon wrote:
> > >>> I think it is OK to add a new dev_ops and a new API function for
> > >>> firmware query. Generally speaking, it is a good thing to avoid
> > >>> putting all informations in the same structure (e.g. rte_eth_dev_info).
> > >>
> > >> OK.
> > >>
> > >>> However, there
> > >>> is a balance to find. Could we plan to add more info to this new query?
> > >>> Instead of
> > >>> 	rte_eth_dev_fwver_get(uint8_t port_id, char *fw_version, int
> > >>> fw_length)
> > > [...]
> > >>> could it fill a struct?
> > >>> 	rte_eth_dev_fw_info_get(uint8_t port_id, struct
> > >>> rte_eth_dev_fw_info *fw_info)
> > >>
> > >> I believe this is better. But the problem we are having with this
> > >> usage
> > >> is: ABI breakage.
> > >>
> > >> Since this struct will be a public structure, in the future if we
> > >> want to add a new field to the struct, it will break the ABI, and
> > >> just this change will cause a new version for whole ethdev library!
> > >>
> > >> When all required fields received via arguments, one by one,
> > >> instead of struct, at least ABI versioning can be done on the API
> > >> when new field added, and can be possible to escape from ABI
> > >> breakage. But this will be ugly when number of arguments increased.
> > >>
> > >> Or any other opinion on how to define API to reduce ABI breakage?
> > >
> > > You're right.
> > > But I don't think we should have a function per data. Just because
> > > it would be ugly :)
> >
> > I am no suggesting function per data, instead something like:
> >
> > rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
> >
> > And in the future if we need etrack_id too, we can have both in
> > versioned manner:
> >
> > rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min);
> >
> > rte_eth_dev_fw_info_get(uint8_t port_id, uint32_t maj, uint32_t min,
> > 	uint32_t etrack_id);
> 
> Oh I see. So it can be versioned with compat macros.
> 
> > So my concern was if the number of the arguments becomes too many by
> time.
> 
> It looks to be a good proposal. We should not have a dozen of arguments.

I'd suggest to do that the similar way of kernel driver/ethtool (Linux or FreeBSD) does,
which should be well discussed.
In addition, for future extention, and avoid breaking any ABI in a strcuture,
we can just pre-define a lot of bytes as reserved, e.g. 64 bytes. Inside DPDK,
there are several strucutres defined like this, e.g. mbuf.

Thanks,
Helin
  

Now, the example ethtool can only show the driver information. From
customers' point of view, it should be better if we can have the same
way that the Linux kernel ethtool does to show firmware version.

These five patches added a new function ``rte_eth_dev_fw_version_get()``
to fetch firmware version related information and implement the
display in example ethtool.


Qiming Yang (5):
  ethdev: add firmware version get
  net/e1000: add firmware version get
  net/ixgbe: add firmware version get
  net/i40e: add firmware version get
  ethtool: display firmware version

 doc/guides/nics/features/default.ini   |  1 +
 doc/guides/nics/features/i40e.ini      |  1 +
 doc/guides/nics/features/igb.ini       |  1 +
 doc/guides/nics/features/ixgbe.ini     |  1 +
 doc/guides/rel_notes/deprecation.rst   |  4 ---
 doc/guides/rel_notes/release_17_02.rst |  3 +++
 drivers/net/e1000/igb_ethdev.c         | 46 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c         | 15 +++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c       | 18 +++++++++++++
 examples/ethtool/ethtool-app/ethapp.c  |  1 +
 examples/ethtool/lib/rte_ethtool.c     |  3 +++
 lib/librte_ether/rte_ethdev.c          | 12 +++++++++
 lib/librte_ether/rte_ethdev.h          | 20 +++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 14 files changed, 123 insertions(+), 4 deletions(-)
  

v6: - renamed fw_length -> fw_size
    - added return value for insufficient fw_size
    - fixed the indentation problem in e1000
    - added ver.build.patch in i40e, keep the same with Linux kernel driver

v5: - modified the API rte_eth_dev_fw_version_get(uint8_t port_id,
      char *fw_version, int fw_length).

v4: - removed deprecation notice
    - renamed API as rte_eth_dev_fw_version_get
    - splited bus info print from this patch set

v3: - changed API, use rte_eth_dev_fw_info_get(uint8_t port_id,
      uint32_t *fw_major, uint32_t *fw_minor, uint32_t *fw_patch,
      uint32_t *etrack_id)
    - added statusment in /doc/guides/nics/features/default.ini and
      release_17_02.rst

v2: - fixed commit log

Now, the example ethtool can only show the driver information. From
customers' point of view, it should be better if we can have the same
way that the Linux kernel ethtool does to show firmware version.

These five patches added a new function ``rte_eth_dev_fw_version_get()``
to fetch firmware version related information and implement the
display in example ethtool.

Qiming Yang (5):
  ethdev: add firmware version get
  net/e1000: add firmware version get
  net/ixgbe: add firmware version get
  net/i40e: add firmware version get
  ethtool: display firmware version

 doc/guides/nics/features/default.ini   |  1 +
 doc/guides/nics/features/i40e.ini      |  1 +
 doc/guides/nics/features/igb.ini       |  1 +
 doc/guides/nics/features/ixgbe.ini     |  1 +
 doc/guides/rel_notes/deprecation.rst   |  4 ---
 doc/guides/rel_notes/release_17_02.rst |  3 ++
 drivers/net/e1000/igb_ethdev.c         | 54 ++++++++++++++++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.c         | 31 +++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c       | 25 ++++++++++++++++
 examples/ethtool/ethtool-app/ethapp.c  |  1 +
 examples/ethtool/lib/rte_ethtool.c     |  6 ++++
 lib/librte_ether/rte_ethdev.c          | 12 ++++++++
 lib/librte_ether/rte_ethdev.h          | 25 ++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  1 +
 14 files changed, 162 insertions(+), 4 deletions(-)