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