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

[Ed Czeck <ed.czeck@atomicrules.com>]

On Tue, Mar 28, 2017 at 10:34 AM, Ferruh Yigit <ferruh.yigit@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.yigit@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@atomicrules.com>
> > Signed-off-by: John Miller <john.miller@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.