Re: [dpdk-dev] [PATCH v9 01/10] drivers/baseband: add PMD for ACC100

[Tom Rix <trix@redhat.com>]

On 9/29/20 4:17 PM, Chautru, Nicolas wrote:
> Hi Tom, 
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Tuesday, September 29, 2020 12:54 PM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> akhil.goyal@nxp.com
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Xu, Rosen
>> <rosen.xu@intel.com>; dave.burley@accelercomm.com;
>> aidan.goddard@accelercomm.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> Liu, Tianjiao <tianjiao.liu@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v9 01/10] drivers/baseband: add PMD for
>> ACC100
>>
>>
>> On 9/28/20 5:29 PM, Nicolas Chautru wrote:
>>> Add stubs for the ACC100 PMD
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> Acked-by: Liu Tianjiao <Tianjiao.liu@intel.com>
>>> ---
>>>  doc/guides/bbdevs/acc100.rst                       | 233 +++++++++++++++++++++
>>>  doc/guides/bbdevs/features/acc100.ini              |  14 ++
>>>  doc/guides/bbdevs/index.rst                        |   1 +
>>>  drivers/baseband/acc100/meson.build                |   6 +
>>>  drivers/baseband/acc100/rte_acc100_pmd.c           | 175
>> ++++++++++++++++
>>>  drivers/baseband/acc100/rte_acc100_pmd.h           |  37 ++++
>>>  .../acc100/rte_pmd_bbdev_acc100_version.map        |   3 +
>>>  drivers/baseband/meson.build                       |   2 +-
>>>  8 files changed, 470 insertions(+), 1 deletion(-)  create mode 100644
>>> doc/guides/bbdevs/acc100.rst  create mode 100644
>>> doc/guides/bbdevs/features/acc100.ini
>>>  create mode 100644 drivers/baseband/acc100/meson.build
>>>  create mode 100644 drivers/baseband/acc100/rte_acc100_pmd.c
>>>  create mode 100644 drivers/baseband/acc100/rte_acc100_pmd.h
>>>  create mode 100644
>>> drivers/baseband/acc100/rte_pmd_bbdev_acc100_version.map
>>>
>>> diff --git a/doc/guides/bbdevs/acc100.rst
>>> b/doc/guides/bbdevs/acc100.rst new file mode 100644 index
>>> 0000000..f87ee09
>>> --- /dev/null
>>> +++ b/doc/guides/bbdevs/acc100.rst
>>> @@ -0,0 +1,233 @@
>>> +..  SPDX-License-Identifier: BSD-3-Clause
>>> +    Copyright(c) 2020 Intel Corporation
>>> +
>>> +Intel(R) ACC100 5G/4G FEC Poll Mode Driver
>>> +==========================================
>>> +
>>> +The BBDEV ACC100 5G/4G FEC poll mode driver (PMD) supports an
>>> +implementation of a VRAN FEC wireless acceleration function.
>>> +This device is also known as Mount Bryce.
>> If this is code name or general chip name it should be removed.
> We have used general chip name for other PMDs (ie. Vista Creek), I can remove but
> why should this be removed for my benefit? This tends to be the most user friendly
> name so arguablygood to name drop in documentation . 

VistaCreek is the code name, the chip would be aria10.

Since mt bryce is the chip name, after more than 1 eASIC this becomes confusing.

Generally public product names should be used because only the early developers will know the development code names.

>
>
>>> +
>>> +Features
>>> +--------
>>> +
>>> +ACC100 5G/4G FEC PMD supports the following features:
>>> +
>>> +- LDPC Encode in the DL (5GNR)
>>> +- LDPC Decode in the UL (5GNR)
>>> +- Turbo Encode in the DL (4G)
>>> +- Turbo Decode in the UL (4G)
>>> +- 16 VFs per PF (physical device)
>>> +- Maximum of 128 queues per VF
>>> +- PCIe Gen-3 x16 Interface
>>> +- MSI
>>> +- SR-IOV
>>> +
>>> +ACC100 5G/4G FEC PMD supports the following BBDEV capabilities:
>>> +
>>> +* For the LDPC encode operation:
>>> +   - ``RTE_BBDEV_LDPC_CRC_24B_ATTACH`` :  set to attach CRC24B to
>> CB(s)
>>> +   - ``RTE_BBDEV_LDPC_RATE_MATCH`` :  if set then do not do Rate Match
>> bypass
>>> +   - ``RTE_BBDEV_LDPC_INTERLEAVER_BYPASS`` : if set then bypass
>>> +interleaver
>>> +
>>> +* For the LDPC decode operation:
>>> +   - ``RTE_BBDEV_LDPC_CRC_TYPE_24B_CHECK`` :  check CRC24B from
>> CB(s)
>>> +   - ``RTE_BBDEV_LDPC_ITERATION_STOP_ENABLE`` :  disable early
>> termination
>>> +   - ``RTE_BBDEV_LDPC_CRC_TYPE_24B_DROP`` :  drops CRC24B bits
>> appended while decoding
>>> +   - ``RTE_BBDEV_LDPC_HQ_COMBINE_IN_ENABLE`` :  provides an input
>> for HARQ combining
>>> +   - ``RTE_BBDEV_LDPC_HQ_COMBINE_OUT_ENABLE`` :  provides an input
>> for HARQ combining
>>> +   - ``RTE_BBDEV_LDPC_INTERNAL_HARQ_MEMORY_IN_ENABLE`` :  HARQ
>> memory input is internal
>>> +   - ``RTE_BBDEV_LDPC_INTERNAL_HARQ_MEMORY_OUT_ENABLE`` :
>> HARQ memory output is internal
>>> +   - ``RTE_BBDEV_LDPC_INTERNAL_HARQ_MEMORY_LOOPBACK`` :
>> loopback data to/from HARQ memory
>>> +   - ``RTE_BBDEV_LDPC_INTERNAL_HARQ_MEMORY_FILLERS`` :  HARQ
>> memory includes the fillers bits
>>> +   - ``RTE_BBDEV_LDPC_DEC_SCATTER_GATHER`` :  supports scatter-
>> gather for input/output data
>>> +   - ``RTE_BBDEV_LDPC_HARQ_6BIT_COMPRESSION`` :  supports
>> compression of the HARQ input/output
>>> +   - ``RTE_BBDEV_LDPC_LLR_COMPRESSION`` :  supports LLR input
>>> +compression
>>> +
>>> +* For the turbo encode operation:
>>> +   - ``RTE_BBDEV_TURBO_CRC_24B_ATTACH`` :  set to attach CRC24B to
>> CB(s)
>>> +   - ``RTE_BBDEV_TURBO_RATE_MATCH`` :  if set then do not do Rate
>> Match bypass
>>> +   - ``RTE_BBDEV_TURBO_ENC_INTERRUPTS`` :  set for encoder dequeue
>> interrupts
>>> +   - ``RTE_BBDEV_TURBO_RV_INDEX_BYPASS`` :  set to bypass RV index
>>> +   - ``RTE_BBDEV_TURBO_ENC_SCATTER_GATHER`` :  supports
>>> +scatter-gather for input/output data
>>> +
>>> +* For the turbo decode operation:
>>> +   - ``RTE_BBDEV_TURBO_CRC_TYPE_24B`` :  check CRC24B from CB(s)
>>> +   - ``RTE_BBDEV_TURBO_SUBBLOCK_DEINTERLEAVE`` :  perform subblock
>> de-interleave
>>> +   - ``RTE_BBDEV_TURBO_DEC_INTERRUPTS`` :  set for decoder dequeue
>> interrupts
>>> +   - ``RTE_BBDEV_TURBO_NEG_LLR_1_BIT_IN`` :  set if negative LLR
>> encoder i/p is supported
>>> +   - ``RTE_BBDEV_TURBO_POS_LLR_1_BIT_IN`` :  set if positive LLR encoder
>> i/p is supported
>>> +   - ``RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP`` :  keep CRC24B bits
>> appended while decoding
>>> +   - ``RTE_BBDEV_TURBO_EARLY_TERMINATION`` :  set early early
>> termination feature
>>> +   - ``RTE_BBDEV_TURBO_DEC_SCATTER_GATHER`` :  supports scatter-
>> gather for input/output data
>>> +   - ``RTE_BBDEV_TURBO_HALF_ITERATION_EVEN`` :  set half iteration
>>> +granularity
>>> +
>>> +Installation
>>> +------------
>>> +
>>> +Section 3 of the DPDK manual provides instuctions on installing and
>>> +compiling DPDK. The default set of bbdev compile flags may be found
>>> +in config/common_base, where for example the flag to build the ACC100
>>> +5G/4G FEC device, ``CONFIG_RTE_LIBRTE_PMD_BBDEV_ACC100``,
>>> +is already set.
>>> +
>>> +DPDK requires hugepages to be configured as detailed in section 2 of the
>> DPDK manual.
>>> +The bbdev test application has been tested with a configuration 40 x
>>> +1GB hugepages. The hugepage configuration of a server may be examined
>> using:
>>> +
>>> +.. code-block:: console
>>> +
>>> +   grep Huge* /proc/meminfo
>>> +
>>> +
>>> +Initialization
>>> +--------------
>>> +
>>> +When the device first powers up, its PCI Physical Functions (PF) can be
>> listed through this command:
>>> +
>>> +.. code-block:: console
>>> +
>>> +  sudo lspci -vd8086:0d5c
>>> +
>>> +The physical and virtual functions are compatible with Linux UIO drivers:
>>> +``vfio`` and ``igb_uio``. However, in order to work the ACC100 5G/4G
>>> +FEC device firstly needs to be bound to one of these linux drivers through
>> DPDK.
>> FEC device first
> ok
>
>>> +
>>> +
>>> +Bind PF UIO driver(s)
>>> +~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +Install the DPDK igb_uio driver, bind it with the PF PCI device ID
>>> +and use ``lspci`` to confirm the PF device is under use by ``igb_uio`` DPDK
>> UIO driver.
>>> +
>>> +The igb_uio driver may be bound to the PF PCI device using one of three
>> methods:
>>> +
>>> +
>>> +1. PCI functions (physical or virtual, depending on the use case) can
>>> +be bound to the UIO driver by repeating this command for every function.
>>> +
>>> +.. code-block:: console
>>> +
>>> +  cd <dpdk-top-level-directory>
>>> +  insmod ./build/kmod/igb_uio.ko
>>> +  echo "8086 0d5c" > /sys/bus/pci/drivers/igb_uio/new_id
>>> +  lspci -vd8086:0d5c
>>> +
>>> +
>>> +2. Another way to bind PF with DPDK UIO driver is by using the
>>> +``dpdk-devbind.py`` tool
>>> +
>>> +.. code-block:: console
>>> +
>>> +  cd <dpdk-top-level-directory>
>>> +  ./usertools/dpdk-devbind.py -b igb_uio 0000:06:00.0
>>> +
>>> +where the PCI device ID (example: 0000:06:00.0) is obtained using
>>> +lspci -vd8086:0d5c
>>> +
>>> +
>>> +3. A third way to bind is to use ``dpdk-setup.sh`` tool
>>> +
>>> +.. code-block:: console
>>> +
>>> +  cd <dpdk-top-level-directory>
>>> +  ./usertools/dpdk-setup.sh
>>> +
>>> +  select 'Bind Ethernet/Crypto/Baseband device to IGB UIO module'
>>> +  or
>>> +  select 'Bind Ethernet/Crypto/Baseband device to VFIO module'
>>> + depending on driver required
>> This is the igb_uio section, should defer vfio select to its section.
> Ok
>
>>> +  enter PCI device ID
>>> +  select 'Display current Ethernet/Crypto/Baseband device settings'
>>> + to confirm binding
>>> +
>>> +
>>> +In the same way the ACC100 5G/4G FEC PF can be bound with vfio, but
>>> +vfio driver does not support SR-IOV configuration right out of the box, so
>> it will need to be patched.
>> Other documentation says works with 5.7
> Yes this is a bit historical now. I can remove this bit which is not very informative and non specific to that PMD. 
>
>>> +
>>> +
>>> +Enable Virtual Functions
>>> +~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +Now, it should be visible in the printouts that PCI PF is under
>>> +igb_uio control "``Kernel driver in use: igb_uio``"
>>> +
>>> +To show the number of available VFs on the device, read ``sriov_totalvfs``
>> file..
>>> +
>>> +.. code-block:: console
>>> +
>>> +  cat /sys/bus/pci/devices/0000\:<b>\:<d>.<f>/sriov_totalvfs
>>> +
>>> +  where 0000\:<b>\:<d>.<f> is the PCI device ID
>>> +
>>> +
>>> +To enable VFs via igb_uio, echo the number of virtual functions
>>> +intended to enable to ``max_vfs`` file..
>>> +
>>> +.. code-block:: console
>>> +
>>> +  echo <num-of-vfs> > /sys/bus/pci/devices/0000\:<b>\:<d>.<f>/max_vfs
>>> +
>>> +
>>> +Afterwards, all VFs must be bound to appropriate UIO drivers as
>>> +required, same way it was done with the physical function previously.
>>> +
>>> +Enabling SR-IOV via vfio driver is pretty much the same, except that
>>> +the file name is different:
>>> +
>>> +.. code-block:: console
>>> +
>>> +  echo <num-of-vfs> >
>>> + /sys/bus/pci/devices/0000\:<b>\:<d>.<f>/sriov_numvfs
>>> +
>>> +
>>> +Configure the VFs through PF
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +The PCI virtual functions must be configured before working or
>>> +getting assigned to VMs/Containers. The configuration involves
>>> +allocating the number of hardware queues, priorities, load balance,
>>> +bandwidth and other settings necessary for the device to perform FEC
>> functions.
>>> +
>>> +This configuration needs to be executed at least once after reboot or
>>> +PCI FLR and can be achieved by using the function
>>> +``acc100_configure()``, which sets up the parameters defined in
>> ``acc100_conf`` structure.
>>> +
>>> +Test Application
>>> +----------------
>>> +
>>> +BBDEV provides a test application, ``test-bbdev.py`` and range of
>>> +test data for testing the functionality of ACC100 5G/4G FEC encode
>>> +and decode, depending on the device's capabilities. The test
>>> +application is located under app->test-bbdev folder and has the following
>> options:
>>> +
>>> +.. code-block:: console
>>> +
>>> +  "-p", "--testapp-path": specifies path to the bbdev test app.
>>> +  "-e", "--eal-params"	: EAL arguments which are passed to the test
>> app.
>>> +  "-t", "--timeout"	: Timeout in seconds (default=300).
>>> +  "-c", "--test-cases"	: Defines test cases to run. Run all if not specified.
>>> +  "-v", "--test-vector"	: Test vector path (default=dpdk_path+/app/test-
>> bbdev/test_vectors/bbdev_null.data).
>>> +  "-n", "--num-ops"	: Number of operations to process on device
>> (default=32).
>>> +  "-b", "--burst-size"	: Operations enqueue/dequeue burst size
>> (default=32).
>>> +  "-s", "--snr"		: SNR in dB used when generating LLRs for bler tests.
>>> +  "-s", "--iter_max"	: Number of iterations for LDPC decoder.
>>> +  "-l", "--num-lcores"	: Number of lcores to run (default=16).
>>> +  "-i", "--init-device" : Initialise PF device with default values.
>>> +
>>> +
>>> +To execute the test application tool using simple decode or encode
>>> +data, type one of the following:
>>> +
>>> +.. code-block:: console
>>> +
>>> +  ./test-bbdev.py -c validation -n 64 -b 1 -v ./ldpc_dec_default.data
>>> + ./test-bbdev.py -c validation -n 64 -b 1 -v ./ldpc_enc_default.data
>>> +
>>> +
>>> +The test application ``test-bbdev.py``, supports the ability to
>>> +configure the PF device with a default set of values, if the "-i" or
>>> +"- -init-device" option is included. The default values are defined in
>> test_bbdev_perf.c.
>>> +
>>> +
>>> +Test Vectors
>>> +~~~~~~~~~~~~
>>> +
>>> +In addition to the simple LDPC decoder and LDPC encoder tests, bbdev
>>> +also provides a range of additional tests under the test_vectors
>>> +folder, which may be useful. The results of these tests will depend
>>> +on the ACC100 5G/4G FEC capabilities which may cause some testcases to
>> be skipped, but no failure should be reported.
>>
>> Just
>>
>> to be skipped.
>>
>> should be able to assume skipped test do not get reported as failures.
> Not necessaraly that obvious from feedback. It doesn't hurt to be explicit and
> this statement is common to all PMDs. 
>
ok

>>> diff --git a/doc/guides/bbdevs/features/acc100.ini
>>> b/doc/guides/bbdevs/features/acc100.ini
>>> new file mode 100644
>>> index 0000000..c89a4d7
>>> --- /dev/null
>>> +++ b/doc/guides/bbdevs/features/acc100.ini
>>> @@ -0,0 +1,14 @@
>>> +;
>>> +; Supported features of the 'acc100' bbdev driver.
>>> +;
>>> +; Refer to default.ini for the full list of available PMD features.
>>> +;
>>> +[Features]
>>> +Turbo Decoder (4G)     = N
>>> +Turbo Encoder (4G)     = N
>>> +LDPC Decoder (5G)      = N
>>> +LDPC Encoder (5G)      = N
>>> +LLR/HARQ Compression   = N
>>> +External DDR Access    = N
>>> +HW Accelerated         = Y
>>> +BBDEV API              = Y
>>> diff --git a/doc/guides/bbdevs/index.rst b/doc/guides/bbdevs/index.rst
>>> index a8092dd..4445cbd 100644
>>> --- a/doc/guides/bbdevs/index.rst
>>> +++ b/doc/guides/bbdevs/index.rst
>>> @@ -13,3 +13,4 @@ Baseband Device Drivers
>>>      turbo_sw
>>>      fpga_lte_fec
>>>      fpga_5gnr_fec
>>> +    acc100
>>> diff --git a/drivers/baseband/acc100/meson.build
>>> b/drivers/baseband/acc100/meson.build
>>> new file mode 100644
>>> index 0000000..8afafc2
>>> --- /dev/null
>>> +++ b/drivers/baseband/acc100/meson.build
>>> @@ -0,0 +1,6 @@
>>> +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2020 Intel
>>> +Corporation
>>> +
>>> +deps += ['bbdev', 'bus_vdev', 'ring', 'pci', 'bus_pci']
>>> +
>>> +sources = files('rte_acc100_pmd.c')
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> new file mode 100644
>>> index 0000000..1b4cd13
>>> --- /dev/null
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> @@ -0,0 +1,175 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2020 Intel Corporation  */
>>> +
>>> +#include <unistd.h>
>>> +
>>> +#include <rte_common.h>
>>> +#include <rte_log.h>
>>> +#include <rte_dev.h>
>>> +#include <rte_malloc.h>
>>> +#include <rte_mempool.h>
>>> +#include <rte_byteorder.h>
>>> +#include <rte_errno.h>
>>> +#include <rte_branch_prediction.h>
>>> +#include <rte_hexdump.h>
>>> +#include <rte_pci.h>
>>> +#include <rte_bus_pci.h>
>>> +
>>> +#include <rte_bbdev.h>
>>> +#include <rte_bbdev_pmd.h>
>> Should these #includes' be in alpha order ?
> Interesting comment. Is this a coding guide line for DPDK or others?
> I have never heard of this personnally, what is the rational? 

Not sure if this is dpdk style, i know some other project do this.

This works for self consistent headers, no idea if dpdk are.

don't bother with this.

>>> +#include "rte_acc100_pmd.h"
>>> +
>>> +#ifdef RTE_LIBRTE_BBDEV_DEBUG
>>> +RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, DEBUG); #else
>>> +RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, NOTICE); #endif
>>> +
>>> +/* Free 64MB memory used for software rings */ static int
>>> +acc100_dev_close(struct rte_bbdev *dev  __rte_unused) {
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct rte_bbdev_ops acc100_bbdev_ops = {
>>> +	.close = acc100_dev_close,
>>> +};
>>> +
>>> +/* ACC100 PCI PF address map */
>>> +static struct rte_pci_id pci_id_acc100_pf_map[] = {
>>> +	{
>>> +		RTE_PCI_DEVICE(RTE_ACC100_VENDOR_ID,
>> RTE_ACC100_PF_DEVICE_ID)
>>> +	},
>>> +	{.device_id = 0},
>>> +};
>>> +
>>> +/* ACC100 PCI VF address map */
>>> +static struct rte_pci_id pci_id_acc100_vf_map[] = {
>>> +	{
>>> +		RTE_PCI_DEVICE(RTE_ACC100_VENDOR_ID,
>> RTE_ACC100_VF_DEVICE_ID)
>>> +	},
>>> +	{.device_id = 0},
>>> +};
>>> +
>>> +/* Initialization Function */
>>> +static void
>>> +acc100_bbdev_init(struct rte_bbdev *dev, struct rte_pci_driver *drv)
>>> +{
>>> +	struct rte_pci_device *pci_dev = RTE_DEV_TO_PCI(dev->device);
>>> +
>>> +	dev->dev_ops = &acc100_bbdev_ops;
>>> +
>>> +	((struct acc100_device *) dev->data->dev_private)->pf_device =
>>> +			!strcmp(drv->driver.name,
>>> +
>> 	RTE_STR(ACC100PF_DRIVER_NAME));
>>> +	((struct acc100_device *) dev->data->dev_private)->mmio_base =
>>> +			pci_dev->mem_resource[0].addr;
>>> +
>>> +	rte_bbdev_log_debug("Init device %s [%s] @ vaddr %p paddr
>> %#"PRIx64"",
>>> +			drv->driver.name, dev->data->name,
>>> +			(void *)pci_dev->mem_resource[0].addr,
>>> +			pci_dev->mem_resource[0].phys_addr);
>>> +}
>>> +
>>> +static int acc100_pci_probe(struct rte_pci_driver *pci_drv,
>>> +	struct rte_pci_device *pci_dev)
>>> +{
>>> +	struct rte_bbdev *bbdev = NULL;
>>> +	char dev_name[RTE_BBDEV_NAME_MAX_LEN];
>>> +
>>> +	if (pci_dev == NULL) {
>>> +		rte_bbdev_log(ERR, "NULL PCI device");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	rte_pci_device_name(&pci_dev->addr, dev_name,
>> sizeof(dev_name));
>>> +
>>> +	/* Allocate memory to be used privately by drivers */
>>> +	bbdev = rte_bbdev_allocate(pci_dev->device.name);
>>> +	if (bbdev == NULL)
>>> +		return -ENODEV;
>>> +
>>> +	/* allocate device private memory */
>>> +	bbdev->data->dev_private = rte_zmalloc_socket(dev_name,
>>> +			sizeof(struct acc100_device), RTE_CACHE_LINE_SIZE,
>>> +			pci_dev->device.numa_node);
>>> +
>>> +	if (bbdev->data->dev_private == NULL) {
>>> +		rte_bbdev_log(CRIT,
>>> +				"Allocate of %zu bytes for device \"%s\"
>> failed",
>>> +				sizeof(struct acc100_device), dev_name);
>>> +				rte_bbdev_release(bbdev);
>>> +			return -ENOMEM;
>>> +	}
>>> +
>>> +	/* Fill HW specific part of device structure */
>>> +	bbdev->device = &pci_dev->device;
>>> +	bbdev->intr_handle = &pci_dev->intr_handle;
>>> +	bbdev->data->socket_id = pci_dev->device.numa_node;
>>> +
>>> +	/* Invoke ACC100 device initialization function */
>>> +	acc100_bbdev_init(bbdev, pci_drv);
>>> +
>>> +	rte_bbdev_log_debug("Initialised bbdev %s (id = %u)",
>>> +			dev_name, bbdev->data->dev_id);
>>> +	return 0;
>>> +}
>>> +
>>> +static int acc100_pci_remove(struct rte_pci_device *pci_dev) {
>>> +	struct rte_bbdev *bbdev;
>>> +	int ret;
>>> +	uint8_t dev_id;
>>> +
>>> +	if (pci_dev == NULL)
>>> +		return -EINVAL;
>>> +
>>> +	/* Find device */
>>> +	bbdev = rte_bbdev_get_named_dev(pci_dev->device.name);
>>> +	if (bbdev == NULL) {
>>> +		rte_bbdev_log(CRIT,
>>> +				"Couldn't find HW dev \"%s\" to uninitialise
>> it",
>>> +				pci_dev->device.name);
>>> +		return -ENODEV;
>>> +	}
>>> +	dev_id = bbdev->data->dev_id;
>>> +
>>> +	/* free device private memory before close */
>>> +	rte_free(bbdev->data->dev_private);
>>> +
>>> +	/* Close device */
>>> +	ret = rte_bbdev_close(dev_id);
>> Do you want to reorder this close before the rte_free so you could recover
>> from the failure ?
> Given this is done the same way for other PMDs I would not change it as it would create a discrepency.
> It could be done in principle as another patch for multiple PMDs to support this, but really I don't see a usecase for try to fall back in case there was such a speculative aerror. 
>
fair enough

Tom

>> Tom
>>
> Thanks
> Nic
>
>
>>> +	if (ret < 0)
>>> +		rte_bbdev_log(ERR,
>>> +				"Device %i failed to close during uninit: %i",
>>> +				dev_id, ret);
>>> +
>>> +	/* release bbdev from library */
>>> +	rte_bbdev_release(bbdev);
>>> +
>>> +	rte_bbdev_log_debug("Destroyed bbdev = %u", dev_id);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct rte_pci_driver acc100_pci_pf_driver = {
>>> +		.probe = acc100_pci_probe,
>>> +		.remove = acc100_pci_remove,
>>> +		.id_table = pci_id_acc100_pf_map,
>>> +		.drv_flags = RTE_PCI_DRV_NEED_MAPPING };
>>> +
>>> +static struct rte_pci_driver acc100_pci_vf_driver = {
>>> +		.probe = acc100_pci_probe,
>>> +		.remove = acc100_pci_remove,
>>> +		.id_table = pci_id_acc100_vf_map,
>>> +		.drv_flags = RTE_PCI_DRV_NEED_MAPPING };
>>> +
>>> +RTE_PMD_REGISTER_PCI(ACC100PF_DRIVER_NAME,
>> acc100_pci_pf_driver);
>>> +RTE_PMD_REGISTER_PCI_TABLE(ACC100PF_DRIVER_NAME,
>>> +pci_id_acc100_pf_map);
>> RTE_PMD_REGISTER_PCI(ACC100VF_DRIVER_NAME,
>>> +acc100_pci_vf_driver);
>>> +RTE_PMD_REGISTER_PCI_TABLE(ACC100VF_DRIVER_NAME,
>>> +pci_id_acc100_vf_map);
>>> +
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h
>>> b/drivers/baseband/acc100/rte_acc100_pmd.h
>>> new file mode 100644
>>> index 0000000..6f46df0
>>> --- /dev/null
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.h
>>> @@ -0,0 +1,37 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2020 Intel Corporation  */
>>> +
>>> +#ifndef _RTE_ACC100_PMD_H_
>>> +#define _RTE_ACC100_PMD_H_
>>> +
>>> +/* Helper macro for logging */
>>> +#define rte_bbdev_log(level, fmt, ...) \
>>> +	rte_log(RTE_LOG_ ## level, acc100_logtype, fmt "\n", \
>>> +		##__VA_ARGS__)
>>> +
>>> +#ifdef RTE_LIBRTE_BBDEV_DEBUG
>>> +#define rte_bbdev_log_debug(fmt, ...) \
>>> +		rte_bbdev_log(DEBUG, "acc100_pmd: " fmt, \
>>> +		##__VA_ARGS__)
>>> +#else
>>> +#define rte_bbdev_log_debug(fmt, ...) #endif
>>> +
>>> +/* ACC100 PF and VF driver names */
>>> +#define ACC100PF_DRIVER_NAME           intel_acc100_pf
>>> +#define ACC100VF_DRIVER_NAME           intel_acc100_vf
>>> +
>>> +/* ACC100 PCI vendor & device IDs */
>>> +#define RTE_ACC100_VENDOR_ID           (0x8086)
>>> +#define RTE_ACC100_PF_DEVICE_ID        (0x0d5c)
>>> +#define RTE_ACC100_VF_DEVICE_ID        (0x0d5d)
>>> +
>>> +/* Private data structure for each ACC100 device */ struct
>>> +acc100_device {
>>> +	void *mmio_base;  /**< Base address of MMIO registers (BAR0) */
>>> +	bool pf_device; /**< True if this is a PF ACC100 device */
>>> +	bool configured; /**< True if this ACC100 device is configured */ };
>>> +
>>> +#endif /* _RTE_ACC100_PMD_H_ */
>>> diff --git a/drivers/baseband/acc100/rte_pmd_bbdev_acc100_version.map
>>> b/drivers/baseband/acc100/rte_pmd_bbdev_acc100_version.map
>>> new file mode 100644
>>> index 0000000..4a76d1d
>>> --- /dev/null
>>> +++ b/drivers/baseband/acc100/rte_pmd_bbdev_acc100_version.map
>>> @@ -0,0 +1,3 @@
>>> +DPDK_21 {
>>> +	local: *;
>>> +};
>>> diff --git a/drivers/baseband/meson.build
>>> b/drivers/baseband/meson.build index 415b672..72301ce 100644
>>> --- a/drivers/baseband/meson.build
>>> +++ b/drivers/baseband/meson.build
>>> @@ -5,7 +5,7 @@ if is_windows
>>>  	subdir_done()
>>>  endif
>>>
>>> -drivers = ['null', 'turbo_sw', 'fpga_lte_fec', 'fpga_5gnr_fec']
>>> +drivers = ['null', 'turbo_sw', 'fpga_lte_fec', 'fpga_5gnr_fec',
>>> +'acc100']
>>>
>>>  config_flag_fmt = 'RTE_LIBRTE_PMD_BBDEV_@0@'
>>>  driver_name_fmt = 'rte_pmd_bbdev_@0@'