From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH] net/mlx4: use PCI address as port name Date: Mon, 12 Feb 2018 11:40:18 +0100 Message-ID: <20180212104018.GG4256@6wind.com> References: <1518337772-24763-1-git-send-email-yliu@fridaylinux.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, Nelio Laranjeiro , Yongseok Koh , Shahaf Shuler , stable@dpdk.org To: Yuanhan Liu Return-path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id 178E21B300 for ; Mon, 12 Feb 2018 11:40:32 +0100 (CET) Received: by mail-wm0-f67.google.com with SMTP id f71so8703608wmf.0 for ; Mon, 12 Feb 2018 02:40:32 -0800 (PST) Content-Disposition: inline In-Reply-To: <1518337772-24763-1-git-send-email-yliu@fridaylinux.org> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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