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-...@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?
