From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C3AC0A0613 for ; Thu, 26 Sep 2019 18:53:44 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EC9692C36; Thu, 26 Sep 2019 18:53:42 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id CBFAD2C30 for ; Thu, 26 Sep 2019 18:53:40 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 09:53:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="196435279" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.10]) ([10.237.221.10]) by FMSMGA003.fm.intel.com with ESMTP; 26 Sep 2019 09:53:38 -0700 To: Gagandeep Singh , dev@dpdk.org Cc: thomas@monjalon.net, Akhil Goyal References: <20190826130246.30485-1-g.singh@nxp.com> <20190828110849.14759-1-g.singh@nxp.com> <20190828110849.14759-3-g.singh@nxp.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEAFiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl1meboFCQlupOoACgkQ+TPrQ98T YR9ACBAAv2tomhyxY0Tp9Up7mNGLfEdBu/7joB/vIdqMRv63ojkwr9orQq5V16V/25+JEAD0 60cKodBDM6HdUvqLHatS8fooWRueSXHKYwJ3vxyB2tWDyZrLzLI1jxEvunGodoIzUOtum0Ce gPynnfQCelXBja0BwLXJMplM6TY1wXX22ap0ZViC0m714U5U4LQpzjabtFtjT8qOUR6L7hfy YQ72PBuktGb00UR/N5UrR6GqB0x4W41aZBHXfUQnvWIMmmCrRUJX36hOTYBzh+x86ULgg7H2 1499tA4o6rvE13FiGccplBNWCAIroAe/G11rdoN5NBgYVXu++38gTa/MBmIt6zRi6ch15oLA Ln2vHOdqhrgDuxjhMpG2bpNE36DG/V9WWyWdIRlz3NYPCDM/S3anbHlhjStXHOz1uHOnerXM 1jEjcsvmj1vSyYoQMyRcRJmBZLrekvgZeh7nJzbPHxtth8M7AoqiZ/o/BpYU+0xZ+J5/szWZ aYxxmIRu5ejFf+Wn9s5eXNHmyqxBidpCWvcbKYDBnkw2+Y9E5YTpL0mS0dCCOlrO7gca27ux ybtbj84aaW1g0CfIlUnOtHgMCmz6zPXThb+A8H8j3O6qmPoVqT3qnq3Uhy6GOoH8Fdu2Vchh TWiF5yo+pvUagQP6LpslffufSnu+RKAagkj7/RSuZV25Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXWZ5wAUJB3FgggAKCRD5M+tD3xNhH2O+D/9OEz62YuJQLuIuOfL67eFTIB5/1+0j8Tsu o2psca1PUQ61SZJZOMl6VwNxpdvEaolVdrpnSxUF31kPEvR0Igy8HysQ11pj8AcgH0a9FrvU /8k2Roccd2ZIdpNLkirGFZR7LtRw41Kt1Jg+lafI0efkiHKMT/6D/P1EUp1RxOBNtWGV2hrd 0Yg9ds+VMphHHU69fDH02SwgpvXwG8Qm14Zi5WQ66R4CtTkHuYtA63sS17vMl8fDuTCtvfPF HzvdJLIhDYN3Mm1oMjKLlq4PUdYh68Fiwm+boJoBUFGuregJFlO3hM7uHBDhSEnXQr5mqpPM 6R/7Q5BjAxrwVBisH0yQGjsWlnysRWNfExAE2sRePSl0or9q19ddkRYltl6X4FDUXy2DTXa9 a+Fw4e1EvmcF3PjmTYs9IE3Vc64CRQXkhujcN4ZZh5lvOpU8WgyDxFq7bavFnSS6kx7Tk29/ wNJBp+cf9qsQxLbqhW5kfORuZGecus0TLcmpZEFKKjTJBK9gELRBB/zoN3j41hlEl7uTUXTI JQFLhpsFlEdKLujyvT/aCwP3XWT+B2uZDKrMAElF6ltpTxI53JYi22WO7NH7MR16Fhi4R6vh FHNBOkiAhUpoXRZXaCR6+X4qwA8CwHGqHRBfYFSU/Ulq1ZLR+S3hNj2mbnSx0lBs1eEqe2vh cA== Message-ID: <55a79342-cab2-042b-a7fe-ae45b03150e9@intel.com> Date: Thu, 26 Sep 2019 17:53:37 +0100 MIME-Version: 1.0 In-Reply-To: <20190828110849.14759-3-g.singh@nxp.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 02/13] net/ppfe: introduce ppfe net poll mode driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > Signed-off-by: Akhil Goyal > Acked-by: Nipun Gupta <...> > @@ -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= "); Better to use 'PPFE_VDEV_GEM_ID_ARG' macro here to prevent typos etc...