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,
> + ¶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...
next prev parent 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).