RE: [PATCH v6 3/8] net/rnp: add device init and uninit

["11" <caowenbo@mucse.com>]

Hi Ferruh,

Thanks your kindly review, please see the below comment.

Regards Wenbo

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: 2023年9月5日 23:44
> To: Wenbo Cao <caowenbo@mucse.com>; Anatoly Burakov
> <anatoly.burakov@intel.com>
> 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
> 
> On 9/1/2023 3:30 AM, Wenbo Cao wrote:
> > Add basic init and uninit function
> >
> > Signed-off-by: Wenbo Cao <caowenbo@mucse.com>
> > ---
> >  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
> >
> 
> 'rnp_osdep.h' not used at all in the patch, can you move it where it is used?
> 
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;
> > +
> 
> All structs seems alligned to the cache, although it may not hurt, it is better to
> align ones that is necessary for performance.
> 
> <...>
> 
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)
> 
> Why variable name is 'primary_pci', are there different pci device types?
> 
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 = rte_eth_dev_allocate(name);
> > +	if (!eth_dev) {
> > +		RNP_PMD_DRV_LOG(ERR, "Could not allocate "
> > +				"eth_dev for %s\n", name);
> 
> Please don't split the log message, instead can do:
> RNP_PMD_DRV_LOG(ERR,
> 	"Could not allocate eth_dev for %s\n",
> 	name);
> 
> Same for all log messages.
> 
Ok, I got it.
> > +		return NULL;
> > +	}
> > +	port = 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;
> 
> Should 'eth_dev' released here?
> 
Yes , this need to free resource.
> > +	}
> > +	eth_dev->data->dev_private = port;
> > +	eth_dev->process_private = 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 = RTE_ETH_DEV_TO_PCI(dev);
> > +	struct rnp_eth_adapter *adapter = NULL;
> > +	char name[RTE_ETH_NAME_MAX_LEN] = " ";
> > +	struct rnp_eth_port *port = NULL;
> > +	struct rte_eth_dev *eth_dev;
> > +	struct rnp_hw *hw = NULL;
> > +	int32_t p_id;
> > +	int ret;
> > +
> > +	PMD_INIT_FUNC_TRACE();
> > +
> > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return 0;
> > +	memset(name, 0, sizeof(name));
> > +	snprintf(name, sizeof(name), "rnp_adapter_%d", dev->data->port_id);
> > +	adapter = rte_zmalloc(name, sizeof(struct rnp_eth_adapter), 0);
> > +	if (!adapter) {
> > +		RNP_PMD_DRV_LOG(ERR, "zmalloc for adapter failed\n");
> > +		return -ENOMEM;
> > +	}
> > +	hw = &adapter->hw;
> > +	adapter->pdev = pci_dev;
> > +	adapter->eth_dev = dev;
> > +	adapter->num_ports = 1;
> 
> This is hardcoded value, so no need the loop below but perhaps it is for future.
> 
I will change the commit sequence ,first achieve mailbox api get hw-info.
So adapter->num_ports info will get from firmware.
> > +	hw->back = adapter;
> > +	hw->iobar4 = pci_dev->mem_resource[RNP_CFG_BAR].addr;
> > +	hw->iobar0 = pci_dev->mem_resource[RNP_PF_INFO_BAR].addr;
> > +	hw->iobar4_len = pci_dev->mem_resource[RNP_CFG_BAR].len;
> > +	hw->iobar0_len = pci_dev->mem_resource[RNP_PF_INFO_BAR].len;
> > +	hw->device_id = pci_dev->id.device_id;
> > +	hw->vendor_id = pci_dev->id.vendor_id;
> > +	hw->device_id = pci_dev->id.device_id;
> > +	for (p_id = 0; p_id < adapter->num_ports; p_id++) {
> > +		/* port 0 resource has been allocated When Probe */
> 
> 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.
> 
There isn't special meaning, I will change it to low.
> > +		if (!p_id) {
> > +			eth_dev = dev;
> > +		} else {
> > +			snprintf(name, sizeof(name), "%s_%d",
> > +					adapter->pdev->device.name,
> > +					p_id);
> > +			eth_dev = 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) {
> 
> This can be 'else' leg of above branch.
> 
I will check the code.
> > +				ret = -ENOMEM;
> > +				goto eth_alloc_error;
> > +			}
> > +		}
> > +		ret = rnp_init_port_resource(adapter, eth_dev, name, p_id);
> > +		if (ret)
> > +			goto eth_alloc_error;
> 
> 'rnp_init_port_resource()' can't return error but perhaps this check is for future.
> 
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 = 0; p_id < adapter->num_ports; p_id++) {
> > +		port = 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)
> 
> This should crash, because 'port' is 'dev_private' and
> 'rte_eth_dev_release_port()' frees the dev_private, so can't access 'port->' here.
> 
> > +	for (p_id = 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.
> 
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 = RNP_DEV_TO_ADAPTER(eth_dev);
> > +	struct rnp_eth_port *port = NULL;
> > +	uint8_t p_id;
> > +
> > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return 0;
> >
> > -	return -ENODEV;
> > +	if (adapter->eth_dev != eth_dev) {
> > +		RNP_PMD_DRV_LOG(ERR, "Input Argument ethdev "
> > +			       "Isn't Primary Ethdev\n");
> > +		return -EINVAL;
> > +	}
> > +	for (p_id = 0; p_id < adapter->num_ports; p_id++) {
> > +		port = 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 = {
> > 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);
> > +
> 
> Do you really need two different log types? What about reducing it to one?
> 
> <...>
> 
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 <stdint.h>
> > +
> > +#include <rte_byteorder.h>
> > +
> > +#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))
> 
> DPDK has RTE_BIT64 / RTE_BIT32 macros that can be reused.
> 
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__ */
>