From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by dpdk.org (Postfix) with ESMTP id BA9B21DF79 for ; Tue, 12 Jun 2018 15:20:22 +0200 (CEST) Received: by mail-wr0-f195.google.com with SMTP id f16-v6so24080693wrm.3 for ; Tue, 12 Jun 2018 06:20:22 -0700 (PDT) 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=8CzjI4OcwAZqiRisU70ZVw5GsF5rXi72ZmAewSU7ysE=; b=TZFBMpk4gPYg2wCG23FzTuXgIEun3aMR9q0vQOT7Uu7hfONZvcSGOaLsAa116ZVTOx uajdTq6D8q86jPSilVXTpaIuVPPf70AtuCykPj6Sli+/aKSAizCbJ5bNQtW85R3uWGaZ V6FbiC9DgnNxxmmhu3yRhM3nHD/LLQJNZWLoK0n63F5SA/EYBmKOUmAlK6QBtrJGHlAO q0wrBPfbSj08m9AehDE65lfoB//YJbLn6MyVIYtOg3yXG27CVMppzZjhGsY3XzAeExR/ Tgvxg7XTSrv4rNGKU6QlIyAvmovAmK5xsXOZCDr9A4JKKTBVRLzf4kKLBBzvm0NIo1cr XB5A== 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=8CzjI4OcwAZqiRisU70ZVw5GsF5rXi72ZmAewSU7ysE=; b=Z3J5K1YLm7umOYfu7+KEs8D6LiBaMwIljhoSlIWOum+1NHJpHP/NHViCQprbewwozF DqxtKoRFuxEkOPew6ZXuDdfeEZzhO1rtT1+dHAZuxXcvb/Ru6JZQkQqTAl987LZg4ei7 nn2QvZiep2W6k9tMPKoP9sx4pjt2UM/rr/BmUHlCLGu96OSM+QkGyeStiLgzGhhEKpfL mnJ/7xRrpQow4UdFX3HlScraaE/7ewJJQxpJhSMByyLoJr2ZMNtL6B1nT8PvmSUXf1xP XdXD9y8C9s/nmcaSsKujHbaklhHy6gJTgQ4XRQkzqxUeEHc6IscFUap04Ma6y0cNUrPJ +5+g== X-Gm-Message-State: APt69E3gd2IvdLPFFHVGb4dW8foM5eWnJTdlkC/SzYHtnvm7+MTP3KjT MKnHHpcdAtWiPyXMCOmojdPZMg== X-Google-Smtp-Source: ADUXVKKhiNSVdNSQ6+gt26aPosJrMFapTGhcQ6S6yQGjFFwPvaFtSwZNfLhbn94iBGmVCXWBkPJ12Q== X-Received: by 2002:adf:ab96:: with SMTP id s22-v6mr335510wrc.90.1528809622579; Tue, 12 Jun 2018 06:20:22 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id c11-v6sm166934wrm.65.2018.06.12.06.20.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 12 Jun 2018 06:20:21 -0700 (PDT) Date: Tue, 12 Jun 2018 15:20:06 +0200 From: Adrien Mazarguil To: "Xueming(Steven) Li" Cc: Shahaf Shuler , "dev@dpdk.org" Message-ID: <20180612132006.GW4025@6wind.com> References: <20180525161814.13873-1-adrien.mazarguil@6wind.com> <20180525161814.13873-4-adrien.mazarguil@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH 3/7] net/mlx5: split PCI from generic probing code 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: Tue, 12 Jun 2018 13:20:22 -0000 On Sun, Jun 10, 2018 at 12:59:06PM +0000, Xueming(Steven) Li wrote: > Hi Adrien, > > The logic looks much more clear now with the split. > > - len = snprintf(name, sizeof(name), PCI_PRI_FMT, > > - pci_dev->addr.domain, pci_dev->addr.bus, > > - pci_dev->addr.devid, pci_dev->addr.function); > > - if (attr.orig_attr.phys_port_cnt > 1) > > - snprintf(name + len, sizeof(name), " port %u", i); > > + if (attr->orig_attr.phys_port_cnt > 1) > > + snprintf(name, sizeof(name), "%s", dpdk_dev->name); > > + else > > + snprintf(name, sizeof(name), "%s port %u", > > + dpdk_dev->name, port); > > Name contains port only if phys_port_cnt > 1 in previous logic, are you sure? Nice catch, will fix it for v2. This wasn't noticed because this code is replaced in a subsequent patch of the series. > > + for (i = 0; i < attr.orig_attr.phys_port_cnt; ++i) { > > + eth_list[i] = mlx5_dev_spawn_one(dpdk_dev, ibv_dev, vf, > > + &attr, i + 1); > > + if (eth_list[i]) > > + continue; > > + /* Save rte_errno and roll back in case of failure. */ > > + ret = rte_errno; > > + while (i--) { > > + mlx5_dev_close(eth_list[i]); > > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) > > + rte_free(eth_list[i]->data->dev_private); > > + claim_zero(rte_eth_dev_release_port(eth_list[i])); > > + } > > + free(eth_list); > > + rte_errno = ret; > > + return NULL; > > The code is correct, but I personally prefer to move complex error handling to > dedicate "error:" block to make the code clear. Since it's the only place where this failure can occur, I'll leave it as is on the basis that doing so saves a goto statement. Those should be avoided where possible. It would have been a different story if the same error handling code was called from multiple places. > > +static int > > +mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused, > > + struct rte_pci_device *pci_dev) { > > + struct ibv_device **ibv_list; > > + struct rte_eth_dev **eth_list = NULL; > > + int vf; > > + int ret; > > + > > + assert(pci_drv == &mlx5_driver); > > + switch (pci_dev->id.device_id) { > > + case PCI_DEVICE_ID_MELLANOX_CONNECTX4VF: > > + case PCI_DEVICE_ID_MELLANOX_CONNECTX4LXVF: > > + case PCI_DEVICE_ID_MELLANOX_CONNECTX5VF: > > + case PCI_DEVICE_ID_MELLANOX_CONNECTX5EXVF: > > + vf = 1; > > + break; > > + default: > > + vf = 0; > > + } > > How about use a macro for vf detection and invoke in mlx5_dev_spawn_one(). > Seems it not used in outer callers. mlx5_dev_spawn_one() can be invoked with IB devices not backed by PCI (e.g. vdevs), for which the caller may still knowingly ask for VF behavior, either by user request or through other means. In this case, the caller happens to be a PCI probing function which, based on the device ID, easily determines whether VF behavior shall be requested. This is basically the only place where PCI device ID can be checked. Adding a macro here would only obfuscate this check. Adding it in mlx5_dev_spawn_one() would entangle PCI and generic code again, the opposite of the purpose of this patch, therefore I'll leave it unmodified for v2. -- Adrien Mazarguil 6WIND