From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Pattan, Reshma" Subject: Re: [PATCH 1/3] drivers/net: add support for IF-MIB and EtherLike-MIB for e1000 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Jain, Deepak K" , "Van Haaren, Harry" , "Azarewicz, PiotrX T" To: "Jastrzebski, MichalX K" , "dev@dpdk.org" Return-path: 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) In-Reply-To: <20170522143202.22424-2-michalx.k.jastrzebski@intel.com> Content-Language: en-US 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=20 Sent: Monday, May 22, 2017 3:32 PM To: dev@dpdk.org Cc: Pattan, Reshma ; Jain, Deepak K ; Van Haaren, Harry ; Jastrzebski, M= ichalX 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 a= dded 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? Eve= rything being displayed as xstats might not be correct. diff --git a/drivers/net/e1000/e1000_ethdev.h b/drivers/net/e1000/e1000_eth= dev.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; }; =20 +#define E1000_MIB_IF_TYPE_ETHERNETCSMACD 6 + +enum e1000_mib_truth_value { + e1000_mib_truth_true =3D 1, + e1000_mib_truth_false +}; 1)All enums should be in capital as per coding guidelines. Similarly correc= t 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 { =20 enumerator-list =20 } =20 =20 + +/* 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; }; =20 #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=20 eth_igb_xstats_reset(). + + adapter->if_counter_discontinuity_time =3D + rte_rdtsc() - adapter->sys_up_time_start; } Thanks, Reshma