Re: [PATCH v5 03/23] net/ntnic: add minimal initialization for PCI device

[Ferruh Yigit <ferruh.yigit@amd.com>]

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->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,
>  };