From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 22/38] net/dpaa: add NXP DPAA PMD driver skeleton Date: Thu, 29 Jun 2017 19:59:56 +0530 Message-ID: <589e8109-196c-daef-855a-6239bb7e0f7e@nxp.com> References: <1497591668-3320-1-git-send-email-shreyansh.jain@nxp.com> <1497591668-3320-23-git-send-email-shreyansh.jain@nxp.com> <39ded621-6575-fdbe-8d7c-9f1f5dd6be6e@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Ferruh Yigit Return-path: Received: from NAM02-BL2-obe.outbound.protection.outlook.com (mail-bl2nam02on0040.outbound.protection.outlook.com [104.47.38.40]) by dpdk.org (Postfix) with ESMTP id 0337A532C for ; Thu, 29 Jun 2017 16:20:49 +0200 (CEST) In-Reply-To: <39ded621-6575-fdbe-8d7c-9f1f5dd6be6e@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Ferruh, I was almost wondering if this series has been forgotten. Thanks for the comprehensive review. My comments inline (and in some other mails): On Wednesday 28 June 2017 09:11 PM, Ferruh Yigit wrote: > On 6/16/2017 6:40 AM, Shreyansh Jain wrote: >> A skeleton which would be called after bus device scan. It currently >> fails to identify the device> >> Signed-off-by: Hemant Agrawal >> Signed-off-by: Shreyansh Jain > > <...> > >> + >> +/* Initialise a network interface */ >> +static int dpaa_eth_dev_init(struct rte_eth_dev *eth_dev __rte_unused) > > __rte_unused can be removed I will correct this. > > <...> > >> + >> +static int >> +rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv __rte_unused, >> + struct rte_dpaa_device *dpaa_dev) >> +{ >> + int diag; >> + int ret; >> + struct rte_eth_dev *eth_dev; >> + char ethdev_name[RTE_ETH_NAME_MAX_LEN]; >> + >> + PMD_INIT_FUNC_TRACE(); >> + >> + if (!is_global_init) { >> + /* One time load of Qman/Bman drivers */ >> + ret = qman_global_init(); >> + if (ret) { >> + PMD_DRV_LOG(ERR, "QMAN initialization failed: %d", >> + ret); >> + return ret; >> + } >> + ret = bman_global_init(); >> + if (ret) { >> + PMD_DRV_LOG(ERR, "BMAN initialization failed: %d", >> + ret); >> + return ret; >> + } >> + >> + is_global_init = 1; >> + } >> + >> + sprintf(ethdev_name, "%s", dpaa_dev->name); > > snprintf can be preferred Ok. Will fix this. > >> + >> + ret = rte_dpaa_portal_init((void *)1); >> + if (ret) { >> + PMD_DRV_LOG(ERR, "Unable to initialize portal"); >> + return ret; >> + } >> + >> + eth_dev = rte_eth_dev_allocate(ethdev_name); > > If this is done without RTE_PROC_PRIMARY check, this will cause > secondary to memset all device data. Agree. I will correct this. > > I am adding this because of below check, it multi process support is > intended, this also be protected. > >> + if (eth_dev == NULL) >> + return -ENOMEM; >> + >> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { >> + eth_dev->data->dev_private = rte_zmalloc( >> + "ethdev private structure", >> + sizeof(struct dpaa_if), >> + RTE_CACHE_LINE_SIZE); >> + if (!eth_dev->data->dev_private) { >> + PMD_INIT_LOG(CRIT, "Cannot allocate memzone for" >> + " private port data\n"); >> + rte_eth_dev_release_port(eth_dev); >> + return -ENOMEM; >> + } >> + } >> + >> + eth_dev->device = &dpaa_dev->device; >> + dpaa_dev->eth_dev = eth_dev; > > I thought "struct rte_dpaa_device" is bus device, like "struct > rte_pci_device", if so why it has link to the eth_dev? Yes, rte_dpaa_device ~ rte_pci_device. This is used to extract the eth_dev back while de-initializing the device. driver->remove = rte_dpaa_remove(rte_dpaa_device) // fetch rte_eth_dev from rte_dpaa_device `-> .eth_dev_stop(eth_dev) So, essentially reusing the internal eth_ops for cleaning up the device. > >> + eth_dev->data->rx_mbuf_alloc_failed = 0; > > not required, data already memset via rte_eth_dev_allocate() Ok. I will remove this. > >> + >> + /* Invoke PMD device initialization function */ >> + diag = dpaa_eth_dev_init(eth_dev); >> + if (diag) { >> + PMD_DRV_LOG(ERR, "Eth dev initialization failed: %d", ret); >> + return diag; >> + } >> + >> + PMD_DRV_LOG(DEBUG, "Eth dev initialized: %d\n", diag); >> + >> + return 0; >> +} >> + >> +static int >> +rte_dpaa_remove(struct rte_dpaa_device *dpaa_dev) >> +{ >> + struct rte_eth_dev *eth_dev; >> + >> + PMD_INIT_FUNC_TRACE(); >> + >> + eth_dev = dpaa_dev->eth_dev; > > can be: > eth_dev = rte_eth_dev_allocated(dpaa_dev->device.name); Hmm, ok, now I understand why you are inquiring about eth_dev being assigned in rte_dpaa_device. I will have a relook at this and fix if required. > >> + >> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) >> + rte_free(eth_dev->data->dev_private); >> + > > no pmd uninit() ? I will fix this. There is an internal commit that we had very recently (a miss in previous series). > >> + rte_eth_dev_release_port(eth_dev); >> + >> + return 0; >> +} > > <...> > >