<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="en-UA" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> Hi Ferruh,<o:p></o:p> Please find our explanation for your comment according to device initialization by function nthw_pci_dev_init.<o:p></o:p> <o:p> </o:p> 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.<o:p></o:p> <o:p> </o:p> 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.<o:p></o:p> Please let us know.<o:p></o:p> <o:p> </o:p> BR,<o:p></o:p> Serhii<o:p></o:p> <o:p> </o:p> <div id="mail-editor-reference-message-container"> <div> <div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm"> 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 <dev@dpdk.org> Cc: Mykola Kostenok <mko-plv@napatech.com>, Christian Koue Muf <ckm@napatech.com>, andrew.rybchenko@oktetlabs.ru <andrew.rybchenko@oktetlabs.ru> Subject: Re: [PATCH v5 03/23] net/ntnic: add minimal initialization for PCI device<o:p></o:p> </div> <div> 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 <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/ntnic_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 = -1; /* 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, > + pci_dev->addr.function, pci_dev->addr.bus, pci_dev->addr.devid, > + pci_dev->addr.function); > + > + n_phy_ports = 0; > + > + for (int n_intf_no = 0; n_intf_no < n_phy_ports; n_intf_no++) { > + struct rte_eth_dev *eth_dev = NULL; > + char name[32]; > + > + if ((1 << n_intf_no) & ~n_port_mask) > + continue; > + > + snprintf(name, sizeof(name), "ntnic%d", n_intf_no); > + > + eth_dev = rte_eth_dev_allocate(name); /* TODO: name */ > Is this TODO still valid? > + > + if (!eth_dev) { > + NT_LOG_DBGX(ERR, NTNIC, "%s: %s: error=%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 = RTE_ETH_SPEED_NUM_NONE; > + pmd_link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX; > + pmd_link.link_status = RTE_ETH_LINK_DOWN; > + pmd_link.link_autoneg = RTE_ETH_LINK_AUTONEG; > + > + eth_dev->device = &pci_dev->device; > + eth_dev->data->dev_link = pmd_link; > + eth_dev->data->numa_node = 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 = NULL; > + eth_dev->state = 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_unused, > 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=https%3a%2f%2fdevice.name&c=E,1,V5OBhPhfiNSro_oj2bitVYkKYAABsPx-MKLgmN0q8g6JbMwgnOO1gKkhj3c3IxCQvEgNbC8ofBuQUVC8VRFgpnK79cZnIRMu2iT6BvGGlWlO-BxzlAK2eTwkKk9z&typo=1); > + > + if (pci_dev->device.devargs) { > + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: args: '%s'\n", > + (pci_dev->device.devargs->args ? pci_dev->device.devargs->args : "NULL")); > + NT_LOG_DBGX(DEBUG, NTNIC, "devargs: data: '%s'\n", > + (pci_dev->device.devargs->data ? pci_dev->device.devargs->data : "NULL")); > + } > + > + const int n_rte_has_pci = rte_eal_has_pci(); > + NT_LOG(DBG, NTNIC, "has_pci=%d\n", n_rte_has_pci); > + > + if (n_rte_has_pci == 0) { > + NT_LOG(ERR, NTNIC, "has_pci=%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 = rte_vfio_noiommu_is_enabled(); > + NT_LOG(DBG, NTNIC, "vfio_no_iommu_enabled=%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=%d: this PMD needs VFIO IOMMU\n", > + n_rte_vfio_no_io_mmu_enabled); > + return -1; > + } > + > + const enum rte_iova_mode n_rte_io_va_mode = rte_eal_iova_mode(); > + NT_LOG(DBG, NTNIC, "iova mode=%d\n", n_rte_io_va_mode); > + > + if (n_rte_io_va_mode != RTE_IOVA_PA) { > + NT_LOG(WRN, NTNIC, "iova mode (%d) should be PA for performance 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=" PCI_PRI_FMT > + " pciid=%04x:%04x_%04x:%04x locstr=%s @ numanode=%d: drv=%s drvalias=%s\n", > + pci_dev->addr.domain, pci_dev->addr.bus, pci_dev->addr.devid, > + pci_dev->addr.function, pci_dev->id.vendor_id, pci_dev->id.device_id, > + pci_dev->id.subsystem_vendor_id, pci_dev->id.subsystem_device_id, > + pci_dev->name[0] ? pci_dev->name : "NA", /* locstr */ > + pci_dev->device.numa_node, > + pci_dev->driver->https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fdriver.name&c=E,1,5_F-hniqJt0w3GPG-nclekQA5nc17FmonNihbX6JRyTd2j6RA7sGlIJ9gBTq_T3p01-6DsO244rVP-3PFWsjaqFJ44V77omLRmCrWio_VmtBudxV30mhcou9&typo=1 ? pci_dev->driver->https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fdriver.name&c=E,1,frkLGUymGHY5ydpNKtk3f74kNLD7KupomSqM7BLD7aOyg83VIXzNCZfCJI9PDNuIjnexaqLnATwKsfczOq1DvmsNBhMbEgTT0QrJ7RKHxIw,&typo=1 : "NA", > + pci_dev->driver->driver.alias ? pci_dev->driver->driver.alias : "NA"); > + > + > res = 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=%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 = { > .name = "net_ntnic", > }, > > + .drv_flags = RTE_PCI_DRV_NEED_MAPPING, > .probe = nthw_pci_probe, > .remove = nthw_pci_remove, > };<o:p></o:p> </div> </div> </div> </div> </body> </html>