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 C283810D77 for ; Thu, 22 Dec 2016 15:36:16 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP; 22 Dec 2016 06:36:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,388,1477983600"; d="scan'208";a="915194011" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.29]) ([10.237.220.29]) by orsmga003.jf.intel.com with ESMTP; 22 Dec 2016 06:36:14 -0800 To: Thomas Monjalon , Qiming Yang References: <1479375779-46629-2-git-send-email-qiming.yang@intel.com> <1481008582-69416-1-git-send-email-qiming.yang@intel.com> <1578263.GeZ0IiYehl@xps13> Cc: dev@dpdk.org, Remy Horton From: Ferruh Yigit Message-ID: Date: Thu, 22 Dec 2016 14:36:13 +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: <1578263.GeZ0IiYehl@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: Thu, 22 Dec 2016 14:36:17 -0000 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 > > 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) >