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 13CC91B2D5 for ; Mon, 12 Feb 2018 11:40:32 +0100 (CET) Received: by mail-wm0-f67.google.com with SMTP id 143so8680599wma.5 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=YxY24LlBmTRTu7csSiAiMTDoF3tJaUwA074FtbjGEhIXsLuS8J0RDSuE6xyqXoXG34 p3uytifDEbsPE57Ib4xPpz1aKPMFM1iJqJsfMKnt3Jim6lZq+hMSsyl6RsxAK6bzgFIM qEX6vtbIurDkStRckTJHo4Rk3278qTkb3+jNGwGpUnCv0hjqEUy7PPJmeupSaVgAnMuD vBk+xniTc1MgzcVt03iRIzrCRZfqJNMjZdfSnltdikEFOGLSkgyA+gLuDz794/9900A/ ALCffM3KIxzKey4ozEBOHCdWH5nSiLmm2vDgKfdtQL/Bbl4fUOCe5v+wV9ub7unjf7qe U/ZQ== X-Gm-Message-State: APf1xPD9FyoVdanQFky4SQerMnm8hn6yBvcbnK2MziYi5sKvEZAG2cZe Es+e+2TTZbEKYurpfApWoPeOSQ== 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-stable] [PATCH] net/mlx4: use PCI address as port name X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches 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