Re: [PATCH 1/3] drivers/net: add support for IF-MIB and EtherLike-MIB for e1000

["Pattan, Reshma" <reshma.pattan@intel.com>]

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 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_MIB_IF_TYPE_ETHERNETCSMACD	6
> +
> +enum e1000_mib_truth_value {
> +	e1000_mib_truth_true = 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. 
So you can check all enumerations added newly and correct them?
Ex:
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.

>  #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

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().

> stats->if_speed = (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 = adapter->if_last_change /
>			(rte_get_tsc_hz() * 100);

Can you clarify this logic?

Thanks,
Reshma