From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v2 2/2] net/mlx5: free eth dev port in case of error Date: Mon, 7 May 2018 10:55:36 -0700 Message-ID: <20180507175534.GA42287@yongseok-MBP.local> References: <1525705833-8573-1-git-send-email-rasland@mellanox.com> <1525705833-8573-2-git-send-email-rasland@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: shahafs@mellanox.com, dev@dpdk.org, thomas@monjalon.net, ophirmu@mellanox.com To: Raslan Darawsheh Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0062.outbound.protection.outlook.com [104.47.2.62]) by dpdk.org (Postfix) with ESMTP id 1C4A32C6A for ; Mon, 7 May 2018 19:56:26 +0200 (CEST) Content-Disposition: inline In-Reply-To: <1525705833-8573-2-git-send-email-rasland@mellanox.com> 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 Mon, May 07, 2018 at 06:10:33PM +0300, Raslan Darawsheh wrote: Please refine the title. No use of 'eth dev...' > If something went wrong in mlx5 pci prop need to free the eth_dev > that was previously allocated during the port setup. Shouldn't it be sent to stable branches if it is a bug? > Signed-off-by: Raslan Darawsheh > > --- > v2 changes: > Reword the commit log. > --- > --- > drivers/net/mlx5/mlx5.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c > index 3831e3d..bffe90f 100644 > --- a/drivers/net/mlx5/mlx5.c > +++ b/drivers/net/mlx5/mlx5.c > @@ -1063,6 +1063,8 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused, > claim_zero(mlx5_glue->dealloc_pd(pd)); > if (ctx) > claim_zero(mlx5_glue->close_device(ctx)); > + if (eth_dev) > + rte_eth_dev_release_port(eth_dev); I worry about the case where secondary process has some failure above. Secondary process doesn't call rte_eth_dev_allocate() but rte_eth_dev_attach_secondary(). But, rte_eth_dev_release_port() clears the shared data, memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data)); Then, the primary process will be malfunctioning. This seems a bug in the rte_eth_dev_release_port(), diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index e5605242d..20180146b 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -352,7 +352,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev) eth_dev->state = RTE_ETH_DEV_UNUSED; - memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data)); + if (rte_eal_process_type() == RTE_PROC_PRIMARY) + memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data)); rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock); Or _detach_secondary() may be necessary. You might want to talk to Thomas because he is the maintainer of ethdev. Thanks, Yongseok