From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=WQjCAhcLNAgQ4rkxzIjQAv4kY+quyWc2R8xFi9JnZ84=; b=CCf3eNKVlaAE2pull7aB6N/AghiLxxoCni42JrymwIPhz4DFzUy4iSE3CZTQYj05Cr lpYqhPaUAEMJkP0afl67SU79MnWos4+eVpeGx0iPHcvkKso88Ss82iPMYGmeBWa/I5U3 OHQ4Dvtelv0Q4w+Y7/zjW6YEvuEJMfwDvmiS7B2UEC0D+kOPReCqa9jV1NvOSnvXZHEK jtQUGJlRQh6CBAKCWIJswmUsScjqxYygSWE+fWEtIoaj94KQENCSykonrU0uWwS4FY5g lqkMyyuBRmeQ6oJ/xo5ZiMPstaP6UMnBncJksdJ0wpQGr8CY2HeCSxF/dLySk/9H5AEP mcag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=WQjCAhcLNAgQ4rkxzIjQAv4kY+quyWc2R8xFi9JnZ84=; b=qc+i1ZKFZrk60LA24YvqQalkuqeWbSObmCnQf8P8wJz3sXPW/KjbWa+YjQhrNW9yHb 4xpPYaRd81WTARlsdbmeo4FIT6cVlsChYnhwZq9VFzZ2hmit76tzUMoEk5NkaLEI0pMe ir8669G1tCz1/n4h0AOlp5grju7yN0ibfAPjxUHwlRE+ru4CIVI8xpAmGP3PM1Dg2A3q UcgQLxXNMok7sWkSk5Oe7D4wIh5FF1z+UIGVxaPDiSZhIrRdQeg2j0S4tO9rkJyNypIb eAg0DJZpjVXzTsKVSrIvNPRraEhD+eiCy4269OFPFZr/IN5fYLYkveT1Gghu8VF1XW02 8XoQ== X-Gm-Message-State: APf1xPAYvReRwnIY3LZghsSEVFl7BI7QNY3MLO1nV1ad66kexNVMRiOQ FPe4Z5vHT2O/UE/Xoi67ivoD+Q== X-Google-Smtp-Source: AH8x225IinWIWHIEq1nbaG4HLKbbyQ+wobgGVH0EPSv1o+dl7bXwn3cALU64lEcAFibLuGXIsFr2Fw== X-Received: by 10.28.69.87 with SMTP id s84mr2972202wma.107.1518432031753; Mon, 12 Feb 2018 02:40:31 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id d73sm7047453wma.16.2018.02.12.02.40.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Feb 2018 02:40:31 -0800 (PST) Date: Mon, 12 Feb 2018 11:40:18 +0100 From: Adrien Mazarguil To: Yuanhan Liu Cc: dev@dpdk.org, Nelio Laranjeiro , Yongseok Koh , Shahaf Shuler , stable@dpdk.org 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 Content-Disposition: inline In-Reply-To: <1518337772-24763-1-git-send-email-yliu@fridaylinux.org> Subject: Re: [dpdk-dev] [PATCH] net/mlx4: use PCI address as port name X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Feb 2018 10:40:32 -0000 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