On Tue, Mar 28, 2017 at 10:34 AM, Ferruh Yigit <ferruh.yi...@intel.com>
wrote:
>
> 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.yi...@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.cz...@atomicrules.com>
> > Signed-off-by: John Miller <john.mil...@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?
These are clarified in the doc, mostly by removing reference to them.
>
>
> > +
> > +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?
The packet generator is part of the arkville hardware. The configuration
file is needed to control it behaviors. Its really not part of the PMD,
just a control hook in the PMD to have access into the FPGA hardware.
>
> > +
>
> > +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?
Internally, we have had this discussion. The features/control are very
useful for diagnostic and isolating Arkville hardware from customer logic.
Our solution is to have a small set of features as device arguments. For
the nominal case these will not be used. However, if there is a special
case, we would like to have the option available to access them without
dropping in a different PMD.
>
>
> > +
> > +/* 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 declarations, its your call.
I'm leaving the order as is. I like to think that there might be a
balance between static prototype declarations and reading the code.
>
>
> > +
> > +#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?
Moved to .h file
>
>
> <...>
>
> > +
> > + 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.
Duplicate code removed.
>
>
> Should check the return value of the function, is it OK to continue if
> invalid devargs provided by user?
Return values are now checked.
>
>
> > +
> > + 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() ?
See comments in patch 7 for a explanation.
>
>
> > +
> > + 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?
Error are properly handled in this function and it caller.
>
>
> > +RTE_PMD_REGISTER_PCI(eth_ark, rte_ark_pmd.pci_drv);
>
> Net device names updated, eth_xxx -> net_xxx. This should be net_ark.
>
Updated to net_ark
Thanks,
Ed.
