Re: [dpdk-dev] [PATCH v2 04/17] net/ionic: register and initialize the adapter

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

On 10/15/2019 9:22 AM, Alfredo Cardigliano wrote:
> Register the Pensando ionic PMD (net_ionic) and define initial probe
> and remove callbacks with adapter initialization.
> 
> Signed-off-by: Alfredo Cardigliano <cardigliano@ntop.org>
> Reviewed-by: Shannon Nelson <snelson@pensando.io>
> ---
>  doc/guides/nics/features/ionic.ini |   2 +
>  drivers/net/ionic/Makefile         |   3 +
>  drivers/net/ionic/ionic.h          |  63 +++++++++++++
>  drivers/net/ionic/ionic_dev.c      | 128 ++++++++++++++++++++++++++
>  drivers/net/ionic/ionic_dev.h      | 138 ++++++++++++++++++++++++++++
>  drivers/net/ionic/ionic_ethdev.c   | 138 ++++++++++++++++++++++++++++
>  drivers/net/ionic/ionic_mac_api.c  |  61 +++++++++++++
>  drivers/net/ionic/ionic_mac_api.h  |  13 +++
>  drivers/net/ionic/ionic_main.c     | 133 +++++++++++++++++++++++++++
>  drivers/net/ionic/ionic_osdep.h    |  79 ++++++++++++++++
>  drivers/net/ionic/ionic_regs.h     | 142 +++++++++++++++++++++++++++++
>  drivers/net/ionic/meson.build      |   4 +
>  12 files changed, 904 insertions(+)
>  create mode 100644 drivers/net/ionic/ionic.h
>  create mode 100644 drivers/net/ionic/ionic_dev.c
>  create mode 100644 drivers/net/ionic/ionic_dev.h
>  create mode 100644 drivers/net/ionic/ionic_mac_api.c
>  create mode 100644 drivers/net/ionic/ionic_mac_api.h
>  create mode 100644 drivers/net/ionic/ionic_main.c
>  create mode 100644 drivers/net/ionic/ionic_osdep.h
>  create mode 100644 drivers/net/ionic/ionic_regs.h
> 

<...>

> +int
> +ionic_dev_setup(struct ionic_adapter *adapter)
> +{
> +	struct ionic_dev_bar *bar = adapter->bars;
> +	unsigned int num_bars = adapter->num_bars;
> +	struct ionic_dev *idev = &adapter->idev;
> +	uint32_t sig;
> +
> +	/* BAR0: dev_cmd and interrupts */
> +	if (num_bars < 1) {
> +		ionic_init_print(ERR, "No bars found, aborting\n");
> +		return -EFAULT;
> +	}
> +
> +	if (bar->len < IONIC_BAR0_SIZE) {
> +		ionic_init_print(ERR,
> +			"Resource bar size %lu too small, aborting\n",
> +			bar->len);
> +		return -EFAULT;
> +	}
> +
> +	idev->dev_info = bar->vaddr + IONIC_BAR0_DEV_INFO_REGS_OFFSET;
> +	idev->dev_cmd = bar->vaddr + IONIC_BAR0_DEV_CMD_REGS_OFFSET;
> +	idev->intr_status = bar->vaddr + IONIC_BAR0_INTR_STATUS_OFFSET;
> +	idev->intr_ctrl = bar->vaddr + IONIC_BAR0_INTR_CTRL_OFFSET;

These are causing build error with clang [1], can you please check?

[1]
.../drivers/net/ionic/ionic_dev.c:33:30: error: arithmetic on a pointer to void
is a GNU extension [-Werror,-Wpointer-arith]
        idev->dev_info = bar->vaddr + IONIC_BAR0_DEV_INFO_REGS_OFFSET;
                         ~~~~~~~~~~ ^
.../drivers/net/ionic/ionic_dev.c:34:29: error: arithmetic on a pointer to void
is a GNU extension [-Werror,-Wpointer-arith]
        idev->dev_cmd = bar->vaddr + IONIC_BAR0_DEV_CMD_REGS_OFFSET;
                        ~~~~~~~~~~ ^
.../drivers/net/ionic/ionic_dev.c:35:33: error: arithmetic on a pointer to void
is a GNU extension [-Werror,-Wpointer-arith]
        idev->intr_status = bar->vaddr + IONIC_BAR0_INTR_STATUS_OFFSET;
                            ~~~~~~~~~~ ^
.../drivers/net/ionic/ionic_dev.c:36:31: error: arithmetic on a pointer to void
is a GNU extension [-Werror,-Wpointer-arith]
        idev->intr_ctrl = bar->vaddr + IONIC_BAR0_INTR_CTRL_OFFSET;
                          ~~~~~~~~~~ ^

<...>

> @@ -0,0 +1,138 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + * Copyright(c) 2018-2019 Pensando Systems, Inc. All rights reserved.
> + */
> +
> +#ifndef _IONIC_DEV_H_
> +#define _IONIC_DEV_H_
> +
> +#include "ionic_osdep.h"
> +#include "ionic_if.h"
> +#include "ionic_regs.h"
> +
> +/** IONIC_API_VERSION - Version number of this interface.
> + *
> + * Any interface changes to this interface must also change the version.
> + *
> + * If drivers are compiled from different sources,
> + * they are compatible only if IONIC_API_VERSION is statically the same in both
> + * sources.  Drivers must have matching values of IONIC_API_VERSION at compile
> + * time, to be considered compatible at run time.
> + */
> +#define IONIC_API_VERSION		"3"

Does it make sense to put a table in to the documenation, to document which dpdk
version supports which API version?

<...>

> + * There is no room in struct rte_pci_driver to keep a reference
> + * to the adapter, using a static list for the time being.
> + */
> +static LIST_HEAD(ionic_pci_adapters_list, ionic_adapter) ionic_pci_adapters =
> +		LIST_HEAD_INITIALIZER(ionic_pci_adapters);

Why this list is used? Will holding the reference in the private data help?

> +static rte_spinlock_t ionic_pci_adapters_lock = RTE_SPINLOCK_INITIALIZER;
> +
> +static int
> +eth_ionic_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> +		struct rte_pci_device *pci_dev)
> +{
> +	struct rte_mem_resource *resource;
> +	struct ionic_adapter *adapter;
> +	struct ionic_hw *hw;
> +	unsigned long i;
> +	int err;
> +
> +	/* Multi-process not supported */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -EPERM;
> +
> +	ionic_init_print(DEBUG, "Initializing device %s %s",
> +			pci_dev->device.name,
> +			rte_eal_process_type() == RTE_PROC_SECONDARY ?
> +			"[SECONDARY]" : "");

According above check, it can't be secondary.

> +
> +	adapter = rte_zmalloc("ionic", sizeof(*adapter), 0);
> +
> +	if (!adapter) {
> +		ionic_init_print(ERR, "OOM");
> +		return -ENOMEM;
> +	}
> +
> +	adapter->pci_dev = pci_dev;
> +	hw = &adapter->hw;
> +
> +	hw->device_id = pci_dev->id.device_id;
> +	hw->vendor_id = pci_dev->id.vendor_id;
> +
> +	err = ionic_init_mac(hw);
> +	if (err != 0) {
> +		ionic_init_print(ERR, "Mac init failed: %d", err);

Should some clenaup done here? Same thing for all returns, should they free
allocated memory.

> +		return -EIO;
> +	}
> +
> +	adapter->is_mgmt_nic = (pci_dev->id.device_id == IONIC_DEV_ID_ETH_MGMT);
> +
> +	adapter->num_bars = 0;
> +	for (i = 0; i < PCI_MAX_RESOURCE && i < IONIC_BARS_MAX; i++) {
> +		resource = &pci_dev->mem_resource[i];
> +		if (resource->phys_addr == 0 || resource->len == 0)
> +			continue;
> +		adapter->bars[adapter->num_bars].vaddr = resource->addr;
> +		adapter->bars[adapter->num_bars].bus_addr = resource->phys_addr;
> +		adapter->bars[adapter->num_bars].len = resource->len;
> +		adapter->num_bars++;
> +	}
> +
> +	/* Discover ionic dev resources */
> +
> +	err = ionic_setup(adapter);
> +	if (err) {
> +		ionic_init_print(ERR, "Cannot setup device: %d, aborting", err);
> +		return err;
> +	}
> +
> +	err = ionic_identify(adapter);
> +	if (err) {
> +		ionic_init_print(ERR, "Cannot identify device: %d, aborting",
> +				err);

According dpdk coding convention, new line should have a tap. I can see you are
using a mixed format but since this is new code, it is good to have a clean code
that applies the coding convention:
https://doc.dpdk.org/guides/contributing/coding_style.html

For the shared code, shared with other platforms, looks like your 'ionic_if.h'
code is like that, we are more flexible to prevent additional overhead of
maintaining different versions just for the syntax changes, but for the code
that is for the DPDK only please follow the DPDK coding convention.

> +		return err;
> +	}
> +
> +	err = ionic_init(adapter);
> +	if (err) {
> +		ionic_init_print(ERR, "Cannot init device: %d, aborting", err);
> +		return err;
> +	}
> +
> +	rte_spinlock_lock(&ionic_pci_adapters_lock);
> +	LIST_INSERT_HEAD(&ionic_pci_adapters, adapter, pci_adapters);
> +	rte_spinlock_unlock(&ionic_pci_adapters_lock);
> +
> +	return 0;
> +}
> +
> +static int
> +eth_ionic_pci_remove(struct rte_pci_device *pci_dev)
> +{
> +	struct ionic_adapter *adapter = NULL;
> +
> +	rte_spinlock_lock(&ionic_pci_adapters_lock);
> +	LIST_FOREACH(adapter, &ionic_pci_adapters, pci_adapters) {
> +		if (adapter->pci_dev == pci_dev)
> +			break;
> +
> +		adapter = NULL;
> +	}
> +	if (adapter)
> +		LIST_REMOVE(adapter, pci_adapters);
> +	rte_spinlock_unlock(&ionic_pci_adapters_lock);

It is possible to get ethdev from pci_dev, and if you strore the adapter in
ethdev private data, you can access it without needing list for adapter, please
check other pysical device drivers, they have the sample.

> +
> +	if (adapter)
> +		rte_free(adapter);
> +
> +	return 0;
> +}
> +
> +static struct rte_pci_driver rte_ionic_pmd = {
> +	.id_table = pci_id_ionic_map,
> +	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> +	.probe = eth_ionic_pci_probe,
> +	.remove = eth_ionic_pci_remove,
> +};
> +
> +RTE_PMD_REGISTER_PCI(net_ionic, rte_ionic_pmd);
> +RTE_PMD_REGISTER_PCI_TABLE(net_ionic, pci_id_ionic_map);
> +RTE_PMD_REGISTER_KMOD_DEP(net_ionic, "* igb_uio | uio_pci_generic | vfio-pci");
> +
>  RTE_INIT(ionic_init_log)
>  {
>  	ionic_logtype_init = rte_log_register("pmd.net.ionic.init");
> @@ -14,6 +150,8 @@ RTE_INIT(ionic_init_log)
>  	if (ionic_logtype_init >= 0)
>  		rte_log_set_level(ionic_logtype_init, RTE_LOG_NOTICE);
>  
> +	ionic_struct_size_checks();
> +

For this check log init function doesn't look like correct place.

This fucntion is c constructor, it is called in the beggining of the
application, even though there is no ionic device attached at all, why you are
making all dpdk users check for it?
Also what will happend when check fails? Does it make sense to do this check in
probe() and return a fail if checks fail?

<...>