From patchwork Mon May 22 16:16:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Pattan, Reshma" X-Patchwork-Id: 24455 Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 4263F2B89; Mon, 22 May 2017 18:16:14 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 4B5C628EE for ; Mon, 22 May 2017 18:16:11 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP; 22 May 2017 09:16:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.38,377,1491289200"; d="scan'208"; a="1151322865" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga001.fm.intel.com with ESMTP; 22 May 2017 09:16:10 -0700 Received: from irsmsx110.ger.corp.intel.com ([169.254.15.151]) by IRSMSX102.ger.corp.intel.com ([169.254.2.153]) with mapi id 14.03.0319.002; Mon, 22 May 2017 17:16:09 +0100 From: "Pattan, Reshma" To: "Jastrzebski, MichalX K" , "dev@dpdk.org" CC: "Jain, Deepak K" , "Van Haaren, Harry" , "Azarewicz, PiotrX T" Thread-Topic: [PATCH 1/3] drivers/net: add support for IF-MIB and EtherLike-MIB for e1000 Thread-Index: AQHS0whjQv3OoFbORkGfkk/aprB3c6IAcZwQ Date: Mon, 22 May 2017 16:16:09 +0000 Message-ID: <3AEA2BF9852C6F48A459DA490692831F0113CF11@irsmsx110.ger.corp.intel.com> References: <20170522143202.22424-1-michalx.k.jastrzebski@intel.com> <20170522143202.22424-2-michalx.k.jastrzebski@intel.com> In-Reply-To: <20170522143202.22424-2-michalx.k.jastrzebski@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/3] drivers/net: add support for IF-MIB and EtherLike-MIB for e1000 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, -----Original Message----- From: Jastrzebski, MichalX K Sent: Monday, May 22, 2017 3:32 PM To: dev@dpdk.org Cc: Pattan, Reshma ; Jain, Deepak K ; Van Haaren, Harry ; Jastrzebski, MichalX K ; Azarewicz, PiotrX T Subject: [PATCH 1/3] drivers/net: add support for IF-MIB and EtherLike-MIB for e1000 Can you add the description to the commit message, describing what has been done. General doubt I have , I see all IF-MIB and Ether-Like-MIB attributes are added as Xstats. But most of IF-MIB attributes are not stats, instead they are information about the interface/port. So I guess such attributes should be displayed using separate method? Everything being displayed as xstats might not be correct. }; #define E1000_DEV_PRIVATE(adapter) \ diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c index e1702d8..705b591 100644 --- a/drivers/net/e1000/igb_ethdev.c +++ b/drivers/net/e1000/igb_ethdev.c Can you rename igb_if_mib_strings to rte_ igb_if_mib_strings and igb_if_mib_strings to rte_ igb_if_mib_strings? Similar to other stats/xstats. eth_igb_stats_reset(), inside this function we might not need below piece of code, as below piece of code you have already added to eth_igb_xstats_reset(). + + adapter->if_counter_discontinuity_time = + rte_rdtsc() - adapter->sys_up_time_start; } Thanks, Reshma diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_ethdev.h index 8352d0a..d242990 100644 --- a/drivers/net/e1000/e1000_ethdev.h +++ b/drivers/net/e1000/e1000_ethdev.h @@ -254,6 +254,62 @@ struct e1000_filter_info { struct e1000_2tuple_filter_list twotuple_list; }; +#define E1000_MIB_IF_TYPE_ETHERNETCSMACD 6 + +enum e1000_mib_truth_value { + e1000_mib_truth_true = 1, + e1000_mib_truth_false +}; 1)All enums should be in capital as per coding guidelines. Similarly correct author enums too. We no need to have enum identifier, just enumerator list should be suffice like below. So you can check all enumerations added newly and correct them? enum { enumerator-list } + +/* IF-MIB statistics */ +struct e1000_if_mib_stats { + uint64_t if_number; /* ifNumber */ + uint64_t if_index; /* ifIndex */ + uint64_t if_type; /* ifType */ + uint64_t if_mtu; /* ifMtu */ + uint64_t if_speed; /* ifSpeed */ + uint64_t if_phys_address; /* ifPhysAddress */ + uint64_t if_oper_status; /* ifOperStatus */ + uint64_t if_last_change; /* ifLastChange */ + uint64_t if_high_speed; /* ifHighSpeed */ + uint64_t if_connector_present; /* ifConnectorPresent */ + uint64_t if_counter_discontinuity_time; /* ifCounterDiscontinuityTime */ +}; + The comments should be inside /**< Here the comment goes. */ Refer other files to see how it is done. +#define E1000_DOT3_CF_PAUSE (1 << 0) /* PAUSE command implemented */ +#define E1000_DOT3_CF_MPCP (1 << 1) /* MPCP implemented */ +#define E1000_DOT3_CF_PFC (1 << 2) /* PFC implemented */ The comments should be inside /**< Here the comment goes. */ + uint64_t dot3_stats_rate_control_ability; + /* dot3StatsRateControlAbility */ + uint64_t dot3_stats_rate_control_status;/* dot3StatsRateControlStatus */ + uint64_t dot3_control_functions_supported; + /* dot3ControlFunctionsSupported */ +}; The comments line should go on top of the variables. + /* * Structure to store private data for each driver instance (for each port). */ @@ -268,6 +324,9 @@ struct e1000_adapter { struct rte_timecounter systime_tc; struct rte_timecounter rx_tstamp_tc; struct rte_timecounter tx_tstamp_tc; + uint64_t sys_up_time_start; + uint64_t if_last_change; + uint64_t if_counter_discontinuity_time;