[dpdk-dev] [PATCH v2 0/5] example/ethtool: add bus info and fw version get

[Ferruh Yigit]

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 at 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)
>