From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v2 0/5] example/ethtool: add bus info and fw version get Date: Fri, 23 Dec 2016 12:48:57 +0000 Message-ID: <5ba58add-186f-a24d-d259-7caaa609e6b5@intel.com> References: <1479375779-46629-2-git-send-email-qiming.yang@intel.com> <6590239.9s5rXc1lKr@xps13> <095d7632-fbb7-a72b-8a48-163ed2b32cb4@intel.com> <4171707.MgFeI6QLbH@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: Qiming Yang , dev@dpdk.org, Remy Horton To: Thomas Monjalon Return-path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 31F252C60 for ; Fri, 23 Dec 2016 13:49:00 +0100 (CET) In-Reply-To: <4171707.MgFeI6QLbH@xps13> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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