Re: [PATCH v5 06/23] net/ntnic: add NT NIC driver dependencies

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

On 6/27/2024 8:38 AM, Serhii Iliushyk wrote:
> Add structures and interfaces specific for NT smartNiC
> 

The commit log is not really descriptive, I can see patch improves
driver with struct eth_dev_ops.
And patch adds global 'struct drv_s' structure, I assume, as one pci
device can have multiple ethdev, this struct is common state for all
these ethdevs. But can you please describe it more.
And it is better if you can escape from a global variable state, and
keep state per device specific data if possible, and if make sense for
your specific usecase.

Also can you please describe what are the driver dependencies mentioned
in the patch title?

<...>

> +static int
> +eth_dev_infos_get(struct rte_eth_dev *eth_dev, struct rte_eth_dev_info *dev_info)
> +{
> +	struct pmd_internals *internals = (struct pmd_internals *)eth_dev->data->dev_private;
> +
> +	dev_info->if_index = internals->if_index;
>

if_index is used wrong, I commented same before.
As a physical driver, you can ignore if_index, it is used for virtual
drivers that has backed by linux interface.

> +	dev_info->driver_name = internals->name;
> +
> +	return 0;
> +}
> +
> +static int
> +eth_dev_configure(struct rte_eth_dev *eth_dev)
> +{
> +	struct pmd_internals *internals = (struct pmd_internals *)eth_dev->data->dev_private;
> +	struct drv_s *p_drv = internals->p_drv;
> +
> +	NT_LOG_DBGX(DEBUG, NTNIC, "Called for eth_dev %p\n", eth_dev);
> +
> +	p_drv->probe_finished = 1;
>

Why setting 'p_drv->probe_finished' in 'eth_dev_configure()'?
And won't multiple ethdev can share same 'p_drv', if so why updating it
per ethdev?

> +
> +	/* The device is ALWAYS running promiscuous mode. */
> +	eth_dev->data->promiscuous ^= ~eth_dev->data->promiscuous;
> +	return 0;
> +}
> +
> +static int
> +eth_dev_start(struct rte_eth_dev *eth_dev)
> +{
> +	struct pmd_internals *internals = (struct pmd_internals *)eth_dev->data->dev_private;
> +
> +	NT_LOG_DBGX(DEBUG, NTNIC, "Port %u, %u\n", internals->n_intf_no, internals->if_index);
> +
> +	return 0;
> +}
> +
> +static int
> +eth_dev_stop(struct rte_eth_dev *eth_dev)
> +{
> +	struct pmd_internals *internals = (struct pmd_internals *)eth_dev->data->dev_private;
> +
> +	NT_LOG_DBGX(DEBUG, NTNIC, "Port %u, %u\n",
> +		internals->n_intf_no, internals->if_index);
> +
> +	eth_dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN;
> +	return 0;
> +}
> +
> +static void
> +drv_deinit(struct drv_s *p_drv)
> +{
> +	if (p_drv == NULL)
> +		return;
> +	/*
> +	 * Mark the global pdrv for cleared. Used by some threads to terminate.
> +	 * 1 second to give the threads a chance to see the termonation.
>

typo termonation.

> +	 */
> +	clear_pdrv(p_drv);
> +	nt_os_wait_usec(1000000);
> +
> +	/* clean memory */
> +	rte_free(p_drv);
> +	p_drv = NULL;
>

Clearing local variable?

> +}
> +
> +static int
> +eth_dev_close(struct rte_eth_dev *eth_dev)
> +{
> +	struct pmd_internals *internals = (struct pmd_internals *)eth_dev->data->dev_private;
> +	struct drv_s *p_drv = internals->p_drv;
> +
> +	internals->p_drv = NULL;
> +
> +	rte_free(internals);
> +	internals = NULL;
> +	eth_dev->data->dev_private = NULL;
> +	eth_dev->data->mac_addrs = NULL;
> +
> +	rte_eth_dev_release_port(eth_dev);
>

'rte_eth_dev_release_port()' clears dev_private and mac_addrs, and more.
It is common to close() free driver specific resources, and remove()
does call close() and 'rte_eth_dev_release_port()' later to free common
resources.

> +
> +	/* decrease initialized ethernet devices */
> +	p_drv->n_eth_dev_init_count--;
> +
> +	/*
> +	 * rte_pci_dev has no private member for p_drv
> +	 * wait until all rte_eth_dev's are closed - then close adapters via p_drv
> +	 */
> +	if (!p_drv->n_eth_dev_init_count && p_drv)
> +		drv_deinit(p_drv);
> +
> +	return 0;
> +}
> +
> +static struct eth_dev_ops nthw_eth_dev_ops = {
> +	.dev_configure = eth_dev_configure,
> +	.dev_start = eth_dev_start,
> +	.dev_stop = eth_dev_stop,
> +	.dev_close = eth_dev_close,
> +	.dev_infos_get = eth_dev_infos_get,
> +};
>  
>  static int
>  nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  {
>  	nt_vfio_init();
>  
> +	struct drv_s *p_drv;
> +	ntdrv_4ga_t *p_nt_drv;
>  	uint32_t n_port_mask = -1;	/* All ports enabled by default */
> +	uint32_t nb_rx_queues = 1;
> +	uint32_t nb_tx_queues = 1;
>  	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);
>  
>  
> +	/* alloc */
> +	p_drv = rte_zmalloc_socket(pci_dev->name, sizeof(struct drv_s), RTE_CACHE_LINE_SIZE,
> +			pci_dev->device.numa_node);
> +
> +	if (!p_drv) {
> +		NT_LOG_DBGX(ERR, NTNIC, "%s: error %d\n",
> +			(pci_dev->name[0] ? pci_dev->name : "NA"), -1);
> +		return -1;
> +	}
> +
>  	/* Setup VFIO context */
>  	int vfio = nt_vfio_setup(pci_dev);
>  
>  	if (vfio < 0) {
>  		NT_LOG_DBGX(ERR, TNIC, "%s: vfio_setup error %d\n",
>  			(pci_dev->name[0] ? pci_dev->name : "NA"), -1);
> +		rte_free(p_drv);
>  		return -1;
>  	}
>  
> +	/* context */
> +	p_nt_drv = &p_drv->ntdrv;
> +
> +	p_drv->p_dev = pci_dev;
> +
> +	/* Set context for NtDrv */
> +	p_nt_drv->pciident = BDF_TO_PCIIDENT(pci_dev->addr.domain, pci_dev->addr.bus,
> +			pci_dev->addr.devid, pci_dev->addr.function);
> +
> +
> +	p_nt_drv->b_shutdown = false;
> +
> +	/* store context */
> +	store_pdrv(p_drv);
> +
>  	n_phy_ports = 0;
>  
>  	for (int n_intf_no = 0; n_intf_no < n_phy_ports; n_intf_no++) {
> +		struct pmd_internals *internals = NULL;
>  		struct rte_eth_dev *eth_dev = NULL;
>  		char name[32];
>  
> @@ -50,6 +253,33 @@ nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  
>  		snprintf(name, sizeof(name), "ntnic%d", n_intf_no);
>  
> +		internals = rte_zmalloc_socket(name, sizeof(struct pmd_internals),
> +				RTE_CACHE_LINE_SIZE, pci_dev->device.numa_node);
> +
> +		if (!internals) {
> +			NT_LOG_DBGX(ERR, NTNIC, "%s: %s: error=%d\n",
> +				(pci_dev->name[0] ? pci_dev->name : "NA"), name, -1);
> +			return -1;
> +		}
> +
> +		internals->pci_dev = pci_dev;
> +		internals->n_intf_no = n_intf_no;
> +		internals->if_index = n_intf_no;
> +
> +
> +		/* Setup queue_ids */
> +		if (nb_rx_queues > 1) {
> +			NT_LOG(DBG, NTNIC,
> +				"(%i) NTNIC configured with Rx multi queues. %i queues\n",
> +				0 /*port*/, nb_rx_queues);
> +		}
> +
> +		if (nb_tx_queues > 1) {
> +			NT_LOG(DBG, NTNIC,
> +				"(%i) NTNIC configured with Tx multi queues. %i queues\n",
> +				0 /*port*/, nb_tx_queues);
> +		}
> +
>  		eth_dev = rte_eth_dev_allocate(name);	/* TODO: name */
>  
>  		if (!eth_dev) {
> @@ -61,6 +291,11 @@ nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  		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);
>  
> +		/* connect structs */
> +		internals->p_drv = p_drv;
> +		eth_dev->data->dev_private = internals;
> +
> +		internals->port_id = eth_dev->data->port_id;
>  
>  		struct rte_eth_link pmd_link;
>  		pmd_link.link_speed = RTE_ETH_SPEED_NUM_NONE;
> @@ -71,7 +306,7 @@ nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  		eth_dev->device = &pci_dev->device;
>  		eth_dev->data->dev_link = pmd_link;
>  		eth_dev->data->numa_node = pci_dev->device.numa_node;
> -		eth_dev->dev_ops = NULL;
> +		eth_dev->dev_ops = &nthw_eth_dev_ops;
>  		eth_dev->state = RTE_ETH_DEV_ATTACHED;
>  
>  		rte_eth_copy_pci_info(eth_dev, pci_dev);
> @@ -79,8 +314,11 @@ nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  		eth_dev_pci_specific_init(eth_dev, pci_dev);
>  
>  		/* increase initialized ethernet devices - PF */
> +		p_drv->n_eth_dev_init_count++;
>  	}
>  
> +	p_drv->setup_finished = 1;
> +
>  	return 0;
>  }
>  
> @@ -156,6 +394,9 @@ nthw_pci_remove(struct rte_pci_device *pci_dev)
>  {
>  	NT_LOG_DBGX(DEBUG, NTNIC);
>  
> +	struct drv_s *p_drv = get_pdrv_from_pci(pci_dev->addr);
> +	drv_deinit(p_drv);
> +
>

You are not really removing the ethdevs, which should be done at this stage.

As one pci device has multiple ethdevs, need to figure them out in this
stage. Normally it is possible to get ethdev via
"rte_eth_dev_allocated(pci_dev->device.name)", but for your case this is
not possible.

octeontx (octeontx_ethdev.c) has similar usage, please check it. It does
iterates on names, for you this is "ntnic%d", and frees the valid ones.

<...>