Re: [PATCH v8 06/21] net/ntnic: add basic eth dev ops to ntnic

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

On 7/12/2024 4:47 PM, Serhii Iliushyk wrote:
> Adds support for eth_dev configure, start, stop, close, and infos_get.
> The internal structs of ntnic is also added and initialized.
> 
> Signed-off-by: Serhii Iliushyk <sil-plv@napatech.com>
> ---
> v6
> * Replace if_index with n_intf_no
> * Unnecessry resources free was fixed
> * Fix typo
> * Useless vars were removed

<...>

> +
> +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->n_intf_no;
>

I commented on this before, but 'if_index' is not a valid field for
physical devices, so it is wrong to set it.

What is the intention to set this value?

<...>

> +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->n_intf_no);
>

Why same value, 'n_intf_no', logged twice?

Btw, log says "Port", and "struct pmd_internals" has 'port_id' field but
it prints 'n_intf_no', is this intentionally?

"struct pmd_internals" has "int n_intf_no", "uint32_t port", "uint32_t
port_id", are these redundant fields?

<...>

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

This struct can be 'const'.

<...>

> @@ -58,6 +252,31 @@ 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;
> +
> +		/* 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);
>

What is hardcoded '0' for "(%i) NTNIC ..."

And normally number of Rx/Tx queues set by user via
'rte_eth_dev_configure()' API, this initialization has queue numbers
hardcoded as '1'. I assume this is for this initial version wher
configure support is missing, but just reminding here in any case.

> +		}
> +
> +		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);
>  
>  		if (!eth_dev) {
> @@ -66,9 +285,14 @@ nthw_pci_dev_init(struct rte_pci_device *pci_dev)
>  			return -1;
>  		}
>  
> -		NT_LOG_DBGX(DEBUG, NTNIC, "eth_dev %p, port_id %u, if_index %u\n",
> +		NT_LOG_DBGX(DEBUG, NTNIC, "eth_dev %p, port_id %u, n_intf_no %u\n",
>

Is above change intentional? Why not add the log correct at first place
instead of updating it here?