From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP; 23 Dec 2016 04:48:59 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,393,1477983600"; d="scan'208";a="45807718" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.29]) ([10.237.220.29]) by orsmga005.jf.intel.com with ESMTP; 23 Dec 2016 04:48:58 -0800 To: Thomas Monjalon References: <1479375779-46629-2-git-send-email-qiming.yang@intel.com> <6590239.9s5rXc1lKr@xps13> <095d7632-fbb7-a72b-8a48-163ed2b32cb4@intel.com> <4171707.MgFeI6QLbH@xps13> Cc: Qiming Yang , dev@dpdk.org, Remy Horton From: Ferruh Yigit Message-ID: <5ba58add-186f-a24d-d259-7caaa609e6b5@intel.com> Date: Fri, 23 Dec 2016 12:48:57 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <4171707.MgFeI6QLbH@xps13> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 0/5] example/ethtool: add bus info and fw version get X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Dec 2016 12:49:00 -0000 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