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 93BAEA0514; Wed, 15 Jan 2020 21:43:56 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 311941C0D7; Wed, 15 Jan 2020 21:43:55 +0100 (CET) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 482211C0D6; Wed, 15 Jan 2020 21:43:54 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 854D721F8E; Wed, 15 Jan 2020 15:43:52 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 15 Jan 2020 15:43:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=Nsgi1bZhtB1aWyWFK/xIqQMw5eR3UbeZLNfLVM2qaqE=; b=cb/gtozjdp2F mjUGH0xAB1743pu0mNNJs4GEcLDLDVhRquoi9+NDTYSInfuSIoadc75pH8GKOqqk s0NG9QJUe+jQs0L30qoZobwm8Z8SWQW6vfDyVz64sGu0LNfp01zvRtdkJDsqkAmj Ka4wcISIZE4jzYSRqeRkzUi87D3zQL0= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=Nsgi1bZhtB1aWyWFK/xIqQMw5eR3UbeZLNfLVM2qa qE=; b=WY7LB/AGR/jDbMl7ytuzXqsLnFI9YlqK8YsdAeo2W3AECL9p+BCHLqid9 i7B4N7uKZa2T7bQZTTsx7OBcwqfU92r0k89siJFIvxetW0mUQAr7aX9IwXbkglBm TLDmUYKpiOAjVCPPJurBWVtx5pgSgB3jg6OLFTe13pJWNSKj+Uh1CJDqxMbN66/1 AqzIgoljEIrASVHGXV2o1h1hH1GZfzS8kOQGCbb0dNNgfuqA0KovD2YtJZlrX/1R ibGRuf3a0bKc9rtdsszlTTzDrsgaCk0kntNFRBny2V7xAXUoGwwITXad4danBa+a yR3Aytu3YSYO1PqyHS7stHQEvrY0g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedrtdefgddufeelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtqhertddttdejnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecukf hppeejjedrudefgedrvddtfedrudekgeenucfrrghrrghmpehmrghilhhfrhhomhepthhh ohhmrghssehmohhnjhgrlhhonhdrnhgvthenucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 1B66480061; Wed, 15 Jan 2020 15:43:49 -0500 (EST) From: Thomas Monjalon To: Ferruh Yigit Cc: =?utf-8?B?5pa557uf5rWpNTA0NTA=?= , arybchenko@solarflare.com, dev@dpdk.org, stable@dpdk.org, jia.guo@intel.com, cunming.liang@intel.com, qi.z.zhang@intel.com, jungle845943968@outlook.com, Jerin Jacob Kollanukkaran Date: Wed, 15 Jan 2020 21:43:46 +0100 Message-ID: <3055448.clyjiGRsXx@xps> In-Reply-To: <32a5b3ff-afe2-75eb-f2cb-b9437a5a8d86@intel.com> References: <32a5b3ff-afe2-75eb-f2cb-b9437a5a8d86@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v2] Fixes: ethdev: secondary process change shared memory 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" 15/01/2020 19:35, Ferruh Yigit: > On 1/15/2020 6:49 AM, =E6=96=B9=E7=BB=9F=E6=B5=A950450 wrote: > > Hi Ferruh, thanks for your message. > >=20 > >=20 > > We developed a ethtool-dpdk which is secondary process based dpdk 17.08= version. Our device > > support hotplug detach, but hotplug deatch is failed when we use ethtoo= l-dpdk.We found the > > secondary process will change the shared memory when initializing.Secon= dary process calls > > "rte_eth_dev_pci_allocate" function and enters "rte_eth_copy_pci_info" = function. > > (rte_eth_dev_pci_generic_probe -> rte_eth_dev_pci_allocate -> rte_eth_c= opy_pci_info) > > Then it sets the value of struct "rte_eth_dev_data.dev_flags" to zero.I= n our platform, this value > > is equal to 0x0003.(RTE_ETH_DEV_DETACHABLE | RTE_ETH_DEV_INTR_LSC),but = after reset > > the "dev_flags", the value changed to 0x0002.(RTE_ETH_DEV_DETACHABLE).S= o, our device hotplug > > detach is failed.I found the similar problem in other dpdk version, inc= lude dpdk 19.11.Even though > > the deivce hotplug detach is discarded,but i think the shared memory ch= anged is unexpected by primary > > process. >=20 > I agree this is the problem. > In the driver code, 'rte_eth_copy_pci_info' is called only by primary pro= cess, > but the generic code is faulty. >=20 > And in 19.11 additionally 'eth_dev_pci_specific_init' also seems has same= problem. >=20 > > Our driver is ixgbe, i think this problem has a little relationship wit= h driver, Secondary process > > enters "rte_eth_copy_pci_info" by "rte_eth_dev_pci_allocate".And I agre= e your opinion, the helper > > function should simple on what it does.I have two ways to fix this prob= lem, one is add an if-statement > >=20 > > in "rte_eth_dev_pci_allocate" function to forbid secondary process ente= rs "rte_eth_copy_pci_info" function, > > another way is add an if-statement in "rte_eth_copy_pci_info" function = to forbid secondary process change > > shared memory.And First way need to ensure the "rte_eth_copy_pci_info" = function won't be called anywhere else. > > I think the second way is simple and lower risk. >=20 > Yes these are the two options. >=20 > I agree adding check in the 'rte_eth_copy_pci_info' covers all cases and = safer. > BUT my concern was adding decision making to simple/leaf function and mak= e it > harder to debug/use, instead of giving what primary/secondary process sho= uld > call decision in higher level. >=20 > But I just recognized that some PMDs are calling 'rte_eth_copy_pci_info' = on > secondary process, like mlx4 or szedata2, and most probably this is not t= heir > intention. > And 'eth_dev->intr_handle' set in 'rte_eth_copy_pci_info', not calling th= is > function may have side affect of 'eth_dev->intr_handle' not set in second= ary. >=20 > With above considerations I am OK to your proposal to cover all cases, Th= omas, > Andrew, any concern? Do you mean drivers need to be fixed?