[dpdk-dev] [PATCH v2 17/25] bnxt: add support for tx loopback, set vf mac and queues drop

[Ferruh Yigit]

On 5/26/2017 7:39 PM, Ajit Khaparde wrote:
> Add functions rte_pmd_bnxt_set_tx_loopback,
> rte_pmd_bnxt_set_all_queues_drop_en and
> rte_pmd_bnxt_set_vf_mac_addr to configure tx_loopback,
> queue_drop and VF MAC address setting in the hardware.
> It also adds the necessary functions to send the HWRM commands
> to the firmware.
> 
> Signed-off-by: Steeven Li <steeven.li at broadcom.com>
> Signed-off-by: Ajit Khaparde <ajit.khaparde at broadcom.com>
> 
> --
> v1->v2: regroup related patches and incorporate other review comments
> ---
>  drivers/net/bnxt/Makefile                 |   4 +
>  drivers/net/bnxt/bnxt_hwrm.c              | 107 ++++++++++++++++++++
>  drivers/net/bnxt/bnxt_hwrm.h              |   4 +
>  drivers/net/bnxt/rte_pmd_bnxt.c           | 163 ++++++++++++++++++++++++++++++
>  drivers/net/bnxt/rte_pmd_bnxt.h           |  87 ++++++++++++++++
>  drivers/net/bnxt/rte_pmd_bnxt_version.map |   9 ++
>  6 files changed, 374 insertions(+)
>  create mode 100644 drivers/net/bnxt/rte_pmd_bnxt.c
>  create mode 100644 drivers/net/bnxt/rte_pmd_bnxt.h

How did you test these new PMD specific APIs?

In testpmd there are already functions for these APIs, can you please
update testpmd to use these APIs, this also makes them compiled so it
will be easy to find any issues.

<...>


> +	if (req.vnic_id_tbl_addr == 0) {
> +		RTE_LOG(ERR, PMD,
> +		"unable to map VNIC ID table address to physical memory\n");
> +		//rte_free(vnic_ids);

Please remove unused code.

<...>

> +		if (vnic.mru == 4)	// Unallocated

And please don't use // comment style.

<...>

> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) Broadcom Limited.

Is year required here? I don't know what it is good for, but files tend
to have it.

<...>

> +int rte_pmd_bnxt_set_tx_loopback(uint8_t port, uint8_t on)
> +{
> +	struct rte_eth_dev *eth_dev;
> +	struct bnxt *bp;
> +	int rc;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> +
> +	if (on > 1)
> +		return -EINVAL;
> +
> +	eth_dev = &rte_eth_devices[port];

For PMD specific APIs, you need to add a protection against application
call the API with port_id of different vendor NIC.

Please check samples on ixgbe or i40e drivers.

<...>

> diff --git a/drivers/net/bnxt/rte_pmd_bnxt_version.map b/drivers/net/bnxt/rte_pmd_bnxt_version.map
> index 349c6e1..8b77163 100644
> --- a/drivers/net/bnxt/rte_pmd_bnxt_version.map
> +++ b/drivers/net/bnxt/rte_pmd_bnxt_version.map
> @@ -1,4 +1,13 @@
>  DPDK_16.04 {
>  
>  	local: *;
> +
> +};
> +
> +DPDK_17.08 {
> +	global:
> +
> +	rte_pmd_bnxt_set_tx_loopback;
> +	rte_pmd_bnxt_set_all_queues_drop_en;
> +	rte_pmd_bnxt_set_vf_mac_addr;
>  };

Since DPDK_16.04 doesn't have any APIs, I think it can be removed
completely this one replace it (meaning moving local: *; to here)