From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 978231F5 for ; Mon, 22 May 2017 18:34:43 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 May 2017 09:34:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,377,1491289200"; d="scan'208";a="90310058" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga002.jf.intel.com with ESMTP; 22 May 2017 09:34:41 -0700 Received: from irsmsx110.ger.corp.intel.com ([169.254.15.151]) by IRSMSX107.ger.corp.intel.com ([169.254.10.107]) with mapi id 14.03.0319.002; Mon, 22 May 2017 17:34:40 +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/aprB3c6IAiosQ Date: Mon, 22 May 2017 16:34:39 +0000 Message-ID: <3AEA2BF9852C6F48A459DA490692831F0113CF3E@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-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjc0YmQ3ZDQtOWQ1NS00NTI3LTg3ZGUtODI3MmMzMGNkZTFjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlJ5UFZienNQMGVHUFBJTWRLZjRHQjBlZHF2bEJzZVBEeTlybkdUZHFWd2s9In0= x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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: , X-List-Received-Date: Mon, 22 May 2017 16:34:44 -0000 Hi, 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. > --- >=20 > +#define E1000_MIB_IF_TYPE_ETHERNETCSMACD 6 > + > +enum e1000_mib_truth_value { > + e1000_mib_truth_true =3D 1, > + e1000_mib_truth_false > +}; > + 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.=20 So you can check all enumerations added newly and correct them? Ex: enum { =20 enumerator-list =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. > #define E1000_DEV_PRIVATE(adapter) \ > diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethde= v.c > index e1702d8..705b591 100644 > --- a/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(). > stats->if_speed =3D (data->dev_link.link_speed < (UINT32_MAX / 1000000)) = ? > (data->dev_link.link_speed * 1000000) : UINT32_MAX; Can you clarify this link speed calculation logic > stats->if_last_change =3D adapter->if_last_change / > (rte_get_tsc_hz() * 100); Can you clarify this logic? Thanks, Reshma