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 11264A09FD; Fri, 18 Dec 2020 15:11:23 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 45B94CAD1; Fri, 18 Dec 2020 15:11:21 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 6765ECAC0 for ; Fri, 18 Dec 2020 15:11:18 +0100 (CET) IronPort-SDR: EaJF6YTafkdkET/cSEJ9tbxqSd7votU3Mr1MjERJr+d7i/8+V2Zm6YUAonDzX0ncZwcBmVd475 Ul6bWlsJYDVg== X-IronPort-AV: E=McAfee;i="6000,8403,9838"; a="162492092" X-IronPort-AV: E=Sophos;i="5.78,430,1599548400"; d="scan'208";a="162492092" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Dec 2020 06:11:17 -0800 IronPort-SDR: 9ygvgqAHbSWTS7ohJrI48QFTHZGnoEJiUETwi0hibYFoRRhGKvH6bgT91ZYs9nnVQC4fetEP05 qRQhOTwgaIZg== X-IronPort-AV: E=Sophos;i="5.78,430,1599548400"; d="scan'208";a="370605352" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.217.25]) ([10.213.217.25]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Dec 2020 06:11:16 -0800 From: Ferruh Yigit To: Lijun Ou Cc: "dev@dpdk.org" , "linuxarm@huawei.com" References: <1607846585-2381-1-git-send-email-oulijun@huawei.com> <1607846585-2381-6-git-send-email-oulijun@huawei.com> Message-ID: <5d570e4d-5acb-f8f6-b58e-58d7a34bff55@intel.com> Date: Fri, 18 Dec 2020 14:11:14 +0000 MIME-Version: 1.0 In-Reply-To: <1607846585-2381-6-git-send-email-oulijun@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 5/7] net/hns3: fix location of saving ethdev 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 12/13/2020 8:03 AM, Lijun Ou wrote: > From: "Min Hu (Connor)" > > In current version, procedure of saving eth_dev in > hns3 PMD init will be called more than twice, one > for primary, the other for secondary. That will cause > segmentation fault in Multi-process as eth_dev will > be changed in secondary process, which is different > from one in primary process. > This patch saved eth_dev in dev_private only in > primary process, as dev_private is shared by primary > process and secondary process. > > Fixes: 8929efbc1c46 ("net/hns3: fix FEC state query") > Cc: stable@dpdk.org > > Signed-off-by: Min Hu (Connor) > Signed-off-by: Lijun Ou > --- > drivers/net/hns3/hns3_ethdev.c | 3 +-- > drivers/net/hns3/hns3_ethdev_vf.c | 1 + > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/hns3/hns3_ethdev.c b/drivers/net/hns3/hns3_ethdev.c > index 7c34e38..f49b779 100644 > --- a/drivers/net/hns3/hns3_ethdev.c > +++ b/drivers/net/hns3/hns3_ethdev.c > @@ -6106,8 +6106,6 @@ hns3_dev_init(struct rte_eth_dev *eth_dev) > > PMD_INIT_FUNC_TRACE(); > > - hns->eth_dev = eth_dev; > - > eth_dev->process_private = (struct hns3_process_private *) > rte_zmalloc_socket("hns3_filter_list", > sizeof(struct hns3_process_private), > @@ -6134,6 +6132,7 @@ hns3_dev_init(struct rte_eth_dev *eth_dev) > return 0; > } > > + hns->eth_dev = eth_dev; > eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS; > The definition of the problem is correct, but with this update the 'hns->eth_dev' will be wrong for the secondary process. The initial problem was access to 'rte_eth_devices' global variable, which is wrong. But current approach can cause problem for the secondaries, moving 'eth_dev' to process private can work but before making things more complex, I would like to check why it is needed. The usage is as following: hns3_query_dev_fec_info(struct hns3_hw *hw) struct rte_eth_dev *eth_dev = hns->eth_dev; ret = hns3_fec_get(eth_dev, &pf->fec_mode); hns3_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa) struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private); Here since 'hns3_fec_get()' is dev_ops, it needs to get 'eth_dev' as parameter. Trying to use it internally forces to know 'eth_dev', but instead it can be possible to create an internal function that gets "struct hns3_hw" as parameter and it can be called internally without knowing 'eth_dev', and the .dev_ops can be wrapper to this, I mean something like this, what do you think: hns3_fec_get_internal(struct hns3_hw *hw, uint32_t *fec_capa); hns3_query_dev_fec_info(struct hns3_hw *hw) ret = hns3_fec_get_internal(hw, &pf->fec_mode); hns3_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa) struct hns3_hw *hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private); hns3_fec_get_internal(hw, fec_capa) There is other patch to switch to 'hns->eth_dev', I wonder can same approach be applied to those usages?