From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 19C2C455D4; Wed, 10 Jul 2024 16:30:49 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0AECF41101; Wed, 10 Jul 2024 16:30:49 +0200 (CEST) Received: from egress-ip42b.ess.de.barracuda.com (egress-ip42b.ess.de.barracuda.com [18.185.115.246]) by mails.dpdk.org (Postfix) with ESMTP id 5B54640FDE for ; Wed, 10 Jul 2024 16:30:46 +0200 (CEST) Received: from EUR05-AM6-obe.outbound.protection.outlook.com (mail-am6eur05lp2107.outbound.protection.outlook.com [104.47.18.107]) by mx-outbound22-81.eu-central-1b.ess.aws.cudaops.com (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 10 Jul 2024 14:30:44 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gSRnVnxYouS3MX/XdEaKu9Qf+NcpJz3LzQCJRm25u2EpOti1Qqne9MU4UcUvC8d8/BJR4UkE+q3t5alN0LDuUdeObC8qaXfh4oRzlLv0+5wq433L4AOOKGCGWPpZ12nzEckrGLECuEGOjhwH4y5IFjDh1uGRwDDytzAKkk+CXoUhuvNsL06dMOy/Qm3XjM7mOkETzVOEyec9Pd8mW88jWIrDYLXItqEGJW6xO5a6p7xogdkEAkcqA9EPAzOeFzvdVZQVgGmPLyIq3fVh9vaMIvZjGe9k3hGDGzmzMNBz5w7Ysiye++84Uq5Ph+4lPnMPPFA7tIs4Yc4+S8lCpzQM9g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=g6NNIh6HLgErGno7BrCcj0GtO9hMMOXof74xS5wSHpE=; b=HWpzUCWK2RmBEBw9aZ648nfXZBDvFCABmqs9I3zyKVf/5B3KObzrXSCJXcfY/V+8Z0hXL1LFbRa7CpqzHBkcq6RkZdc50SPb68hv1mg0OPmkQinV89CFpo4xX6oCWM0bd2jVaTGzOgVI6PuWXkxmZGClZ2x/6PtOmtmczW63dz7PojPrRG0JIbv1277R1ukeZLKquf7ndfEQE2jubuazTDzWvbSnFUBmtjLAPJ0EKOPzzLIY+JXdW5xrphZb63cPV4pN2dBLlU0qbHTD0P5nHXhKFPzwwyEqzZMG2Mv7io2anK/OaqTDIxPaDUBYZ2yer4D+yqs+rX2w0otkmtuJpA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=napatech.com; dmarc=pass action=none header.from=napatech.com; dkim=pass header.d=napatech.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=napatech.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=g6NNIh6HLgErGno7BrCcj0GtO9hMMOXof74xS5wSHpE=; b=G9zXknFl3l2b42xF9QGsyGo6urIolNGYwfQICf8Y9k7S7uHtqp8bY5lwEm4C7JB1PZJ0jYaw0l1a8HAMC9Ko4zyCJGJoQoa4uW0dxPlx0uGbmvj3wbZWbNXwjGxZLSL2RuJ6GsJJOjVh9STEtg6s205j0nouQTuSyc3FTNm7vMQ= Received: from VE1P190MB0830.EURP190.PROD.OUTLOOK.COM (2603:10a6:800:1a9::5) by PA6P190MB2070.EURP190.PROD.OUTLOOK.COM (2603:10a6:102:3cf::6) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7741.35; Wed, 10 Jul 2024 14:30:36 +0000 Received: from VE1P190MB0830.EURP190.PROD.OUTLOOK.COM ([fe80::fb19:d808:3eac:2ea3]) by VE1P190MB0830.EURP190.PROD.OUTLOOK.COM ([fe80::fb19:d808:3eac:2ea3%6]) with mapi id 15.20.7741.033; Wed, 10 Jul 2024 14:30:36 +0000 From: Serhii Iliushyk To: Ferruh Yigit , "dev@dpdk.org" CC: Mykola Kostenok , Christian Koue Muf , "andrew.rybchenko@oktetlabs.ru" , Uffe Jakobsen Subject: Re: [PATCH v5 03/23] net/ntnic: add minimal initialization for PCI device Thread-Topic: [PATCH v5 03/23] net/ntnic: add minimal initialization for PCI device Thread-Index: AQHayGUjdVDJ1z9qVEyr2UWz698MdbHnNqUAgAjiFHE= Date: Wed, 10 Jul 2024 14:30:36 +0000 Message-ID: References: <20240530144929.4127931-1-sil-plv@napatech.com> <20240627073918.2039719-1-sil-plv@napatech.com> <20240627073918.2039719-3-sil-plv@napatech.com> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-GB X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=napatech.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: VE1P190MB0830:EE_|PA6P190MB2070:EE_ x-ms-office365-filtering-correlation-id: 41f77cde-3768-48f4-7a7f-08dca0ecdd3c x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; ARA:13230040|1800799024|376014|366016|38070700018; x-microsoft-antispam-message-info: =?us-ascii?Q?aprLyfpDabQsQtPkt/aTjVwuc3NRvmdVzjnzONpFROFp72QFY88zdp5lSQDw?= =?us-ascii?Q?7MvHtRK79PHo8x5St3hryGl8e3kGQohg8eUdlLZw/LopYvM8O0ptT5oYZjDx?= =?us-ascii?Q?BmUXrl+bfC369MGVzM0+iFn1dIfaaYBeZc2R/qkxavOvYFHE4GlAkwQt77N7?= =?us-ascii?Q?nauoWuJQT+3gpEzsroVqi54Pbvhw/iDNPR3IlkQ2QKVbKsuW0D0USWdUdHSy?= =?us-ascii?Q?4B/eXTXvOl6KxLo4waECFiISJ4PdoepZs1spNTUnm5F8eJUcG3/nmCY28uo8?= =?us-ascii?Q?OKIEGFZ7mQvCYqn6JPAnprBV6rDhQ50LgVpxVAgIm9lxOW/lTR1QLyZDlQqT?= =?us-ascii?Q?mqL9yOCOhwxT+CpdQqnoOcrh93/yMjRmC/C5//IZY1RMllaOMysN+/88Ypyq?= =?us-ascii?Q?ETgUsCgePgNf4788FHYIQL1BQERT4wAKlKZOKFF1+Sn4wrFLknOxAVF99bkC?= =?us-ascii?Q?ojorJ1gxcFNPfpXnr+EyIWx1knvIS930H3YIqBOpNwG+Yy6cVZhbkmboz4P3?= =?us-ascii?Q?DCoE2M3NmW8AdyWGlkhd+Wps8QLmsYdbzL+g5F6cxgh4i5JHYlckzXG7BR28?= =?us-ascii?Q?y0EmjF076e/nneuSM5t+wSGmiPPDll1dLqVJv0Vffuqv5wUbXexLDYJa6CfY?= =?us-ascii?Q?CzEHuYsRmiOUcRRDcrXASXvvA2Q43Se0XFZ2wktwUlexym41EsVnfeCIZ73t?= =?us-ascii?Q?XqX8XlkdLJGZTi5gON1u/Qcktg13qmQDbD5tbY1agi1qpPTLk1g0OCIYUSnM?= =?us-ascii?Q?AD4bk30RaCpFFaQyMYWpe3LJBH5ZLgY8iDrfu0RAr0+Dygadtj7mH4pRvxAa?= =?us-ascii?Q?BnaQwJQrFuqIMGZ/sXlg6YHbQ2/sfbghz00P9pQzgbabNYeU+tHnxi0QS0ND?= =?us-ascii?Q?oKNmmQ1lC2V51e2rtzk7oWEFeyFJUUmN3jjhVlLKTG+RFFVE1EFo1KhWZz4L?= =?us-ascii?Q?wWs3KBr8vA06vNsIYpMRTe2K4j8QbH5otVdfmfmM0VWQT977+W+WkwEfBw1T?= =?us-ascii?Q?Kk6VKlQ+iJGXr3zkZddWrVYJTTS+ddLC8mRlbbt0GYYL3S6GkjLCrJdc0krB?= =?us-ascii?Q?UEIRTfslRoh1kHe3TpWBd57T/LXzQGKfDA+5de/jkxUd/HcyMn9kZCFX9o7N?= =?us-ascii?Q?7MGb5Myec2hzdrb7TGKikQ7Pw1S80YH1HCZiEcKxyNSH2U7NLXErzoNtZs/V?= =?us-ascii?Q?GWqhvBQOXfL3j2r9hMsA0uVjnisbu6IojXwlZfmEWulIMXdJGJaSo0Bvjn65?= =?us-ascii?Q?pXfg5eT4RbSqT2C6OpPNZGCtB00GmykAn1HUz4846PxORLfKXrMp5FeGVcMk?= =?us-ascii?Q?8HRtjwi7TXUDgdoZW2VAvxItAMCr/M8BHFK4kolfQfzFDQ=3D=3D?= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VE1P190MB0830.EURP190.PROD.OUTLOOK.COM; PTR:; CAT:NONE; SFS:(13230040)(1800799024)(376014)(366016)(38070700018); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?aTlNIwN6aQ4h29rqC5HSbC1FCKPYvCX9o5DN58bSGWN5GebV5CJhhdy8dpEz?= =?us-ascii?Q?FRThz9QITva0aoQVF8boEIgeWPlznNCGcOF5/Sm9cfe9XPc1bIYto345d6oI?= =?us-ascii?Q?AYaWwVQyjll/l9G0khX5QpTZX/liDLSqMEUcbmNXkNk0hYqq6Yd1JB0vE2vp?= =?us-ascii?Q?vz8Txt1BYk9tGcCM4txbI5l54egG/+WVvYDOsPMahtrdWUMTkHFmFvOCEQyr?= =?us-ascii?Q?fs1Lmmt2sJMYm6gYIT+aYJRisoHZHKD2AzJ8U+0tVSsIFJvxQgJkqq9H3/Ft?= =?us-ascii?Q?kEqkAQxWP91ZDbtiQyIFf6Q5IOI2CrjbAEtg6hZiJ2CLezu9gOiGaUi2FPjl?= =?us-ascii?Q?N+GnUYWzimED5iE1lLP4NxGsjcWOXw6uIKc4XGVYtkCKTZkfGSmPpWTKp69y?= =?us-ascii?Q?oaE9Z/CjkNEFKeufBlMGS4Vmw8F4zzsATjYauSv4/0Uzyg4Z2eWZQ7W9OpTm?= =?us-ascii?Q?ZkmasDl1WXPtDZJ92zB10+8eZsN4tomc0+CyUGr6cGw1g5wr85jCJXEyn1on?= =?us-ascii?Q?QdXV0K7MXVit2ye4ovkie2EtT6G8Y5UWttXExYeZN3T2floQSOv+KFJndm0q?= =?us-ascii?Q?cpnyqNRKasKMBe3kjr973nJQj/hSajfD6JmeP+ark+jkE/N4DiyFqanzHTos?= =?us-ascii?Q?CV0GkyHpCoaFpDDztKw8ZKSe7Mq/SoZ5P7uvXFNNsV/Qi5vg+brlW+CHJ7jd?= =?us-ascii?Q?8JH+wYjR0fy+WcXys7YCkvK04pfq0VUEAHioSL2IOVqWh9NCKx8YGNxyhTjV?= =?us-ascii?Q?BLc78DeRtlFMWJBJfmHun23rqjhGtIfm0pXvn6TQ5cumAHTLdld9beLSW0kE?= =?us-ascii?Q?VEp90g4usOWaUxBRQrPYv/VqvtXQg8qJyFLRIvgJhinVxfbDu0xIgSlmKAjy?= =?us-ascii?Q?xk+iP/a17LeDun0i7WbizMS4w+ibKIR1ikKdfH3GNMb4rOmBeWLxLJQ7Q+l3?= =?us-ascii?Q?4vZ0k4bwcKi9WjF7ndLjX4XenbDufL84iWJGP7z/LZm3jUgBTbkK3cAmScNA?= =?us-ascii?Q?DSr2dWdWxcYmPc3mOF+Zea/lKdHFyM3Fp35mMnYSLQ8HGN3W/EqMXU/zYPQM?= =?us-ascii?Q?rsXndgknBRy3CVU8Ht9MXZb2NLhE1AfMWCc2IufvrovDGzStAgt/aE/aBdEk?= =?us-ascii?Q?o83SWNIQwqLzsQrKE6HveOFhIil3Oenchz26jVrIWbzADrajXPUfTqcXkXn7?= =?us-ascii?Q?mpmjvqsE4cGyNkH1oj1rz/Pa4oQv1pN9LwI5iGOtUbBak6b09XnMUAbl7Ijo?= =?us-ascii?Q?EoJTC7fcPaqZksTFHnDKzUfV+8bxXVyZIAINCh030GuNiXw/6jQT5kIYnUQO?= =?us-ascii?Q?TJ+eOnh6nnsMvsnI9cLh76V3xmAZVWxoLaQPSYoGd0ngLLbe1PcnFJAVCkf2?= =?us-ascii?Q?DZORGjznmRwx93C55AZxdQN5C0JC1hq6gk2NCZL1BPzqdPZz91vfRIVh4DFs?= =?us-ascii?Q?HTs3GwbhW1X/36Vqo8XF0CMCIDt8NhQyQD3BoW4teevZTwllqvPWjarK/DgP?= =?us-ascii?Q?SKYLYLY8+QJDzZBTxy5zrh9OfC5kiwl2w2NKysRzviO6FZ58hrzAUYqVqKX5?= =?us-ascii?Q?HXwjl1Btf3Pwvf0DBPr+ASBzyJ/0gouZsyHKsfBUUqx/JcJzeIcQmqoRwdrB?= =?us-ascii?Q?LA=3D=3D?= Content-Type: multipart/alternative; boundary="_000_VE1P190MB083004D8E64806A4F2CA4D3580A42VE1P190MB0830EURP_" MIME-Version: 1.0 X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: FJs777l+84qaNdexIamNpo8H1JdDlLlG65qjI67AIwTCTZ8UPKje8sD1+pzimr/D0py8Tv8wndqXrThP3mxdVSc9N+mpmi490C9F9TUZ2U7DMlfhVk7PApJz5TER08DdAqqhl/w4YKt7xRDIeZY35GzxBhE/rTjGt24PTF8v5BszrXgVqMhBB407DKBGHbofwnhQ7wX/m8ru2EvFmzb+aV/cYT6nnaJLAFsHQ8sSRh07Ta0zbzRYUPuC5J57IGrVNf2uUlUmHyPK5TIW/8m4nv9c6h6+OUR4WAZzbt3iGLZqkgk4Ok/OEry2xYyEbU7QyNkhnIj6z4yOC2M3jY6F0/K5yxPl8HEvjXueTe4XEdcCtwZxapjGqvO7+zfg90gLAHJw4Yk8Fzno0EnQ8w/YLthcn54ysAl3PRY0zIBpzM9ibnkgoztlxY+f15jWUlF43TP5kbuHKHag0LQmtCUlG25bFbFZCV2byeLdl1loBGNG0c5V2yYflULpinnFQtpADA1QWDS0/hcj8wdxnJXY0ltDQJKyH1MbmnTG4f9Fx0BwCE5Gu9PqtPaIDJ6mCvYOa/xdqr1NHsw3zEzLko1HZyL2+rgXINFao/9xOiPAwRU2uXvtprCJhGxJkdqvKn+jvHiEgFBHkPhV4ERyUj4PfA== X-OriginatorOrg: napatech.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: VE1P190MB0830.EURP190.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-Network-Message-Id: 41f77cde-3768-48f4-7a7f-08dca0ecdd3c X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Jul 2024 14:30:36.3750 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: c4540d0b-728a-4233-9da5-9ea30c7ec3ed X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: pZbs8oJXRFIFPrM69gCh13+M5PhiXgu46ol87DY6iTbyerpEPMmYU5edFlhKuSl9yCOHQ/sciVO3aVYcnnQqCg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA6P190MB2070 X-BESS-ID: 1720621839-305713-12653-35857-2 X-BESS-VER: 2019.1_20240702.1505 X-BESS-Apparent-Source-IP: 104.47.18.107 X-BESS-Parts: H4sIAAAAAAACAzWMMQ6EMAwE/+KawrEd7PAVdEWMHNEgClKcdOLvpDia1Wik2f UH8e2wQB87wXnBYoo2aB9yZlQTQcwemmpzcpLsEhq2ad7gnt5+78e/T4XTe0DMc1hq4T VjI6rMUSyIsVRXYbg/D+AGgTKCAAAA X-BESS-Outbound-Spam-Score: 0.00 X-BESS-Outbound-Spam-Report: Code version 3.2, rules version 3.2.2.257531 [from cloudscan12-238.eu-central-1a.ess.aws.cudaops.com] Rule breakdown below pts rule name description ---- ---------------------- -------------------------------- 0.00 HTML_MESSAGE BODY: HTML included in message 0.00 BSF_BESS_OUTBOUND META: BESS Outbound X-BESS-Outbound-Spam-Status: SCORE=0.00 using account:ESS113687 scores of KILL_LEVEL=7.0 tests=HTML_MESSAGE, BSF_BESS_OUTBOUND X-BESS-BRTS-Status: 1 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --_000_VE1P190MB083004D8E64806A4F2CA4D3580A42VE1P190MB0830EURP_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ferruh, Please find our explanation for your comment according to device initializa= tion by function nthw_pci_dev_init. Our NIC handles multiple ports within the same PCI Bus Device Function (BDF= ) - hence, we need to create several ETH devices during the probe init. Allocation of multiple eth devices is handled inside the nthw_pci_dev_init(= ) function. It appears to be similar to what the Octeontx driver does. We believe the current implementation fits better for our PMD, so we would = like to keep it as is, unless it is a blocking requirement for upstreaming. Please let us know. BR, Serhii From: Ferruh Yigit Date: Friday, 5 July 2024 at 01:44 To: Serhii Iliushyk , dev@dpdk.org Cc: Mykola Kostenok , Christian Koue Muf , andrew.rybchenko@oktetlabs.ru Subject: Re: [PATCH v5 03/23] net/ntnic: add minimal initialization for PCI= device On 6/27/2024 8:38 AM, Serhii Iliushyk wrote: > add implementation for probe/init and remove/deinit of the PCI device > > Signed-off-by: Serhii Iliushyk > --- > drivers/net/ntnic/ntnic_ethdev.c | 104 ++++++++++++++++++++++++++++++- > 1 file changed, 103 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ntnic/ntnic_ethdev.c b/drivers/net/ntnic/ntnic_e= thdev.c > index 3079bd98e4..e9a584877f 100644 > --- a/drivers/net/ntnic/ntnic_ethdev.c > +++ b/drivers/net/ntnic/ntnic_ethdev.c > @@ -17,14 +17,63 @@ > /* Global static variables: */ > > static int > -nthw_pci_dev_init(struct rte_pci_device *pci_dev __rte_unused) > +nthw_pci_dev_init(struct rte_pci_device *pci_dev) > { > + uint32_t n_port_mask =3D -1; /* All ports enabled by default *= / > + int n_phy_ports; > + NT_LOG_DBGX(DEBUG, NTNIC, "Dev %s PF #%i Init : %02x:%02x:%i\n", pc= i_dev->name, > + pci_dev->addr.function, pci_dev->addr.bus, pci_dev->addr.de= vid, > + pci_dev->addr.function); > + > + n_phy_ports =3D 0; > + > + for (int n_intf_no =3D 0; n_intf_no < n_phy_ports; n_intf_no++) { > + struct rte_eth_dev *eth_dev =3D NULL; > + char name[32]; > + > + if ((1 << n_intf_no) & ~n_port_mask) > + continue; > + > + snprintf(name, sizeof(name), "ntnic%d", n_intf_no); > + > + eth_dev =3D rte_eth_dev_allocate(name); /* TODO: name */ > Is this TODO still valid? > + > + if (!eth_dev) { > + NT_LOG_DBGX(ERR, NTNIC, "%s: %s: error=3D%d\n", > + (pci_dev->name[0] ? pci_dev->name : "NA"), = name, -1); > + return -1; > + } > + > + NT_LOG_DBGX(DEBUG, NTNIC, "eth_dev %p, port_id %u, if_index= %u\n", > + eth_dev, eth_dev->data->port_id, n_= intf_no); > + > + > + struct rte_eth_link pmd_link; > + pmd_link.link_speed =3D RTE_ETH_SPEED_NUM_NONE; > + pmd_link.link_duplex =3D RTE_ETH_LINK_FULL_DUPLEX; > + pmd_link.link_status =3D RTE_ETH_LINK_DOWN; > + pmd_link.link_autoneg =3D RTE_ETH_LINK_AUTONEG; > + > + eth_dev->device =3D &pci_dev->device; > + eth_dev->data->dev_link =3D pmd_link; > + eth_dev->data->numa_node =3D pci_dev->device.numa_node; > rte_eth_copy_pci_info() should be setting numa_node, no need to duplicate here. Please consider using 'rte_eth_dev_create()' to help these kind of boilerplate initialization. I did same comment below. > + eth_dev->dev_ops =3D NULL; > + eth_dev->state =3D RTE_ETH_DEV_ATTACHED; > Shouldn't need to set state directly, please call 'rte_eth_dev_probing_finish()' as a last thing in probe(). This call will set the state, also will do some other required work. > + > + rte_eth_copy_pci_info(eth_dev, pci_dev); > + /* performs rte_eth_copy_pci_info() */ > + eth_dev_pci_specific_init(eth_dev, pci_dev); > As comment says, 'eth_dev_pci_specific_init()' calls the 'rte_eth_copy_pci_info()', so why calling it twice, can clean the init and remove the comment. > + > + /* increase initialized ethernet devices - PF */ > Is this comment valid here? > + } > + > return 0; > } > > static int > nthw_pci_dev_deinit(struct rte_eth_dev *eth_dev __rte_unused) > { > + NT_LOG_DBGX(DEBUG, NTNIC, "PCI device deinitialization\n"); > return 0; > } > > @@ -33,13 +82,65 @@ nthw_pci_probe(struct rte_pci_driver *pci_drv __rte_u= nused, > struct rte_pci_device *pci_dev) > { > int res; > + > + NT_LOG_DBGX(DEBUG, NTNIC, "pcidev: name: '%s'\n", pci_dev->name); > + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: name: '%s'\n", pci_dev->https:/= /linkprotect.cudasvc.com/url?a=3Dhttps%3a%2f%2fdevice.name&c=3DE,1,V5OBhPhf= iNSro_oj2bitVYkKYAABsPx-MKLgmN0q8g6JbMwgnOO1gKkhj3c3IxCQvEgNbC8ofBuQUVC8VRF= gpnK79cZnIRMu2iT6BvGGlWlO-BxzlAK2eTwkKk9z&typo=3D1); > + > + if (pci_dev->device.devargs) { > + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: args: '%s'\n", > + (pci_dev->device.devargs->args ? pci_dev->device.de= vargs->args : "NULL")); > + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: data: '%s'\n", > + (pci_dev->device.devargs->data ? pci_dev->device.de= vargs->data : "NULL")); > + } > + > + const int n_rte_has_pci =3D rte_eal_has_pci(); > + NT_LOG(DBG, NTNIC, "has_pci=3D%d\n", n_rte_has_pci); > + > + if (n_rte_has_pci =3D=3D 0) { > + NT_LOG(ERR, NTNIC, "has_pci=3D%d: this PMD needs hugepages\= n", n_rte_has_pci); > It is checking PCI bus, but log is about hugepages. > + return -1; > + } > What is the intention here for the 'n_rte_has_pci' check? If pci bus is disabled, this probe call should not be called at all, in that manner this check is useless. > + > + const int n_rte_vfio_no_io_mmu_enabled =3D rte_vfio_noiommu_is_enab= led(); > + NT_LOG(DBG, NTNIC, "vfio_no_iommu_enabled=3D%d\n", n_rte_vfio_no_io= _mmu_enabled); > + > + if (n_rte_vfio_no_io_mmu_enabled) { > + NT_LOG(ERR, NTNIC, "vfio_no_iommu_enabled=3D%d: this PMD ne= eds VFIO IOMMU\n", > + n_rte_vfio_no_io_mmu_enabled); > + return -1; > + } > + > + const enum rte_iova_mode n_rte_io_va_mode =3D rte_eal_iova_mode(); > + NT_LOG(DBG, NTNIC, "iova mode=3D%d\n", n_rte_io_va_mode); > + > + if (n_rte_io_va_mode !=3D RTE_IOVA_PA) { > + NT_LOG(WRN, NTNIC, "iova mode (%d) should be PA for perform= ance reasons\n", > + n_rte_io_va_mode); > + } > Is this comment valid? Won't iommu be used for address translation both IOVA_VA and IOVA_PA mode? How much performance improvement we are talking about? > + > + NT_LOG(DBG, NTNIC, > + "busid=3D" PCI_PRI_FMT > + " pciid=3D%04x:%04x_%04x:%04x locstr=3D%s @ numanode=3D%d: = drv=3D%s drvalias=3D%s\n", > + pci_dev->addr.domain, pci_dev->addr.bus, pci_dev->addr.devi= d, > + pci_dev->addr.function, pci_dev->id.vendor_id, pci_dev->id.= device_id, > + pci_dev->id.subsystem_vendor_id, pci_dev->id.subsystem_devi= ce_id, > + pci_dev->name[0] ? pci_dev->name : "NA", /* locstr *= / > + pci_dev->device.numa_node, > + pci_dev->driver->https://linkprotect.cudasvc.com/url?a=3Dht= tps%3a%2f%2fdriver.name&c=3DE,1,5_F-hniqJt0w3GPG-nclekQA5nc17FmonNihbX6JRyT= d2j6RA7sGlIJ9gBTq_T3p01-6DsO244rVP-3PFWsjaqFJ44V77omLRmCrWio_VmtBudxV30mhco= u9&typo=3D1 ? pci_dev->driver->https://linkprotect.cudasvc.com/url?a=3Dhttp= s%3a%2f%2fdriver.name&c=3DE,1,frkLGUymGHY5ydpNKtk3f74kNLD7KupomSqM7BLD7aOyg= 83VIXzNCZfCJI9PDNuIjnexaqLnATwKsfczOq1DvmsNBhMbEgTT0QrJ7RKHxIw,&typo=3D1 : = "NA", > + pci_dev->driver->driver.alias ? pci_dev->driver->driver.ali= as : "NA"); > + > + > res =3D nthw_pci_dev_init(pci_dev); > Instead of calling 'nthw_pci_dev_init()' directly, you can use 'rte_eth_dev_create()' and pass 'nthw_pci_dev_init()' as paramter, this helps on some set of boilerplate code. > + > + NT_LOG_DBGX(DEBUG, NTNIC, "leave: res=3D%d\n", res); > return res; > Doesn't really matter but mostly 'ret' is used as short version of "return value", what 'res' is? > } > > static int > nthw_pci_remove(struct rte_pci_device *pci_dev) > { > + NT_LOG_DBGX(DEBUG, NTNIC); > + > return rte_eth_dev_pci_generic_remove(pci_dev, nthw_pci_dev_deinit= ); > } > > @@ -48,6 +149,7 @@ static struct rte_pci_driver rte_nthw_pmd =3D { > .name =3D "net_ntnic", > }, > > + .drv_flags =3D RTE_PCI_DRV_NEED_MAPPING, > .probe =3D nthw_pci_probe, > .remove =3D nthw_pci_remove, > }; --_000_VE1P190MB083004D8E64806A4F2CA4D3580A42VE1P190MB0830EURP_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Ferruh,


Please find our explanation for your comment according to device initializa= tion by function nthw_pci_dev_init.

&nbs= p;

Our NIC handles multiple= ports within the same PCI Bus Device Function (BDF) - hence, we need to cr= eate several ETH devices during the probe init.
Allocation of multiple eth devices is handled inside the nthw_pci_dev_init(= ) function.
It appears to be similar to what the Octeontx driver does.

&nbs= p;

We believe the current i= mplementation fits better for our PMD, so we would like to keep it as is, u= nless it is a blocking requirement for upstreaming.

Please let us know.

&nbs= p;

BR,<= /o:p>

Serhii

 

From: Ferruh Yigit <ferruh.yigit@amd.com>
Date: Friday, 5 July 2024 at 01:44
To: Serhii Iliushyk <sil-plv@napatech.com>, dev@dpdk.org <d= ev@dpdk.org>
Cc: Mykola Kostenok <mko-plv@napatech.com>, Christian Koue Muf= <ckm@napatech.com>, andrew.rybchenko@oktetlabs.ru <andrew.rybchen= ko@oktetlabs.ru>
Subject: Re: [PATCH v5 03/23] net/ntnic: add minimal initialization = for PCI device

On 6/27/2024 8:38 AM, Serhii Iliushyk wrote:
> add implementation for probe/init and remove/deinit of the PCI device<= br> >
> Signed-off-by: Serhii Iliushyk <sil-plv@napatech.com>
> ---
>  drivers/net/ntnic/ntnic_ethdev.c | 104 +++++++++++++++++++++++++= +++++-
>  1 file changed, 103 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ntnic/ntnic_ethdev.c b/drivers/net/ntnic/ntni= c_ethdev.c
> index 3079bd98e4..e9a584877f 100644
> --- a/drivers/net/ntnic/ntnic_ethdev.c
> +++ b/drivers/net/ntnic/ntnic_ethdev.c
> @@ -17,14 +17,63 @@
>  /* Global static variables: */
> 
>  static int
> -nthw_pci_dev_init(struct rte_pci_device *pci_dev __rte_unused)
> +nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  {
> +     uint32_t n_port_mask =3D -1;  &nbs= p;   /* All ports enabled by default */
> +     int n_phy_ports;
> +     NT_LOG_DBGX(DEBUG, NTNIC, "Dev %s PF #%= i Init : %02x:%02x:%i\n", pci_dev->name,
> +           &nb= sp; pci_dev->addr.function, pci_dev->addr.bus, pci_dev->addr.devid= ,
> +           &nb= sp; pci_dev->addr.function);
> +
> +     n_phy_ports =3D 0;
> +
> +     for (int n_intf_no =3D 0; n_intf_no < n_p= hy_ports; n_intf_no++) {
> +           &nb= sp; struct rte_eth_dev *eth_dev =3D NULL;
> +           &nb= sp; char name[32];
> +
> +           &nb= sp; if ((1 << n_intf_no) & ~n_port_mask)
> +           &nb= sp;         continue;
> +
> +           &nb= sp; snprintf(name, sizeof(name), "ntnic%d", n_intf_no);
> +
> +           &nb= sp; eth_dev =3D rte_eth_dev_allocate(name);   /* TODO: name */ >

Is this TODO still valid?

> +
> +           &nb= sp; if (!eth_dev) {
> +           &nb= sp;         NT_LOG_DBGX(ERR, NTNIC,= "%s: %s: error=3D%d\n",
> +           &nb= sp;            =      (pci_dev->name[0] ? pci_dev->name : "NA= "), name, -1);
> +           &nb= sp;         return -1;
> +           &nb= sp; }
> +
> +           &nb= sp; NT_LOG_DBGX(DEBUG, NTNIC, "eth_dev %p, port_id %u, if_index %u\n&q= uot;,
> +           &nb= sp;            =              et= h_dev, eth_dev->data->port_id, n_intf_no);
> +
> +
> +           &nb= sp; struct rte_eth_link pmd_link;
> +           &nb= sp; pmd_link.link_speed =3D RTE_ETH_SPEED_NUM_NONE;
> +           &nb= sp; pmd_link.link_duplex =3D RTE_ETH_LINK_FULL_DUPLEX;
> +           &nb= sp; pmd_link.link_status =3D RTE_ETH_LINK_DOWN;
> +           &nb= sp; pmd_link.link_autoneg =3D RTE_ETH_LINK_AUTONEG;
> +
> +           &nb= sp; eth_dev->device =3D &pci_dev->device;
> +           &nb= sp; eth_dev->data->dev_link =3D pmd_link;
> +           &nb= sp; eth_dev->data->numa_node =3D pci_dev->device.numa_node;
>

rte_eth_copy_pci_info() should be setting numa_node, no need to
duplicate here.

Please consider using 'rte_eth_dev_create()' to help these kind of
boilerplate initialization. I did same comment below.

> +           &nb= sp; eth_dev->dev_ops =3D NULL;
> +           &nb= sp; eth_dev->state =3D RTE_ETH_DEV_ATTACHED;
>

Shouldn't need to set state directly, please call
'rte_eth_dev_probing_finish()' as a last thing in probe().
This call will set the state, also will do some other required work.

> +
> +           &nb= sp; rte_eth_copy_pci_info(eth_dev, pci_dev);
> +           &nb= sp; /* performs rte_eth_copy_pci_info() */
> +           &nb= sp; eth_dev_pci_specific_init(eth_dev, pci_dev);
>

As comment says, 'eth_dev_pci_specific_init()' calls the
'rte_eth_copy_pci_info()', so why calling it twice, can clean the init
and remove the comment.

> +
> +           &nb= sp; /* increase initialized ethernet devices - PF */
>

Is this comment valid here?

> +     }
> +
>        return 0;
>  }
> 
>  static int
>  nthw_pci_dev_deinit(struct rte_eth_dev *eth_dev __rte_unused) >  {
> +     NT_LOG_DBGX(DEBUG, NTNIC, "PCI device d= einitialization\n");
>        return 0;
>  }
> 
> @@ -33,13 +82,65 @@ nthw_pci_probe(struct rte_pci_driver *pci_drv __rt= e_unused,
>        struct rte_pci_device *pci_d= ev)
>  {
>        int res;
> +
> +     NT_LOG_DBGX(DEBUG, NTNIC, "pcidev: name= : '%s'\n", pci_dev->name);
> +     NT_LOG_DBGX(DEBUG, NTNIC, "devargs: nam= e: '%s'\n", pci_dev->https://linkprotect.cudasvc.com/url?a=3Dhttps%= 3a%2f%2fdevice.name&c=3DE,1,V5OBhPhfiNSro_oj2bitVYkKYAABsPx-MKLgmN0q8g6= JbMwgnOO1gKkhj3c3IxCQvEgNbC8ofBuQUVC8VRFgpnK79cZnIRMu2iT6BvGGlWlO-BxzlAK2eT= wkKk9z&typo=3D1);
> +
> +     if (pci_dev->device.devargs) {
> +           &nb= sp; NT_LOG_DBGX(DEBUG, NTNIC, "devargs: args: '%s'\n",
> +           &nb= sp;         (pci_dev->device.dev= args->args ? pci_dev->device.devargs->args : "NULL")); > +           &nb= sp; NT_LOG_DBGX(DEBUG, NTNIC, "devargs: data: '%s'\n",
> +           &nb= sp;         (pci_dev->device.dev= args->data ? pci_dev->device.devargs->data : "NULL")); > +     }
> +
> +     const int n_rte_has_pci =3D rte_eal_has_pci(= );
> +     NT_LOG(DBG, NTNIC, "has_pci=3D%d\n"= ;, n_rte_has_pci);
> +
> +     if (n_rte_has_pci =3D=3D 0) {
> +           &nb= sp; NT_LOG(ERR, NTNIC, "has_pci=3D%d: this PMD needs hugepages\n"= , n_rte_has_pci);
>

It is checking PCI bus, but log is about hugepages.

> +           &nb= sp; return -1;
> +     }
>

What is the intention here for the 'n_rte_has_pci' check?
If pci bus is disabled, this probe call should not be called at all, in
that manner this check is useless.

> +
> +     const int n_rte_vfio_no_io_mmu_enabled =3D r= te_vfio_noiommu_is_enabled();
> +     NT_LOG(DBG, NTNIC, "vfio_no_iommu_enabl= ed=3D%d\n", n_rte_vfio_no_io_mmu_enabled);
> +
> +     if (n_rte_vfio_no_io_mmu_enabled) {
> +           &nb= sp; NT_LOG(ERR, NTNIC, "vfio_no_iommu_enabled=3D%d: this PMD needs VFI= O IOMMU\n",
> +           &nb= sp;         n_rte_vfio_no_io_mmu_en= abled);
> +           &nb= sp; return -1;
> +     }
> +
> +     const enum rte_iova_mode n_rte_io_va_mode = =3D rte_eal_iova_mode();
> +     NT_LOG(DBG, NTNIC, "iova mode=3D%d\n&qu= ot;, n_rte_io_va_mode);
> +
> +     if (n_rte_io_va_mode !=3D RTE_IOVA_PA) {
> +           &nb= sp; NT_LOG(WRN, NTNIC, "iova mode (%d) should be PA for performance re= asons\n",
> +           &nb= sp;         n_rte_io_va_mode);
> +     }
>

Is this comment valid?
Won't iommu be used for address translation both IOVA_VA and IOVA_PA
mode? How much performance improvement we are talking about?

> +
> +     NT_LOG(DBG, NTNIC,
> +           &nb= sp; "busid=3D" PCI_PRI_FMT
> +           &nb= sp; " pciid=3D%04x:%04x_%04x:%04x locstr=3D%s @ numanode=3D%d: drv=3D%= s drvalias=3D%s\n",
> +           &nb= sp; pci_dev->addr.domain, pci_dev->addr.bus, pci_dev->addr.devid,<= br> > +           &nb= sp; pci_dev->addr.function, pci_dev->id.vendor_id, pci_dev->id.dev= ice_id,
> +           &nb= sp; pci_dev->id.subsystem_vendor_id, pci_dev->id.subsystem_device_id,=
> +           &nb= sp; pci_dev->name[0] ? pci_dev->name : "NA",  &nb= sp;     /* locstr */
> +           &nb= sp; pci_dev->device.numa_node,
> +           &nb= sp; pci_dev->driver->https://linkprotect.cudasvc.com/url?a=3Dhttps%3a= %2f%2fdriver.name&c=3DE,1,5_F-hniqJt0w3GPG-nclekQA5nc17FmonNihbX6JRyTd2= j6RA7sGlIJ9gBTq_T3p01-6DsO244rVP-3PFWsjaqFJ44V77omLRmCrWio_VmtBudxV30mhcou9= &typo=3D1 ? pci_dev->driver->https://linkprotect.cudasvc.com/url?= a=3Dhttps%3a%2f%2fdriver.name&c=3DE,1,frkLGUymGHY5ydpNKtk3f74kNLD7Kupom= SqM7BLD7aOyg83VIXzNCZfCJI9PDNuIjnexaqLnATwKsfczOq1DvmsNBhMbEgTT0QrJ7RKHxIw,= &typo=3D1 : "NA",
> +           &nb= sp; pci_dev->driver->driver.alias ? pci_dev->driver->driver.ali= as : "NA");
> +
> +
>        res =3D nthw_pci_dev_init(pc= i_dev);
>

Instead of calling 'nthw_pci_dev_init()' directly, you can use
'rte_eth_dev_create()' and pass 'nthw_pci_dev_init()' as paramter, this
helps on some set of boilerplate code.

> +
> +     NT_LOG_DBGX(DEBUG, NTNIC, "leave: res= =3D%d\n", res);
>        return res;
>

Doesn't really matter but mostly 'ret' is used as short version of
"return value", what 'res' is?


>  }
> 
>  static int
>  nthw_pci_remove(struct rte_pci_device *pci_dev)
>  {
> +     NT_LOG_DBGX(DEBUG, NTNIC);
> +
>        return rte_eth_dev_pci_gener= ic_remove(pci_dev, nthw_pci_dev_deinit);
>  }
> 
> @@ -48,6 +149,7 @@ static struct rte_pci_driver rte_nthw_pmd =3D {
>            = ;    .name =3D "net_ntnic",
>        },
> 
> +     .drv_flags =3D RTE_PCI_DRV_NEED_MAPPING,
>        .probe =3D nthw_pci_probe, >        .remove =3D nthw_pci_remove,=
>  };

--_000_VE1P190MB083004D8E64806A4F2CA4D3580A42VE1P190MB0830EURP_--