On 8/28/2019 12:08 PM, Gagandeep Singh wrote:
> ppfe (programmable packet forwarding engine)
> is a network poll mode driver for NXP SoC
> ls1012a.
>
> This patch introduces the framework of ppfe
> driver with basic functions of initialisation
> and teardown.
>
> Signed-off-by: Gagandeep Singh <g.si...@nxp.com>
> Signed-off-by: Akhil Goyal <akhil.go...@nxp.com>
> Acked-by: Nipun Gupta <nipun.gu...@nxp.com>
<...>
> @@ -0,0 +1,8 @@
> +;
> +; Supported features of the 'ppfe' network poll mode driver.
> +;
> +; Refer to default.ini for the full list of available PMD features.
> +;
> +[Features]
> +Linux VFIO = Y
> +ARMv8 = Y
Only arm? Should this be reflected to config file? like enabling it only in
'common_armv8a_linux'.
<...>
> +struct pfe_eth {
> + struct pfe_eth_priv_s *eth_priv[3];
Why hardcoded 3? Can it use PFE_CDEV_ETH_COUNT (if they are for same thing) ?
<...>
> +
> +/* for link status and IOCTL support using pfe character device
> + * XXX: Should be kept in sync with Kernel module
How can we know if it is in sync with kernel module, is there any check in the
code?
> + */
> +
> +/* Extracted from ls1012a_pfe_platform_data, there are 3 interfaces which are
> + * supported by PFE driver. Should be updated if number of eth devices are
> + * changed.
> + */
> +#define PFE_CDEV_ETH_COUNT 3
> +
> +#define PFE_CDEV_PATH "/dev/pfe_us_cdev"
Can you please add a comment why this char device is used?
<...>
> +struct pfe *g_pfe;
Any reason to not make this static?
> +unsigned int pfe_svr = SVR_LS1012A_REV1;
There are some checks for this in hal layer, should this be a runtime option
instead of hardcoded in the .c file?
<...>
> +/* pfe_eth_exit
> + */
> +static void
> +pfe_eth_exit(struct rte_eth_dev *dev, struct pfe *pfe)
> +{
> + /* Close the device file for link status */
> + pfe_eth_close_cdev(dev->data->dev_private);
You may want to do stop before close.
> +
> + rte_eth_dev_release_port(dev);
> + pfe->nb_devs--;
> +}
> +
> +/* pfe_eth_init_one
Comment is wrong, and not sure commenting the function name is that useful
> + */
> +static int pfe_eth_init(struct rte_vdev_device *vdev, struct pfe *pfe, int
> id)
Please apply same syntax to function prototypes.
> +{
> + struct rte_eth_dev_data *data = NULL;
> + struct rte_eth_dev *eth_dev = NULL;
> + struct pfe_eth_priv_s *priv = NULL;
> + int err;
> +
> + if (id >= pfe->max_intf)
> + return -EINVAL;
Can this happen at this stage?
> +
> + data = rte_zmalloc(NULL, sizeof(*data), 64);
> + if (data == NULL)
> + return -ENOMEM;
> +
> + eth_dev = rte_eth_vdev_allocate(vdev, sizeof(*priv));
> + if (eth_dev == NULL) {
> + rte_free(data);
> + return -ENOMEM;
> + }
> +
> + priv = eth_dev->data->dev_private;
> + rte_memcpy(data, eth_dev->data, sizeof(*data));
Why you allocate and copy to 'data', as far as I can see you are not using it at
all J Unless I am missing something.
<...>
> +
> + /* For link status, open the PFE CDEV; Error from this function
> + * is silently ignored; In case of error, the link status will not
> + * be available.
> + */
> + pfe_eth_open_cdev(priv);
> + rte_eth_dev_probing_finish(eth_dev);
> +
> + return 0;
> +err0:
> + rte_free(data);
Need to release the 'eth_dev', rte_eth_dev_release_port()
> + return err;
> +}
> +
> +/* Parse integer from integer argument */
> +static int
> +parse_integer_arg(const char *key __rte_unused,
> + const char *value, void *extra_args)
> +{
> + int *i = (int *)extra_args;
> +
> + *i = atoi(value);
> + if (*i < 0)
If you are trying to detect the error, atoi is not working that way, 'strtol'
can be better option for that.
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> +pfe_parse_vdev_init_params(struct pfe_vdev_init_params *params,
> + struct rte_vdev_device *dev)
> +{
> + struct rte_kvargs *kvlist = NULL;
> + int ret = 0;
> +
> + static const char * const pfe_vdev_valid_params[] = {
> + PPFE_VDEV_GEM_ID_ARG,
> + NULL
> + };
> +
> + const char *input_args = rte_vdev_device_args(dev);
> + if (params == NULL)
> + return -EINVAL;
This is a static function and only used once, not sure about verifying the input
param, although it won't hurt.
> +
> +
> + if (input_args) {
Not sure if 'input_args' can bu NULL at all, but anyway, what about retun
immediately if it is NULL, it can reduce the indentation.
Also if the PPFE_VDEV_GEM_ID_ARG is optional and you want to check if user
provided it or not, you should check the value of the 'input_args' instead of
its address I think.
> + kvlist = rte_kvargs_parse(input_args, pfe_vdev_valid_params);
> + if (kvlist == NULL)
> + return -1;
> +
> + ret = rte_kvargs_process(kvlist,
> + PPFE_VDEV_GEM_ID_ARG,
> + &parse_integer_arg,
> + ¶ms->gem_id);
> + if (ret < 0)
> + goto free_kvlist;
> + }
> +
> +free_kvlist:
> + rte_kvargs_free(kvlist);
> + return ret;
> +}
> +
> +static int
> +pmd_pfe_probe(struct rte_vdev_device *vdev)
> +{
> + const u32 *prop;
> + const struct device_node *np;
> + const char *name;
> + const uint32_t *addr;
> + uint64_t cbus_addr, ddr_size, cbus_size;
> + int rc = -1, fd = -1, gem_id;
> + unsigned int interface_count = 0;
> + size_t size = 0;
> + struct pfe_vdev_init_params init_params = {
> + -1
I would prefer designated initializes to make it more clear, up to you.
> + };
> +
> + name = rte_vdev_device_name(vdev);
> + rc = pfe_parse_vdev_init_params(&init_params, vdev);
> + if (rc < 0)
> + return -EINVAL;
> +
> + if (g_pfe) {
> + if (g_pfe->nb_devs >= g_pfe->max_intf)
> + return -EINVAL;
> +
> + goto eth_init;
> + }
> +
> + g_pfe = rte_zmalloc(NULL, sizeof(*g_pfe), 64);
If 64 is for cache line, may be better to use macro for it.
> + if (g_pfe == NULL)
> + return -EINVAL;
Is there a value of dynamically allocate the g_pfe, it is already global, so why
not define it statically? May be secondary process can have benefit from
allocating in shared memory but you are not using shared process as far as I can
see.
> +
> + /* Load the device-tree driver */
> + rc = of_init();
> + if (rc)
> + goto err;
> +
> + np = of_find_compatible_node(NULL, NULL, "fsl,pfe");
> + if (!np) {
> + rc = -EINVAL;
> + goto err;
> + }
> +
> + addr = of_get_address(np, 0, &cbus_size, NULL);
> + if (!addr)
> + goto err;
> +
> + cbus_addr = of_translate_address(np, addr);
> + if (!cbus_addr)
> + goto err;
> +
> +
> + addr = of_get_address(np, 1, &ddr_size, NULL);
> + if (!addr)
> + goto err;
> +
> + g_pfe->ddr_phys_baseaddr = of_translate_address(np, addr);
> + if (!g_pfe->ddr_phys_baseaddr)
> + goto err;
> +
> + g_pfe->ddr_size = ddr_size;
> +
> + fd = open("/dev/mem", O_RDWR);
> + g_pfe->cbus_baseaddr = mmap(NULL, cbus_size, PROT_READ | PROT_WRITE,
> + MAP_SHARED, fd, cbus_addr);
Above reads device information from a device tree file and creates a new mapping
for its address space. I wonder if device can be probbed as we do the pcie
devices, do you have to depend on the 'of', can't the driver registered for a
pysical device?
<...>
> + rc = pfe_eth_init(vdev, g_pfe, gem_id);
> + if (rc < 0)
> + goto err_eth;
> + else
> + g_pfe->nb_devs++;
> +
> + return 0;
> +
> +err_eth:
> +err_prop:
Should 'munmap' here? and close fd?
<...>
> +RTE_PMD_REGISTER_VDEV(PFE_PMD, pmd_pfe_drv);
> +RTE_PMD_REGISTER_ALIAS(PFE_PMD, eth_pfe);
Please drop the alias, that is for compatibility for old pmds.
> +RTE_PMD_REGISTER_PARAM_STRING(PFE_PMD, "intf=<int> ");
Better to use 'PPFE_VDEV_GEM_ID_ARG' macro here to prevent typos etc...
