Re: [dpdk-dev] [PATCH v2 11/19] net/bnxt: add support for xstats get by id

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/18/2017 4:17 PM, Ajit Khaparde wrote:
> This patch adds support for xstats_get_by_id/xstats_get_names_by_id.
> 
> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> 
> --
> v1->v2: incorporate review comments.
> ---
>  drivers/net/bnxt/bnxt_ethdev.c |  2 ++
>  drivers/net/bnxt/bnxt_stats.c  | 47 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/bnxt/bnxt_stats.h  |  5 +++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index cecdfde..6eed19c 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -1564,6 +1564,8 @@ static const struct eth_dev_ops bnxt_dev_ops = {
>  	.txq_info_get = bnxt_txq_info_get_op,
>  	.dev_led_on = bnxt_dev_led_on_op,
>  	.dev_led_off = bnxt_dev_led_off_op,
> +	.xstats_get_by_id = bnxt_dev_xstats_get_by_id_op,
> +	.xstats_get_names_by_id = bnxt_dev_xstats_get_names_by_id_op,
>  };
>  
>  static bool bnxt_vf_pciid(uint16_t id)
> diff --git a/drivers/net/bnxt/bnxt_stats.c b/drivers/net/bnxt/bnxt_stats.c
> index d7d0e35..89921fb 100644
> --- a/drivers/net/bnxt/bnxt_stats.c
> +++ b/drivers/net/bnxt/bnxt_stats.c
> @@ -358,3 +358,50 @@ void bnxt_dev_xstats_reset_op(struct rte_eth_dev *eth_dev)
>  	if (!(bp->flags & BNXT_FLAG_PORT_STATS))
>  		RTE_LOG(ERR, PMD, "Operation not supported\n");
>  }
> +
> +int bnxt_dev_xstats_get_by_id_op(struct rte_eth_dev *dev, const uint64_t *ids,
> +		uint64_t *values, unsigned int limit)
> +{
> +	/* Account for the Tx drop pkts aka the Anti spoof counter */

Is this comment valid?

> +	const unsigned int stat_cnt = RTE_DIM(bnxt_rx_stats_strings) +
> +				RTE_DIM(bnxt_tx_stats_strings) + 1;
> +	struct rte_eth_xstat xstats[stat_cnt];
> +	uint16_t i;
> +
> +	if (!ids)
> +		return bnxt_dev_xstats_get_op(dev, xstats, stat_cnt);

Why not just return "stat_cnt" ?

> +
> +	for (i = 0; i < limit; i++) {
> +		if (ids[i] >= stat_cnt) {
> +			RTE_LOG(ERR, PMD, "id value isn't valid");
> +			return -1;
> +		}
> +		values[i] = xstats[ids[i]].value;

Where xstats[] has been filled?

Same comments valid for bnxt_dev_xstats_get_names_by_id_op()

> +	}
> +	return stat_cnt;
> +}
> +

<...>