DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Gagandeep Singh <g.singh@nxp.com>, dev@dpdk.org
Cc: thomas@monjalon.net, Akhil Goyal <akhil.goyal@nxp.com>
Subject: Re: [dpdk-dev] [PATCH v2 02/13] net/ppfe: introduce ppfe net poll mode driver
Date: Thu, 26 Sep 2019 17:53:37 +0100	[thread overview]
Message-ID: <55a79342-cab2-042b-a7fe-ae45b03150e9@intel.com> (raw)
In-Reply-To: <20190828110849.14759-3-g.singh@nxp.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...

  reply	other threads:[~2019-09-26 16:53 UTC|newest]

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-26 13:02 [dpdk-dev] [PATCH v1 00/13] introduces ppfe network PMD Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 01/13] common/dpaax: moving OF lib code from dpaa bus Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 02/13] net/ppfe: introduce ppfe net poll mode driver Gagandeep Singh
2019-10-28 17:18   ` Stephen Hemminger
2019-10-29  9:27     ` Ferruh Yigit
2019-11-04 11:06       ` Bruce Richardson
2019-11-05 16:02         ` Ferruh Yigit
2019-11-06  9:38           ` Bruce Richardson
2019-11-06 12:22             ` Ferruh Yigit
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 03/13] doc: add guide for ppfe net PMD Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 04/13] net/ppfe: support dynamic logging Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 05/13] net/ppfe: add HW specific macros and operations Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 06/13] net/ppfe: add MAC and host interface initialisation Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 07/13] net/ppfe: add device start stop operations Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 08/13] net/ppfe: add queue setup and release operations Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 09/13] net/ppfe: add burst enqueue and dequeue operations Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 10/13] net/ppfe: add supported packet types and basic statistics Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 11/13] net/ppfe: add MTU and MAC address set operations Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 12/13] net/ppfe: add allmulticast and promiscuous Gagandeep Singh
2019-08-26 13:02 ` [dpdk-dev] [PATCH v1 13/13] net/ppfe: add link status update Gagandeep Singh
2019-08-27  7:16 ` [dpdk-dev] [PATCH v1 00/13] introduces ppfe network PMD Gagandeep Singh
2019-08-28 11:08 ` [dpdk-dev] [PATCH v2 " Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 01/13] common/dpaax: moving OF lib code from dpaa bus Gagandeep Singh
2019-09-26 16:54     ` Ferruh Yigit
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 02/13] net/ppfe: introduce ppfe net poll mode driver Gagandeep Singh
2019-09-26 16:53     ` Ferruh Yigit [this message]
2019-10-01  7:05       ` Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 03/13] doc: add guide for ppfe net PMD Gagandeep Singh
2019-09-26 16:56     ` Ferruh Yigit
2019-09-26 18:00     ` Ferruh Yigit
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 04/13] net/ppfe: support dynamic logging Gagandeep Singh
2019-09-26 16:57     ` Ferruh Yigit
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 05/13] net/ppfe: add HW specific macros and operations Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 06/13] net/ppfe: add MAC and host interface initialisation Gagandeep Singh
2019-09-26 17:00     ` Ferruh Yigit
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 07/13] net/ppfe: add device start stop operations Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 08/13] net/ppfe: add queue setup and release operations Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 09/13] net/ppfe: add burst enqueue and dequeue operations Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 10/13] net/ppfe: add supported packet types and basic statistics Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 11/13] net/ppfe: add MTU and MAC address set operations Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 12/13] net/ppfe: add allmulticast and promiscuous Gagandeep Singh
2019-08-28 11:08   ` [dpdk-dev] [PATCH v2 13/13] net/ppfe: add link status update Gagandeep Singh
2019-09-26 17:28   ` [dpdk-dev] [PATCH v2 00/13] introduces ppfe network PMD Ferruh Yigit
2019-09-27 14:55     ` Gagandeep Singh
2019-10-01 11:01 ` [dpdk-dev] [PATCH v3 00/14] " Gagandeep Singh
2019-10-01 11:01   ` [dpdk-dev] [PATCH v3 01/14] common/dpaax: moving OF lib code from dpaa bus Gagandeep Singh
2019-10-01 11:01   ` [dpdk-dev] [PATCH v3 02/14] net/ppfe: introduce ppfe net poll mode driver Gagandeep Singh
2019-10-04 15:38     ` Ferruh Yigit
2019-10-09  6:52       ` Gagandeep Singh
2019-10-01 11:01   ` [dpdk-dev] [PATCH v3 03/14] doc: add guide for ppfe net PMD Gagandeep Singh
2019-10-04 15:41     ` Ferruh Yigit
2019-10-09  6:54       ` Gagandeep Singh
2019-10-01 11:01   ` [dpdk-dev] [PATCH v3 04/14] net/ppfe: support dynamic logging Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 05/14] net/ppfe: add HW specific macros and operations Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 06/14] net/ppfe: add MAC and host interface initialisation Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 07/14] net/ppfe: add device start stop operations Gagandeep Singh
2019-10-04 15:42     ` Ferruh Yigit
2019-10-09  6:54       ` Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 08/14] net/ppfe: add queue setup and release operations Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 09/14] net/ppfe: add burst enqueue and dequeue operations Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 10/14] net/ppfe: add supported packet types and basic statistics Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 11/14] net/ppfe: add MTU and MAC address set operations Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 12/14] net/ppfe: add allmulticast and promiscuous Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 13/14] net/ppfe: add link status update Gagandeep Singh
2019-10-04 15:43     ` Ferruh Yigit
2019-10-09  6:57       ` Gagandeep Singh
2019-10-01 11:02   ` [dpdk-dev] [PATCH v3 14/14] doc: add NXP PPFE PMD in release notes Gagandeep Singh
2019-10-10  6:32   ` [dpdk-dev] [PATCH v4 00/14] introduces pfe network PMD Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 01/14] common/dpaax: moving OF lib code from dpaa bus Gagandeep Singh
2019-10-10 17:01       ` Ferruh Yigit
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 02/14] net/pfe: introduce pfe net poll mode driver Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 03/14] doc: add guide for pfe net PMD Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 04/14] net/pfe: support dynamic logging Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 05/14] net/pfe: add HW specific macros and operations Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 06/14] net/pfe: add MAC and host interface initialisation Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 07/14] net/pfe: add device start stop operations Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 08/14] net/pfe: add queue setup and release operations Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 09/14] net/pfe: add burst enqueue and dequeue operations Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 10/14] net/pfe: add supported packet types and basic statistics Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 11/14] net/pfe: add MTU and MAC address set operations Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 12/14] net/pfe: add allmulticast and promiscuous Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 13/14] net/pfe: add link status update Gagandeep Singh
2019-10-10  6:32     ` [dpdk-dev] [PATCH v4 14/14] doc: add NXP PFE PMD in release notes Gagandeep Singh
2019-10-10  7:11     ` [dpdk-dev] [PATCH v4 00/14] introduces pfe network PMD Thomas Monjalon
2019-10-10 17:01       ` Ferruh Yigit
2019-10-10 17:47     ` Ferruh Yigit
2019-10-25  7:59       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=55a79342-cab2-042b-a7fe-ae45b03150e9@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=akhil.goyal@nxp.com \
    --cc=dev@dpdk.org \
    --cc=g.singh@nxp.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).