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 9CB9CA034E; Wed, 6 May 2020 19:33:09 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CF7A81DA80; Wed, 6 May 2020 19:33:07 +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 95E721DA7B; Wed, 6 May 2020 19:33:06 +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 BB0F1E0007; Wed, 6 May 2020 17:33:05 +0000 (UTC) Date: Wed, 6 May 2020 19:32: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: <20200506173259.yqulw5omozcw6szz@u256.net> References: <8aab6e5eb6d8d4769cd4e47a32403c836a13b5ef.1588705694.git.grive@u256.net> <20200506123344.6ui6wfhwevawbyoh@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] [dpdk-stable] [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 14:43 +0100, Ferruh Yigit wrote: > On 5/6/2020 1:33 PM, Gaëtan Rivet wrote: > > 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. > > Agree, what about using a 'goto' to have the assignment and > 'rte_eth_dev_probing_finish()' in a single place? > But check seems needed since creation may failed at that stage, if you think > better check can be done on 'ret' instead of 'eth_dev'... > > > > > * 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. > > That is a bigger change, may affect many (if not all) PMDs, so I think this can > be considered when that change is available. > > And although that change may fix the issues that 'eth_dev->device' is not set, > which we had a few times before, not sure it worth to change all PMDs and ethdev > API directly couple with rte_device, instead of PMD being the glue. Can be > discussed more on its own patch. > > > > > 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. > > > > That was the concern, that is too much code to take a value which is already > available a few level up the stack. Ok, future-proofing is a bad habit so let's not do it. I'm not a fan of the goto for the 'happy path' though. Another simple way would be to bring the vdev pointer with me as we go down the stack. I will send a v2 shortly, if it's too ugly to move the pointer down I'll use the goto, or if you tell me you'd prefer it. -- Gaëtan