From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id B3EE92C01 for ; Wed, 4 Jan 2017 09:47:48 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP; 04 Jan 2017 00:47:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,458,1477983600"; d="scan'208";a="209465072" Received: from amcewan-mobl1.ger.corp.intel.com (HELO [10.252.2.89]) ([10.252.2.89]) by fmsmga004.fm.intel.com with ESMTP; 04 Jan 2017 00:47:45 -0800 To: "Yang, Qiming" , "dev@dpdk.org" , "thomas.monjalon@6wind.com" References: <1481008582-69416-1-git-send-email-qiming.yang@intel.com> <1482841816-54143-1-git-send-email-qiming.yang@intel.com> <1482841816-54143-3-git-send-email-qiming.yang@intel.com> Cc: "Horton, Remy" From: Ferruh Yigit Message-ID: Date: Wed, 4 Jan 2017 08:47:45 +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: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 2/4] net/e1000: add firmware 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: Wed, 04 Jan 2017 08:47:49 -0000 On 1/4/2017 3:14 AM, Yang, Qiming wrote: > See the reply below. > > -----Original Message----- > From: Yigit, Ferruh > Sent: Tuesday, January 3, 2017 11:03 PM > To: Yang, Qiming ; dev@dpdk.org; thomas.monjalon@6wind.com > Cc: Horton, Remy > Subject: Re: [PATCH v3 2/4] net/e1000: add firmware version get > > On 12/27/2016 12:30 PM, Qiming Yang wrote: >> This patch adds a new function eth_igb_fw_version_get. >> >> Signed-off-by: Qiming Yang >> --- >> v3 changes: >> * use eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major, >> u32 *fw_minor, u32 *fw_minor, u32 *fw_patch, u32 *etrack_id) instead >> of eth_igb_fw_version_get(struct rte_eth_dev *dev, char *fw_version, >> int fw_length). Add statusment in /doc/guides/nics/features/igb.ini. >> --- >> --- >> doc/guides/nics/features/igb.ini | 1 + >> drivers/net/e1000/igb_ethdev.c | 43 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/doc/guides/nics/features/igb.ini >> b/doc/guides/nics/features/igb.ini >> index 9fafe72..ffd87ba 100644 >> --- a/doc/guides/nics/features/igb.ini >> +++ b/doc/guides/nics/features/igb.ini >> @@ -39,6 +39,7 @@ EEPROM dump = Y >> Registers dump = Y >> BSD nic_uio = Y >> Linux UIO = Y >> +FW version = Y > > Please keep same location with default.ini file. Why you are putting this just into middle of the uio and vfio? > Qiming: It's a clerical error, I want to add this line at the end of this file. > >> Linux VFIO = Y >> x86-32 = Y >> x86-64 = Y >> diff --git a/drivers/net/e1000/igb_ethdev.c >> b/drivers/net/e1000/igb_ethdev.c index 4a15447..25344b7 100644 >> --- a/drivers/net/e1000/igb_ethdev.c >> +++ b/drivers/net/e1000/igb_ethdev.c >> @@ -120,6 +120,8 @@ static int eth_igb_xstats_get_names(struct rte_eth_dev *dev, >> unsigned limit); >> static void eth_igb_stats_reset(struct rte_eth_dev *dev); static >> void eth_igb_xstats_reset(struct rte_eth_dev *dev); >> +static void eth_igb_fw_version_get(struct rte_eth_dev *dev, u32 *fw_major, >> + u32 *fw_minor, u32 *fw_patch, u32 *etrack_id); > > I think you can use a struct as parameter here. But beware, that struct should NOT be a public struct. > Qiming: I think only add a private struct for igb is unnecessary. Keep the arguments consistent with rte_eth_dev_fw_info_get is better. > What do you think? Both are OK. Normally, I believe using struct is better. But we are not using struct in public API because of the ABI compatibility issues. Here it is internal usage, there is no ABI breakage concern, so it may be possible to use a struct. But if you prefer to keep the arguments same here with public API, that is fine. > >> static void eth_igb_infos_get(struct rte_eth_dev *dev, >> struct rte_eth_dev_info *dev_info); static const uint32_t >> *eth_igb_supported_ptypes_get(struct rte_eth_dev *dev); @@ -389,6 >> +391,7 @@ static const struct eth_dev_ops eth_igb_ops = { >> .xstats_get_names = eth_igb_xstats_get_names, >> .stats_reset = eth_igb_stats_reset, >> .xstats_reset = eth_igb_xstats_reset, >> + .fw_version_get = eth_igb_fw_version_get, >> .dev_infos_get = eth_igb_infos_get, >> .dev_supported_ptypes_get = eth_igb_supported_ptypes_get, >> .mtu_set = eth_igb_mtu_set, >> @@ -1981,6 +1984,46 @@ eth_igbvf_stats_reset(struct rte_eth_dev *dev) >> } >> > > <...> >