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

[Ferruh Yigit]

On 3/23/2017 10:59 PM, 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
> 
> v5:
> * Address comments from Ferruh Yigit <ferruh.yigit at intel.com>
> * Added documentation on driver args
> * Makefile fixes
> * Safe argument processing
> * vdev args to dev args
> 
> v4:
> * Address issues report from review
> * Add internal comments on driver arg
> * provide a bare-bones dev init to avoid compiler warnings
> 
> v3:
> * Split large patch into several smaller ones
> 
> Signed-off-by: Ed Czeck <ed.czeck at atomicrules.com>
> Signed-off-by: John Miller <john.miller at atomicrules.com>

<...>

> +Device Parameters
> +-------------------
> +
> +The ARK PMD supports a series of parameters that are used for packet routing
> +and for internal packet generation and packet checking.  This section describes
> +the supported parameters.  These features are primarily used for
> +diagnostics, testing, and performance verification.  The nominal use
> +of Arkville does not require any configuration using these parameters.
> +
> +"Pkt_dir"
> +
> +The Packet Director controls connectivity between the Packet Generator,
> +Packet Checker, UDM, DDM, and external ingress and egress interfaces for
> +diagnostic purposes. It includes an internal loopback path from the DDM to the UDM.

What are UDM and DDM are?

> +
> +NOTE: Packets from the packet generator to the UDM are all directed to UDM RX
> +queue 0. Packets looped back from the DDM to the UDM are directed to the same
> +queue number they originated from.

So a packet generator is part of this PMD.

Isn't it overkill for PMD to have packet generator feauture. Is it
really needs to be part of PMD? Can be an option to use external packet
generators?

> +
> +bit 24: enRxChk (default 0)
> +bit 20: enDDMChk (default 1)
> +bit 16: enPGIngress (default 1)
> +bit 12: enPGEgress (default 0)
> +bit 8:  enExtIngress (default 1)
> +bit 4:  enDDMEgress (default 1)
> +bit 0:  enIntLpbk (default 0)
> +
> +Power On state
> +0x00110110
> +
> +These bits control which diagnostic paths are enabled. Refer to the PktDirector block
> +diagram in the Arkville documentation.
> +
> +Format:
> +Pkt_dir=0x00110110
> +
> +"Pkt_gen"
> +
> +The packet generator parameter takes a file as its argument.  The file contains configuration
> +parameters used internally for regression testing and are not intended to be published at this
> +level.

If these config options are not planned to use by public, should the
stripped from public version of the PMD? Is there a benefit to keep them
in dpdk repo?

> +
> +Format:
> +Pkt_gen=./config/pg.conf
> +
> +"Pkt_chkr"
> +
> +The packet checker parameter takes a file as its argument.  The file contains configuration
> +parameters used internally for regression testing and are not intended to be published at this
> +level.
> +
> +Format:
> +Pkt_chkr=./config/pc.conf
> +
> +

<...>

> +
> +/*  Internal prototypes */
> +static int eth_ark_check_args(struct ark_adapter *ark, const char *params);
> +static int eth_ark_dev_init(struct rte_eth_dev *dev);
> +static int eth_ark_dev_uninit(struct rte_eth_dev *eth_dev);
> +static int eth_ark_dev_configure(struct rte_eth_dev *dev);
> +static void eth_ark_dev_info_get(struct rte_eth_dev *dev,
> +				 struct rte_eth_dev_info *dev_info);

This may be personal, but I am for reordering functions and removing
static function declerations, its your call.

> +
> +#define ARK_DEV_TO_PCI(eth_dev)			\
> +	RTE_DEV_TO_PCI((eth_dev)->device)

This macro can go to header file, ark_ethdev.h perhaps?

<...>

> +
> +	if (pci_dev->device.devargs)
> +		eth_ark_check_args(ark, 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;
> +
> +	/*  We process our args last as they require everything to be setup */
> +	if (pci_dev->device.devargs)
> +		eth_ark_check_args(ark, pci_dev->device.devargs->args);
> +	else
> +		PMD_DRV_LOG(INFO, "No Device args found\n");

This is duplicate, please check ~10 lines above.

Should check the return value of the function, is it OK to continue if
invalid devargs provided by user?

> +
> +	return ret;
> +}
> +
> +static int
> +eth_ark_dev_uninit(struct rte_eth_dev *dev)
> +{
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;

Should primary process check also required in eth_ark_dev_init() ?

> +
> +	dev->dev_ops = NULL;
> +	dev->rx_pkt_burst = NULL;
> +	dev->tx_pkt_burst = NULL;
> +	return 0;
> +}

<...>

> +
> +	if (rte_kvargs_process(kvlist,
> +			       ARK_PKTDIR_ARG,
> +			       &process_pktdir_arg,
> +			       ark) != 0) {
> +		PMD_DRV_LOG(ERR, "Unable to parse arg %s\n", ARK_PKTDIR_ARG);

No error returned for this case?

> +RTE_PMD_REGISTER_PCI(eth_ark, rte_ark_pmd.pci_drv);

Net device names updated, eth_xxx -> net_xxx. This should be net_ark.