[dpdk-dev] [PATCH v4 1/7] net/ark: PMD for Atomic Rules Arkville driver stub

Ferruh Yigit ferruh.yigit at intel.com
Thu Mar 23 12:36:18 CET 2017


On 3/23/2017 1:03 AM, Ed Czeck wrote:
> Enable Arkville on supported configurations
> Add overview documentation
> Minimum driver support for valid compile
> Arkville PMD is not supported on ARM or PowerPC at this time

Can you please send new version of patchset as reply to previous
version, using --in-reply-to ?
This helps to see all patches in same mail tread and helps seeing
previous comments, also in mail archive all versions stays in same place.

> 
> v4:
> * Address issues report from review
> * Add internal comments on driver arg
> * provide a bare-biones dev init to avoid compiler warnings
> 
> v3:
> * Split large patch into several smaller ones
> 
> Signed-off-by: Ed Czeck <ed.czeck at atomicrules.com>
> ---

<...>

> diff --git a/config/defconfig_ppc_64-power8-linuxapp-gcc b/config/defconfig_ppc_64-power8-linuxapp-gcc
> index 35f7fb6..89bc396 100644
> --- a/config/defconfig_ppc_64-power8-linuxapp-gcc
> +++ b/config/defconfig_ppc_64-power8-linuxapp-gcc
> @@ -48,6 +48,7 @@ CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=n
>  
>  # Note: Initially, all of the PMD drivers compilation are turned off on Power
>  # Will turn on them only after the successful testing on Power
> +CONFIG_RTE_LIBRTE_ARK_PMD=n

Is it not tested or known that it is not supported?

>  CONFIG_RTE_LIBRTE_IXGBE_PMD=n
>  CONFIG_RTE_LIBRTE_I40E_PMD=n
>  CONFIG_RTE_LIBRTE_VIRTIO_PMD=y

<...>

> +Configuration Information
> +-------------------------
> +
> +**DPDK Configuration Parameters**
> +
> +  The following configuration options are available for the ARK PMD:
> +
> +   * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
> +     of the ARK PMD driver in the DPDK compilation.
> +
> +   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
> +     packets are padded to 60 bytes to support downstream MACS.
> +
> +   * **CONFIG_RTE_LIBRTE_ARK_DEBUG_RX** (default n): Enables or disables debug
> +     logging and internal checking of RX ingress logic within the ARK PMD driver.
> +
> +   * **CONFIG_RTE_LIBRTE_ARK_DEBUG_TX** (default n): Enables or disables debug
> +     logging and internal checking of TX egress logic within the ARK PMD driver.
> +
> +   * **CONFIG_RTE_LIBRTE_ARK_DEBUG_STATS** (default n): Enables or disables debug
> +     logging of detailed packet and performance statistics gathered in
> +     the PMD and FPGA.
> +
> +   * **CONFIG_RTE_LIBRTE_ARK_DEBUG_TRACE** (default n): Enables or disables debug
> +     logging of detailed PMD events and status.
> +
> +

Can you also please document the device arguments in this file?

Device Parameters
-----------------

"Pkt_gen"
"Pkt_chkr"
"Pkt_dir"

Also perhaps you can include these device arguments in below "Usage
Example" section of this document.

It can be good to document expected format/dataset for these arguments.

> +Building DPDK
> +-------------
> +
> +See the :ref:`DPDK Getting Started Guide for Linux <linux_gsg>` for
> +instructions on how to build DPDK.
> +
> +By default the ARK PMD library will be built into the DPDK library.
> +
> +For configuring and using UIO and VFIO frameworks, please also refer :ref:`the
> +documentation that comes with DPDK suite <linux_gsg>`.
> +

<...>

> diff --git a/doc/guides/nics/features/ark.ini b/doc/guides/nics/features/ark.ini
> new file mode 100644
> index 0000000..dc8a0e2
> --- /dev/null
> +++ b/doc/guides/nics/features/ark.ini
> @@ -0,0 +1,15 @@
> +;
> +; Supported features of the 'ark' poll mode driver.
> +;
> +; Refer to default.ini for the full list of available PMD features.
> +;
> +[Features]
> +Queue start/stop     = Y
> +Jumbo frame          = Y
> +Scattered Rx         = Y
> +Basic stats          = Y
> +Stats per queue      = Y
> +FW version           = Y

Features can be added with the patch that adds functionality. I believe
above features not supported with current patch.

> +Linux UIO            = Y
> +x86-64               = Y
> +Usage doc            = Y

<...>

> +#
> +# all source are stored in SRCS-y
> +#
> +SRCS-y += ark_ethdev.c

Please use SRCS-y only in comment, for actual usage please prefer:
SRCS-$(CONFIG_RTE_LIBRTE_ARK_PMD)

> +
> +
> +# this lib depends upon:
> +DEPDIRS-y += lib/librte_mbuf

DEPDIRS-$(CONFIG_RTE_LIBRTE_ARK_PMD)

> +DEPDIRS-y += lib/librte_ether

<...>

> +#define ARK_TRACE_ON(fmt, ...) \
> +	PMD_DRV_LOG(ERR, fmt, ##__VA_ARGS__)
> +
> +#define ARK_TRACE_OFF(fmt, ...) \
> +	do {if (0) PMD_DRV_LOG(ERR, fmt, ##__VA_ARGS__); } while (0)

why not just "do { } while(0)" ?

> +
> +/* Debug macro for reporting Packet stats */
> +#ifdef RTE_LIBRTE_ARK_DEBUG_STATS
> +#define ARK_DEBUG_STATS(fmt, ...) ARK_TRACE_ON(fmt, ##__VA_ARGS__)

This is debug option but prints only "ERR" level log, shouldn't this be
DEBUG.
Also there are dpdk wide log level option, helps optimizing out some
code, if you use only ERR type, you won't be benefiting from it.

> +#else
> +#define ARK_DEBUG_STATS(fmt, ...)  ARK_TRACE_OFF(fmt, ##__VA_ARGS__)
> +#endif
> +
> +/* Debug macro for tracing full behavior*/
> +#ifdef RTE_LIBRTE_ARK_DEBUG_TRACE
> +#define ARK_DEBUG_TRACE(fmt, ...)  ARK_TRACE_ON(fmt, ##__VA_ARGS__)
> +#else
> +#define ARK_DEBUG_TRACE(fmt, ...)  ARK_TRACE_OFF(fmt, ##__VA_ARGS__)
> +#endif
> +
> +/* tracing including the function name */
> +#define PMD_DRV_LOG(level, fmt, args...) \
> +	RTE_LOG(level, PMD, "%s(): " fmt, __func__, ## args)

<...>

> +static int
> +eth_ark_dev_init(struct rte_eth_dev *dev __rte_unused)

__rte_unused not required.

> +{
> +	struct ark_adapter *ark =
> +		(struct ark_adapter *)dev->data->dev_private;
> +	struct rte_pci_device *pci_dev;
> +	int ret = -1;
> +
> +	ark->eth_dev = dev;
> +
> +	ARK_DEBUG_TRACE("eth_ark_dev_init(struct rte_eth_dev *dev)\n");
> +	gark[0] = ark;

Is it OK to have this hardcoded index "0"? When there are two instance
of this device, this value be overwritten by second one.

> +
> +	pci_dev = ARK_DEV_TO_PCI(dev);
> +	rte_eth_copy_pci_info(dev, pci_dev);

dev->data->dev_flags |= RTE_ETH_DEV_DETACHABLE;

> +
> +	if (pci_dev->device.devargs)
> +		eth_ark_check_args(pci_dev->device.devargs->args);
> +	else
> +		PMD_DRV_LOG(INFO, "No Device args found\n");
> +
> +
> +	ark->bar0 = (uint8_t *)pci_dev->mem_resource[0].addr;
> +	ark->a_bar = (uint8_t *)pci_dev->mem_resource[2].addr;
> +
> +	dev->dev_ops = &ark_eth_dev_ops;
> +
> +	return ret;
> +}

<...>

> +static void
> +eth_ark_dev_info_get(struct rte_eth_dev *dev,
> +		     struct rte_eth_dev_info *dev_info)
> +{
> +	/* device specific configuration */
> +	memset(dev_info, 0, sizeof(*dev_info));

memset not required, since already done by ethdev abstraction layer,
specially desc_lim values already overwritten below.

> +
> +	dev_info->max_rx_pktlen = (16 * 1024) - 128;
> +	dev_info->min_rx_bufsize = 1024;

Using some macros instead of hardcoded values helps to understand values.

<...>

> +static inline int
> +process_pktdir_arg(const char *key, const char *value,
> +		   void *extra_args __rte_unused)
> +{
> +	ARK_DEBUG_TRACE("In process_pktdir_arg, key = %s, value = %s\n",
> +			key, value);

The general usage of DEBUG_TRACE is providing backtrace log, function
enterance / exit informations. I guess, that is why it has been
controlled by different config option.

Here what you need looks like regular debugging functions, PMD_DRV_LOG /
RTE_LOG variant.

> +	pkt_dir_v = strtol(value, NULL, 16);
> +	ARK_DEBUG_TRACE("pkt_dir_v = 0x%x\n", pkt_dir_v);
> +	return 0;
> +}

<...>

> +
> +	while (fgets(line, sizeof(line), file)) {
> +		if (first) {
> +			strncpy(args, line, ARK_MAX_ARG_LEN);
> +			first = 0;
> +		} else {
> +			strncat(args, line, ARK_MAX_ARG_LEN);

Can this overflow args variable? Any way to prevent possible crash?

What happens if somebody, by accident, provides a file as device
argument which is larger than 256 bytes?

> +		}
> +	}
> +	ARK_DEBUG_TRACE("file = %s\n", args);
> +	fclose(file);
> +	return 0;
> +}
> +

<...>

> +/*
> + * Although Arkville is a physical device we take advantage of the virtual
> + * device initialization as a per test runtime initialization for
> + * regression testing.  Parameters are passed into the virtual device to
> + * configure the packet generator, packet director and packet checker.
> + */

Why not providing these arguments via physical device?

Can you please give more detail about your use case? This still looks
wrong to me.

> +static struct rte_vdev_driver pmd_ark_drv = {
> +	.probe = pmd_ark_probe,
> +	.remove = pmd_ark_remove,
> +};
> +

<...>

> diff --git a/drivers/net/ark/rte_pmd_ark_version.map b/drivers/net/ark/rte_pmd_ark_version.map
> new file mode 100644
> index 0000000..7f84780
> --- /dev/null
> +++ b/drivers/net/ark/rte_pmd_ark_version.map
> @@ -0,0 +1,4 @@
> +DPDK_2.0 {

s/DPDK_2.0/DPDK_17.05

> +	 local: *;
> +
> +};

<...>




More information about the dev mailing list