Re: [dpdk-dev] [PATCH v2 12/40] bnxt: statistics operations

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, May 13, 2016 at 03:46:01PM -0700, Stephen Hurd wrote:
> Add get and clear staitstics operations and the asociated HWRM calls.
> 
> Signed-off-by: Stephen Hurd <stephen.hurd@broadcom.com>
> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> ---
>  drivers/net/bnxt/Makefile              |   1 +
>  drivers/net/bnxt/bnxt.h                |   5 +-
>  drivers/net/bnxt/bnxt_cpr.c            |   5 +-
>  drivers/net/bnxt/bnxt_cpr.h            |   2 -
>  drivers/net/bnxt/bnxt_ethdev.c         |   3 +
>  drivers/net/bnxt/bnxt_hwrm.c           |  49 ++++++++++++
>  drivers/net/bnxt/bnxt_hwrm.h           |   8 +-
>  drivers/net/bnxt/bnxt_rxq.c            |   1 +
>  drivers/net/bnxt/bnxt_stats.c          | 142 +++++++++++++++++++++++++++++++++
>  drivers/net/bnxt/bnxt_stats.h          |  44 ++++++++++
>  drivers/net/bnxt/bnxt_txq.c            |   1 +
>  drivers/net/bnxt/hsi_struct_def_dpdk.h | 107 +++++++++++++++++++++++++
>  12 files changed, 358 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/net/bnxt/bnxt_stats.c
>  create mode 100644 drivers/net/bnxt/bnxt_stats.h
> 
> diff --git a/drivers/net/bnxt/Makefile b/drivers/net/bnxt/Makefile
> index 21ed71c..f6a04f8 100644
> --- a/drivers/net/bnxt/Makefile
> +++ b/drivers/net/bnxt/Makefile
> @@ -54,6 +54,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_filter.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_hwrm.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_ring.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_rxq.c
> +SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_stats.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_txq.c
>  SRCS-$(CONFIG_RTE_LIBRTE_BNXT_PMD) += bnxt_vnic.c
>  
> diff --git a/drivers/net/bnxt/bnxt.h b/drivers/net/bnxt/bnxt.h
> index 38b590b..96f162e 100644
> --- a/drivers/net/bnxt/bnxt.h
> +++ b/drivers/net/bnxt/bnxt.h
> @@ -42,9 +42,6 @@
>  #include <rte_lcore.h>
>  #include <rte_spinlock.h>
>  
> -/* TODO make bnxt.def_cp_ring a pointer to avoid this... */
> -#include "bnxt_cpr.h"
> -

This fix doesn't seem to logically belong in this patch. See if it can be merged
in with the patch where the TODO was first raised.

>  #define BNXT_MAX_MTU		9000
>  #define VLAN_TAG_SIZE		4
>  
> @@ -141,7 +138,7 @@ struct bnxt {
>  	struct bnxt_tx_queue **tx_queues;
>  
>  	/* Default completion ring */
> -	struct bnxt_cp_ring_info	def_cp_ring;
> +	struct bnxt_cp_ring_info	*def_cp_ring;

Even though it's not strictly necessary, it might be nice to put in a dummy
forward definition of the bnxt_cp_ring_info before this structure to show that
it's a structure type that will be used later, but is opaque. i.e. put in a line
with:

"struct bnxt_cp_ring_info;"

/Bruce