From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 59282A052A; Tue, 26 Jan 2021 16:25:53 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C811B140F8C; Tue, 26 Jan 2021 16:25:52 +0100 (CET) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id 2E6B6140F88 for ; Tue, 26 Jan 2021 16:25:50 +0100 (CET) IronPort-SDR: 5JNXARm+jsRFTd9PzHjHYzZF1BYhf7lc7srkipxI3J6vPZR2+XRbbwU6OnJGhWySGwMGwZq4PM eQSAV7dSB+sQ== X-IronPort-AV: E=McAfee;i="6000,8403,9876"; a="167589046" X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="167589046" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 07:25:49 -0800 IronPort-SDR: 5s5k+51t41xvlNhb28dMO8siwCYQI98xh/np+7BoGLFwdx2dVIhrsxD29HMnVoto0wXeqgGxMZ xM343yoQkD0A== X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="362020199" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.227.53]) ([10.213.227.53]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 07:25:46 -0800 To: Nalla Pradeep , Radha Mohan Chintakuntla , Veerasenareddy Burru , Anatoly Burakov Cc: jerinj@marvell.com, dev@dpdk.org, sburla@marvell.com References: <20210118093602.5449-1-pnalla@marvell.com> <20210118093602.5449-3-pnalla@marvell.com> From: Ferruh Yigit Message-ID: Date: Tue, 26 Jan 2021 15:25:43 +0000 MIME-Version: 1.0 In-Reply-To: <20210118093602.5449-3-pnalla@marvell.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 03/11] net/octeontx_ep: add device init and uninit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/18/2021 9:35 AM, Nalla Pradeep wrote: > Add basic init and uninit function which includes > initializing fields of ethdev private structure. > > Signed-off-by: Nalla Pradeep <...> ep/otx_ep_common.h > +++ b/drivers/net/octeontx_ep/otx_ep_common.h > @@ -4,11 +4,28 @@ > #ifndef _OTX_EP_COMMON_H_ > #define _OTX_EP_COMMON_H_ > > +#define otx_ep_printf(level, fmt, args...) \ > + rte_log(RTE_LOG_ ## level, RTE_LOGTYPE_PMD, \ > + fmt, ##args) Can you please register a new type specific to PMD, instead of using the 'RTE_LOGTYPE_PMD'? <...> > +static int > +otx_ep_chip_specific_setup(struct otx_ep_device *otx_epvf) > +{ > + struct rte_pci_device *pdev = otx_epvf->pdev; > + uint32_t dev_id = pdev->id.device_id; > + int ret; > + > + switch (dev_id) { > + case PCI_DEVID_OCTEONTX_EP_VF: > + otx_epvf->chip_id = dev_id; > + break; > + case PCI_DEVID_OCTEONTX2_EP_NET_VF: > + case PCI_DEVID_CN98XX_EP_NET_VF: > + otx_epvf->chip_id = dev_id; > + break; > + default: > + otx_ep_err("Unsupported device\n"); > + ret = -EINVAL; > + } > + > + if (!ret) > + otx_ep_info("OTX_EP dev_id[%d]\n", dev_id); 'ret' may be used uninitialized here. (I see it is fixed in later patches, but please fix here too) <...> > + > + return ret; > +} > + > +/* OTX_EP VF device initialization */ > +static int > +otx_epdev_init(struct otx_ep_device *otx_epvf) > +{ > + if (otx_ep_chip_specific_setup(otx_epvf)) { > + otx_ep_err("Chip specific setup failed\n"); > + goto setup_fail; > + } > + > + return 0; > + > +setup_fail: > + return -ENOMEM; Is 'NOMEM' correct return type, there seems nothing related to the memory. > +} > + > static int > otx_ep_eth_dev_uninit(struct rte_eth_dev *eth_dev) > { > - RTE_SET_USED(eth_dev); > + struct otx_ep_device *otx_epvf = OTX_EP_DEV(eth_dev); > + > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > + return 0; > + otx_epvf->port_configured = 0; > + > + if (eth_dev->data->mac_addrs != NULL) > + rte_free(eth_dev->data->mac_addrs); > After 'otx_ep_eth_dev_uninit()' the 'rte_eth_dev_release_port()' is called: rte_eth_dev_pci_generic_remove dev_uninit() rte_eth_dev_release_port() 'rte_eth_dev_pci_generic_remove()' also frees the 'eth_dev->data->mac_addrs' leading double free. You can either free yourself and explicitly set the pointer to NULL, or relay the freeing of it to the 'rte_eth_dev_pci_generic_remove()'. > - return -ENODEV; > + return 0; > } > > static int > otx_ep_eth_dev_init(struct rte_eth_dev *eth_dev) > { > - RTE_SET_USED(eth_dev); > + struct rte_pci_device *pdev = RTE_ETH_DEV_TO_PCI(eth_dev); > + struct otx_ep_device *otx_epvf = OTX_EP_DEV(eth_dev); > + unsigned char vf_mac_addr[RTE_ETHER_ADDR_LEN]; > > - return -ENODEV; > + /* Single process support */ > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) > + return 0; > + > + rte_eth_copy_pci_info(eth_dev, pdev); > + 'rte_eth_copy_pci_info()' already done before this point, as: rte_eth_dev_pci_generic_probe rte_eth_dev_pci_allocate rte_eth_copy_pci_info Can you please double check is the copying again required? > + if (pdev->mem_resource[0].addr) { > + otx_ep_info("OTX_EP BAR0 is mapped:\n"); > + } else { > + otx_ep_err("OTX_EP: Failed to map device BARs\n"); > + otx_ep_err("BAR0 %p\n", pdev->mem_resource[0].addr); > + return -ENODEV; > + } Why do you need to check if BAR0 is mapped or not, when it can be unmapped? > + otx_epvf->eth_dev = eth_dev; > + otx_epvf->port_id = eth_dev->data->port_id; > + eth_dev->data->mac_addrs = rte_zmalloc("otx_ep", RTE_ETHER_ADDR_LEN, 0); > + if (eth_dev->data->mac_addrs == NULL) { > + otx_ep_err("MAC addresses memory allocation failed\n"); > + return -ENOMEM; > + } > + rte_eth_random_addr(vf_mac_addr); > + memcpy(eth_dev->data->mac_addrs, vf_mac_addr, RTE_ETHER_ADDR_LEN); You can use 'rte_ether_addr_copy()' to copy mac. > + otx_epvf->hw_addr = pdev->mem_resource[0].addr; > + otx_epvf->pdev = pdev; > + > + otx_epdev_init(otx_epvf); > + otx_epvf->port_configured = 0; Is 'port_configured' used? It seems not checked at all. > + > + return 0; > } > > static int > @@ -42,7 +121,6 @@ otx_ep_eth_dev_pci_remove(struct rte_pci_device *pci_dev) > otx_ep_eth_dev_uninit); > } > > - Please fix these kind of things in the original patch that introduces it. > /* Set of PCI devices this driver supports */ > static const struct rte_pci_id pci_id_otx_ep_map[] = { > { RTE_PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVID_OCTEONTX_EP_VF) }, >