From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E6CE042529; Wed, 6 Sep 2023 13:03:36 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 79A8A4027E; Wed, 6 Sep 2023 13:03:36 +0200 (CEST) Received: from smtpbguseast2.qq.com (smtpbguseast2.qq.com [54.204.34.130]) by mails.dpdk.org (Postfix) with ESMTP id 30ED44027C for ; Wed, 6 Sep 2023 13:03:33 +0200 (CEST) X-QQ-mid: bizesmtp83t1693998190tla6if5r Received: from LAPTOP96V0OHHN ( [183.81.182.182]) by bizesmtp.qq.com (ESMTP) with id ; Wed, 06 Sep 2023 19:03:08 +0800 (CST) X-QQ-SSF: 00400000000000D0F000000A0000000 X-QQ-FEAT: a7KvnLxF6FfM7Ab4BO/lnH0openDAewCf9EYZCRJSy9wb7nmlbQfgHp5085zS ihIi7ovsDwfTII7G1Ee9qezujrLGDjHdrANbf9h+lhp5gsfeNPJilQSpG0pT82rm916BFBd B/ouoYUqvXIx2FGJaiL3+2AZQgC0q/XoJypYwY2QQ24bsmFjjM5U0pnZOXY1P9IBLQmfE2g vSXFZdAommorxt7yC0WfcxA+AhL6swCrSgS7M5XlNrFd4yMGcVTzL83IWBfAaaqx7XS24kv e65spMQTviPtpLkwPXSsJyHY6UkQz8vWQ4t5iHAvzkVBuDKVHFuMef71lgBla4uoZf9S8Jq y0Lferaa4jftYkXXRvV2nweuussNdnHPMVv7R6r1DE8g+7X3HpoTZjEWZSsEzHjCpQiWHcD eBPp9ykJqB7qn3qqG81hBA== X-QQ-GoodBg: 2 X-BIZMAIL-ID: 33677748686037865 From: "11" To: "'Ferruh Yigit'" , "'Anatoly Burakov'" Cc: , , , References: <20230901023050.40893-1-caowenbo@mucse.com> <20230901023050.40893-4-caowenbo@mucse.com> <566b2003-cdde-0f3a-9568-184f64777038@amd.com> In-Reply-To: <566b2003-cdde-0f3a-9568-184f64777038@amd.com> Subject: RE: [PATCH v6 3/8] net/rnp: add device init and uninit Date: Wed, 6 Sep 2023 19:03:09 +0800 Message-ID: <1733C38DB34AA89D+01a601d9e0b1$b9bced60$2d36c820$@mucse.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Outlook 16.0 Content-Language: zh-cn Thread-Index: AQI30P9XUj3aWZJ7n+uYhLh06w3v0AJsYS39Alv3lCWvK3jawA== X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:mucse.com:qybglogicsvrgz:qybglogicsvrgz5a-0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Ferruh, Thanks your kindly review, please see the below comment. Regards Wenbo > -----Original Message----- > From: Ferruh Yigit > Sent: 2023=E5=B9=B49=E6=9C=885=E6=97=A5 23:44 > To: Wenbo Cao ; Anatoly Burakov > > Cc: dev@dpdk.org; thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru; > yaojun@mucse.com > Subject: Re: [PATCH v6 3/8] net/rnp: add device init and uninit >=20 > On 9/1/2023 3:30 AM, Wenbo Cao wrote: > > Add basic init and uninit function > > > > Signed-off-by: Wenbo Cao > > --- > > drivers/net/rnp/base/rnp_hw.h | 19 ++++ > > drivers/net/rnp/meson.build | 1 + > > drivers/net/rnp/rnp.h | 25 +++++ > > drivers/net/rnp/rnp_ethdev.c | 196 = +++++++++++++++++++++++++++++++++- > > drivers/net/rnp/rnp_logs.h | 34 ++++++ > > drivers/net/rnp/rnp_osdep.h | 30 ++++++ > > 6 files changed, 300 insertions(+), 5 deletions(-) create mode > > 100644 drivers/net/rnp/base/rnp_hw.h create mode 100644 > > drivers/net/rnp/rnp_logs.h create mode 100644 > > drivers/net/rnp/rnp_osdep.h > > >=20 > 'rnp_osdep.h' not used at all in the patch, can you move it where it = is used? >=20 I got it. > > diff --git a/drivers/net/rnp/base/rnp_hw.h > > b/drivers/net/rnp/base/rnp_hw.h new file mode 100644 index > > 0000000000..d80d23f4b4 > > --- /dev/null > > +++ b/drivers/net/rnp/base/rnp_hw.h > > @@ -0,0 +1,19 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(C) 2023 Mucse IC Design Ltd. > > + */ > > +#ifndef __RNP_HW_H__ > > +#define __RNP_HW_H__ > > + > > +struct rnp_eth_adapter; > > +struct rnp_hw { > > + struct rnp_eth_adapter *back; > > + void *iobar0; > > + uint32_t iobar0_len; > > + void *iobar4; > > + uint32_t iobar4_len; > > + > > + uint16_t device_id; > > + uint16_t vendor_id; > > +} __rte_cache_aligned; > > + >=20 > All structs seems alligned to the cache, although it may not hurt, it = is better to > align ones that is necessary for performance. >=20 > <...> >=20 For this I need to select a often used struct value? > > + > > +static struct rte_eth_dev * > > +rnp_alloc_eth_port(struct rte_pci_device *primary_pci, char *name) >=20 > Why variable name is 'primary_pci', are there different pci device = types? >=20 Because of the chip is just have two pcie-bdf, the can have max eight = port, So rte_eth_dev_pci_generic_probe alloc the first port, the other port is = a fake string Add for x:x.0_1, x:x.0_2, x:x.0_3, so the 'primary_pci' is to mark the = real pci_addr > > +{ > > + struct rnp_eth_port *port; > > + struct rte_eth_dev *eth_dev; > > + > > + eth_dev =3D rte_eth_dev_allocate(name); > > + if (!eth_dev) { > > + RNP_PMD_DRV_LOG(ERR, "Could not allocate " > > + "eth_dev for %s\n", name); >=20 > Please don't split the log message, instead can do: > RNP_PMD_DRV_LOG(ERR, > "Could not allocate eth_dev for %s\n", > name); >=20 > Same for all log messages. >=20 Ok, I got it. > > + return NULL; > > + } > > + port =3D rte_zmalloc_socket(name, > > + sizeof(*port), > > + RTE_CACHE_LINE_SIZE, > > + primary_pci->device.numa_node); > > + if (!port) { > > + RNP_PMD_DRV_LOG(ERR, "Could not allocate " > > + "rnp_eth_port for %s\n", name); > > + return NULL; >=20 > Should 'eth_dev' released here? >=20 Yes , this need to free resource. > > + } > > + eth_dev->data->dev_private =3D port; > > + eth_dev->process_private =3D calloc(1, sizeof(struct = rnp_share_ops)); > > + if (!eth_dev->process_private) { > > + RNP_PMD_DRV_LOG(ERR, "Could not calloc " > > + "for Process_priv\n"); > > + goto fail_calloc; > > + } > > + return eth_dev; > > +fail_calloc: > > + rte_free(port); > > + rte_eth_dev_release_port(eth_dev); > > + > > + return NULL; > > +} > > + > > +static int > > +rnp_eth_dev_init(struct rte_eth_dev *dev) { > > + struct rte_pci_device *pci_dev =3D RTE_ETH_DEV_TO_PCI(dev); > > + struct rnp_eth_adapter *adapter =3D NULL; > > + char name[RTE_ETH_NAME_MAX_LEN] =3D " "; > > + struct rnp_eth_port *port =3D NULL; > > + struct rte_eth_dev *eth_dev; > > + struct rnp_hw *hw =3D NULL; > > + int32_t p_id; > > + int ret; > > + > > + PMD_INIT_FUNC_TRACE(); > > + > > + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) > > + return 0; > > + memset(name, 0, sizeof(name)); > > + snprintf(name, sizeof(name), "rnp_adapter_%d", = dev->data->port_id); > > + adapter =3D rte_zmalloc(name, sizeof(struct rnp_eth_adapter), 0); > > + if (!adapter) { > > + RNP_PMD_DRV_LOG(ERR, "zmalloc for adapter failed\n"); > > + return -ENOMEM; > > + } > > + hw =3D &adapter->hw; > > + adapter->pdev =3D pci_dev; > > + adapter->eth_dev =3D dev; > > + adapter->num_ports =3D 1; >=20 > This is hardcoded value, so no need the loop below but perhaps it is = for future. >=20 I will change the commit sequence ,first achieve mailbox api get = hw-info. So adapter->num_ports info will get from firmware. > > + hw->back =3D adapter; > > + hw->iobar4 =3D pci_dev->mem_resource[RNP_CFG_BAR].addr; > > + hw->iobar0 =3D pci_dev->mem_resource[RNP_PF_INFO_BAR].addr; > > + hw->iobar4_len =3D pci_dev->mem_resource[RNP_CFG_BAR].len; > > + hw->iobar0_len =3D pci_dev->mem_resource[RNP_PF_INFO_BAR].len; > > + hw->device_id =3D pci_dev->id.device_id; > > + hw->vendor_id =3D pci_dev->id.vendor_id; > > + hw->device_id =3D pci_dev->id.device_id; > > + for (p_id =3D 0; p_id < adapter->num_ports; p_id++) { > > + /* port 0 resource has been allocated When Probe */ >=20 > Why 'When' & 'Probe' starts with uppercase, similar usage exists many = places, > although that is not an issue, just catches eye, can you please fix = them if there is > no specific reason. >=20 There isn't special meaning, I will change it to low. > > + if (!p_id) { > > + eth_dev =3D dev; > > + } else { > > + snprintf(name, sizeof(name), "%s_%d", > > + adapter->pdev->device.name, > > + p_id); > > + eth_dev =3D rnp_alloc_eth_port(pci_dev, name); > > + if (eth_dev) > > + rte_memcpy(eth_dev->process_private, > > + adapter->share_priv, > > + sizeof(*adapter->share_priv)); > > + if (!eth_dev) { >=20 > This can be 'else' leg of above branch. >=20 I will check the code. > > + ret =3D -ENOMEM; > > + goto eth_alloc_error; > > + } > > + } > > + ret =3D rnp_init_port_resource(adapter, eth_dev, name, p_id); > > + if (ret) > > + goto eth_alloc_error; >=20 > 'rnp_init_port_resource()' can't return error but perhaps this check = is for future. >=20 Thanks for you advise, I will consider it . > > + > > + rnp_mac_rx_disable(eth_dev); > > + rnp_mac_tx_disable(eth_dev); > > + } > > + > > + return 0; > > +eth_alloc_error: > > + for (p_id =3D 0; p_id < adapter->num_ports; p_id++) { > > + port =3D adapter->ports[p_id]; > > + if (!port) > > + continue; > > + if (port->eth_dev) { > > + rnp_dev_close(port->eth_dev); > > + rte_eth_dev_release_port(port->eth_dev); > > + if (port->eth_dev->process_private) >=20 > This should crash, because 'port' is 'dev_private' and > 'rte_eth_dev_release_port()' frees the dev_private, so can't access = 'port->' here. >=20 > > + for (p_id =3D 0; p_id < adapter->num_ports; p_id++) { This adapter->num_ports need to use port init success num. > Also 'process_private' set to NULL in the = 'rte_eth_dev_release_port()', it should > be freed in 'rnp_dev_close()', not here. >=20 I have miss check the last dpdk version. > > + free(port->eth_dev->process_private); > > + } > > + rte_free(port); > > + } > > + rte_free(adapter); > > + > > + return 0; > > } > > > > static int > > rnp_eth_dev_uninit(struct rte_eth_dev *eth_dev) { > > - RTE_SET_USED(eth_dev); > > + struct rnp_eth_adapter *adapter =3D RNP_DEV_TO_ADAPTER(eth_dev); > > + struct rnp_eth_port *port =3D NULL; > > + uint8_t p_id; > > + > > + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) > > + return 0; > > > > - return -ENODEV; > > + if (adapter->eth_dev !=3D eth_dev) { > > + RNP_PMD_DRV_LOG(ERR, "Input Argument ethdev " > > + "Isn't Primary Ethdev\n"); > > + return -EINVAL; > > + } > > + for (p_id =3D 0; p_id < adapter->num_ports; p_id++) { > > + port =3D adapter->ports[p_id]; > > + if (!port) > > + continue; > > + if (port->eth_dev) { > > + rnp_dev_close(port->eth_dev); > > + /* Just Release Not Primary Port Allocated By PMD */ > > + if (p_id) > > + rte_eth_dev_release_port(port->eth_dev); > > + } > > + } > > + > > + return 0; > > } > > > > static int > > @@ -84,3 +259,14 @@ static struct rte_pci_driver rte_rnp_pmd =3D { > > RTE_PMD_REGISTER_PCI(net_rnp, rte_rnp_pmd); > > RTE_PMD_REGISTER_PCI_TABLE(net_rnp, pci_id_rnp_map); > > RTE_PMD_REGISTER_KMOD_DEP(net_rnp, "igb_uio | uio_pci_generic | > > vfio-pci"); > > + > > +RTE_LOG_REGISTER_SUFFIX(rnp_init_logtype, init, NOTICE); > > +RTE_LOG_REGISTER_SUFFIX(rnp_drv_logtype, driver, NOTICE); > > + >=20 > Do you really need two different log types? What about reducing it to = one? >=20 > <...> >=20 This can reduce it . > > diff --git a/drivers/net/rnp/rnp_osdep.h = b/drivers/net/rnp/rnp_osdep.h > > new file mode 100644 index 0000000000..5685dd2404 > > --- /dev/null > > +++ b/drivers/net/rnp/rnp_osdep.h > > @@ -0,0 +1,30 @@ > > +#ifndef __RNP_OSDEP_H__ > > +#define __RNP_OSDEP_H__ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(C) 2023 Mucse IC Design Ltd. > > + */ > > +#include > > + > > +#include > > + > > +#define __iomem > > +#define _RING_(off) ((off) + 0x000000) > > +#define _DMA_(off) ((off)) > > +#define _GLB_(off) ((off) + 0x000000) > > +#define _NIC_(off) ((off) + 0x000000) > > +#define _ETH_(off) ((off)) > > +#define _MAC_(off) ((off)) > > +#define BIT(n) (1UL << (n)) > > +#define BIT64(n) (1ULL << (n)) >=20 > DPDK has RTE_BIT64 / RTE_BIT32 macros that can be reused. >=20 Old DPDK version don't have the macro, this can be removed. > > +#define BITS_PER_LONG (__SIZEOF_LONG__ * 8) > > +#define GENMASK(h, l) \ > > + (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h)))) > > + > > +typedef uint8_t u8; > > +typedef uint16_t u16; > > +typedef uint32_t u32; > > +typedef uint64_t u64; > > +typedef int32_t s32; > > +typedef int16_t s16; > > +typedef int8_t s8; > > +#endif /* __RNP_OSDEP_H__ */ >=20