Re: [dpdk-dev] [PATCH v2 02/13] net/ppfe: introduce ppfe net poll mode driver

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.singh@nxp.com>
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> Acked-by: Nipun Gupta <nipun.gupta@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,
> +					&params->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...