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-...@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->device.name);
> +
> + 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->driver.name ? pci_dev->driver->driver.name :
> "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,
> };
