From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 50F8BA034F; Wed, 6 May 2020 14:35:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1BB551D8D0; Wed, 6 May 2020 14:35:02 +0200 (CEST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by dpdk.org (Postfix) with ESMTP id 815C21D153; Wed, 6 May 2020 14:35:00 +0200 (CEST) X-Originating-IP: 86.246.31.132 Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id B5956E0035; Wed, 6 May 2020 12:34:03 +0000 (UTC) Date: Wed, 6 May 2020 14:33:59 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Ferruh Yigit Cc: dev@dpdk.org, stable@dpdk.org, thomas@monjalon.net Message-ID: <20200506123344.6ui6wfhwevawbyoh@u256.net> References: <8aab6e5eb6d8d4769cd4e47a32403c836a13b5ef.1588705694.git.grive@u256.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v1 2/3] net/ring: fix eth_dev device pointer on allocation 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 06/05/20 12:48 +0100, Ferruh Yigit wrote: > On 5/5/2020 8:10 PM, Gaetan Rivet wrote: > > When a net_ring device is allocated, its device pointer is not set > > before calling rte_eth_dev_probing_finish, which is incorrect. > > > > The following: > > commit: 96cb19521147 ("net/ring: use EAL APIs in PMD specific API") > > commit: a6992e961050 ("net/ring: set ethernet device field") > > > > already attempted to fix this issue in 17.08, which was fine at the > > time. Adding the hook rte_eth_dev_probing_finish() however created this > > bug, as the eth_dev exposed when this hook is executed is expected to be > > complete. > > > > Remove the prior attempts to fix the issue in rte_pmd_ring_probe() and > > write the pointer properly in do_eth_dev_ring_create(). > > > > Cc: stable@dpdk.org > > Fixes: fbe90cdd776c ("ethdev: add probing finish function") > > Cc: ferruh.yigit@intel.com > > Cc: thomas@monjalon.net > > Signed-off-by: Gaetan Rivet > > <...> > > > @@ -325,10 +346,17 @@ do_eth_dev_ring_create(const char *name, > > data->kdrv = RTE_KDRV_NONE; > > data->numa_node = numa_node; > > > > - /* finally assign rx and tx ops */ > > + /* assign rx and tx ops */ > > eth_dev->rx_pkt_burst = eth_ring_rx; > > eth_dev->tx_pkt_burst = eth_ring_tx; > > > > + /* finally set the rte_device pointer in eth_dev. */ > > + eth_dev->device = ring_device_from_name(name); > > + if (eth_dev->device == NULL) { > > + rte_errno = ENODEV; > > + goto error; > > + } > > + > > rte_eth_dev_probing_finish(eth_dev); > > *eth_dev_p = eth_dev; > > Why not move the 'rte_eth_dev_probing_finish()' to the 'rte_pmd_ring_probe()', > below where 'eth_dev->device' set? Hi Ferruh, Sure it would work. The reason I did it this way is two-fold: * I disliked having two places where eth_dev->device was conditionally set. It makes it harder to read rte_pmd_ring_probe. * I was actually thinking, doing this patch, that we should modify rte_eth_dev_allocate() to take an rte_device as parameter, as all eth_dev are meant to be backed by an rte_device. Keeping this in mind, I meant to move writing the pointer closer to the rte_eth_dev_allocate() call. But you are right that it is needlessly verbose, using vdev_bus->find_device() to do this stuff. I'm ok with changing it as you described if you prefer. -- Gaëtan