DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Alfredo Cardigliano <cardigliano@ntop.org>,
	John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	Anatoly Burakov <anatoly.burakov@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 04/17] net/ionic: register and initialize the adapter
Date: Mon, 2 Dec 2019 16:09:49 +0000
Message-ID: <40e1e2aa-bbbc-9384-a33e-f249b3157104@intel.com> (raw)
In-Reply-To: <20191015082235.28639-5-cardigliano@ntop.org>

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?

<...>

  reply index

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-15  8:22 [dpdk-dev] [PATCH v2 00/17] Introduces net/ionic PMD Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 01/17] net/ionic: add skeleton Alfredo Cardigliano
2019-12-02 16:06   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 02/17] net/ionic: add hardware structures definitions Alfredo Cardigliano
2019-12-02 16:07   ` Ferruh Yigit
2019-12-02 16:33   ` Stephen Hemminger
2019-12-04 16:26     ` Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 03/17] net/ionic: add log Alfredo Cardigliano
2019-12-02 16:07   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 04/17] net/ionic: register and initialize the adapter Alfredo Cardigliano
2019-12-02 16:09   ` Ferruh Yigit [this message]
2019-12-08 19:25     ` Alfredo Cardigliano
2019-12-09  9:12       ` Ferruh Yigit
2019-12-02 16:35   ` Stephen Hemminger
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 05/17] net/ionic: add port management commands Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 06/17] net/ionic: add basic lif support Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 07/17] net/ionic: add doorbells Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 08/17] net/ionic: add adminq support Alfredo Cardigliano
2019-12-02 16:15   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 09/17] net/ionic: add notifyq support Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 10/17] net/ionic: add basic port operations Alfredo Cardigliano
2019-12-02 16:11   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 11/17] net/ionic: add RX filters support Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 12/17] net/ionic: add Flow Control support Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 13/17] net/ionic: add RX and TX handling Alfredo Cardigliano
2019-12-02 16:13   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 14/17] net/ionic: add RSS support Alfredo Cardigliano
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 15/17] net/ionic: add stats Alfredo Cardigliano
2019-12-02 16:14   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 16/17] net/ionic: add TX checksum support Alfredo Cardigliano
2019-12-02 16:15   ` Ferruh Yigit
2019-10-15  8:22 ` [dpdk-dev] [PATCH v2 17/17] net/ionic: read fw version Alfredo Cardigliano
2019-10-15 13:09 ` [dpdk-dev] [PATCH v2 00/17] Introduces net/ionic PMD Ferruh Yigit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=40e1e2aa-bbbc-9384-a33e-f249b3157104@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=anatoly.burakov@intel.com \
    --cc=cardigliano@ntop.org \
    --cc=dev@dpdk.org \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git