Re: [dpdk-dev] [PATCH] net/mlx4: use PCI address as port name

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Yuanhan,

On Sun, Feb 11, 2018 at 04:29:32PM +0800, Yuanhan Liu wrote:
> It is suggested to use PCI BDF to identify a port for port addition
> in OVS-DPDK. While mlx5 has its own naming style: name it by ib dev
> name. This breaks the typical OVS DPDK use case and brings more puzzle
> to the end users.
> 
> To fix it, this patch changes it to use PCI BDF as the name, too.
> Judging the fact that there are 2 ports associated with one PCI for
> ConnectX-3, a postfix is needed. Thus, the final name looks like
> something below:
> 
>     0000:04:00.0-port0
>     0000:04:00.0-port1
> 
> Cc: stable@dpdk.org
> Signed-off-by: Yuanhan Liu <yliu@fridaylinux.org>

OK for this naming scheme, I have several comments regarding the rest of the
patch though, please see below.

> ---
>  drivers/net/mlx4/mlx4.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index ee93daf..eb8851c 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -497,6 +497,14 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
>  		struct priv *priv = NULL;
>  		struct rte_eth_dev *eth_dev = NULL;
>  		struct ether_addr mac;
> +		char name[RTE_ETH_NAME_MAX_LEN];
> +		int len;
> +
> +		len = snprintf(name, sizeof(name), PCI_PRI_FMT,
> +			 pci_dev->addr.domain, pci_dev->addr.bus,
> +			 pci_dev->addr.devid, pci_dev->addr.function);

pci_dev must be aligned with the contents of the opening "(".

This snprintf() should also be performed later (see below).

> +		if (device_attr.phys_port_cnt > 1)
> +			snprintf(name + len, sizeof(name), "-port%u", i);

Possible buffer overflow, should be sizeof(name) - len.

Are you sure two naming schemes are OK? How about always appending "-port%u"
regardless of whether the adapter has one or more ports? I'm fine with
either but would like to confirm it's really on purpose. Also see below.

>  

Unnecessary empty line.

>  		/* If port is not enabled, skip. */
>  		if (!(conf.ports.enabled & (1 << i)))

So this check as well as the remaining ones may cause the PMD to expose only
one physical port out of many, Maybe you want to drop the "-port%u" suffix
in such a case? While I don't recommend it, it may be consistent with
dropping the "-port%u" suffix for single-port adapters.

> @@ -605,14 +613,7 @@ mlx4_pci_probe(struct rte_pci_driver *pci_drv, struct rte_pci_device *pci_dev)
>  		/* Get actual MTU if possible. */
>  		mlx4_mtu_get(priv, &priv->mtu);
>  		DEBUG("port %u MTU is %u", priv->port, priv->mtu);
> -		/* from rte_ethdev.c */
> -		{
> -			char name[RTE_ETH_NAME_MAX_LEN];
> -
> -			snprintf(name, sizeof(name), "%s port %u",
> -				 mlx4_glue->get_device_name(ibv_dev), port);
> -			eth_dev = rte_eth_dev_allocate(name);
> -		}

I'm fine with removing the extra { } if you prefer to relocate name[] at the
beginning of the function, however in my opinion snprintf() should remain in
this area, i.e. no need to do it if the port is not enabled or usable.

Also since the last mlx4_glue->get_device_name() call is removed, you need
to remove it as well from mlx4_glue.h and mlx4_glue.c and update
LIB_GLUE_VERSION in the Makefile. Assuming this patch is targeted at 18.05:

 LIB_GLUE_VERSION = 18.05.0

> +		eth_dev = rte_eth_dev_allocate(name);
>  		if (eth_dev == NULL) {
>  			ERROR("can not allocate rte ethdev");
>  			rte_errno = ENOMEM;
> -- 
> 2.7.4
> 

-- 
Adrien Mazarguil
6WIND