Re: [PATCH v5 01/12] net/nfp: move app specific attributes to own struct

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 8/5/22 09:32, Chaoyong He wrote:
> The NFP Card can load different firmware applications. Currently
> only the CoreNIC application is supported. This commit makes
> needed infrastructure changes in order to support other firmware
> applications too.
> 
> Clearer separation is made between the PF device and any application
> specific concepts. The PF struct is now generic regardless of the
> application loaded. A new struct is also made for the CoreNIC
> application. Future additions to support other applications should
> also add an applications specific struct.
> 
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Signed-off-by: Heinrich Kuhn <heinrich.kuhn@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> ---
>   drivers/net/nfp/nfp_common.h |  33 +++++++-
>   drivers/net/nfp/nfp_ethdev.c | 196 +++++++++++++++++++++++++++----------------
>   2 files changed, 154 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_common.h b/drivers/net/nfp/nfp_common.h
> index 6d917e4..2aaf1d6 100644
> --- a/drivers/net/nfp/nfp_common.h
> +++ b/drivers/net/nfp/nfp_common.h
> @@ -111,6 +111,14 @@
>   #include <linux/types.h>
>   #include <rte_io.h>
>   
> +/* Firmware application ID's */
> +enum nfp_app_id {
> +	NFP_APP_CORE_NIC               = 0x1,

[snip]

> +	NFP_APP_BPF_NIC                = 0x2,
> +	NFP_APP_FLOWER_NIC             = 0x3,
> +	NFP_APP_ACTIVE_BUFFER_MGMT_NIC = 0x4,

Above three defines are dead in the patch. I think it would make
subsequent patches more selfcontained if corresponding defines are
added later when they are really required and used.

> +};
> +
>   /* nfp_qcp_ptr - Read or Write Pointer of a queue */
>   enum nfp_qcp_ptr {
>   	NFP_QCP_READ_PTR = 0,
> @@ -121,8 +129,10 @@ struct nfp_pf_dev {
>   	/* Backpointer to associated pci device */
>   	struct rte_pci_device *pci_dev;
>   
> -	/* Array of physical ports belonging to this PF */
> -	struct nfp_net_hw *ports[NFP_MAX_PHYPORTS];
> +	enum nfp_app_id app_id;
> +
> +	/* Pointer to the app running on the PF */
> +	void *app_priv;
>   
>   	/* Current values for control */
>   	uint32_t ctrl;
> @@ -151,8 +161,6 @@ struct nfp_pf_dev {
>   	struct nfp_cpp_area *msix_area;
>   
>   	uint8_t *hw_queues;
> -	uint8_t total_phyports;
> -	bool	multiport;
>   
>   	union eth_table_entry *eth_table;
>   
> @@ -161,6 +169,20 @@ struct nfp_pf_dev {
>   	uint32_t nfp_cpp_service_id;
>   };
>   
> +struct nfp_app_nic {
> +	/* Backpointer to the PF device */
> +	struct nfp_pf_dev *pf_dev;
> +
> +	/*
> +	 * Array of physical ports belonging to the this CoreNIC app
> +	 * This is really a list of vNIC's. One for each physical port
> +	 */
> +	struct nfp_net_hw *ports[NFP_MAX_PHYPORTS];
> +
> +	bool multiport;
> +	uint8_t total_phyports;
> +};
> +
>   struct nfp_net_hw {
>   	/* Backpointer to the PF this port belongs to */
>   	struct nfp_pf_dev *pf_dev;
> @@ -424,6 +446,9 @@ int nfp_net_rss_hash_conf_get(struct rte_eth_dev *dev,
>   #define NFP_NET_DEV_PRIVATE_TO_PF(dev_priv)\
>   	(((struct nfp_net_hw *)dev_priv)->pf_dev)
>   
> +#define NFP_APP_PRIV_TO_APP_NIC(app_priv)\
> +	((struct nfp_app_nic *)app_priv)
> +

Wouldn't it be better if tiny function is used instead.
It should accept struct nfp_pf_dev pointer as an input argument.
It would allow to validate that pf_dev->app_id is NFP_APP_CORE_NIC and
make code more robust.

>   #endif /* _NFP_COMMON_H_ */
>   /*
>    * Local variables:
> diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
> index 5cdd34e..3c4b0ac 100644
> --- a/drivers/net/nfp/nfp_ethdev.c
> +++ b/drivers/net/nfp/nfp_ethdev.c
> @@ -39,15 +39,15 @@
>   #include "nfp_cpp_bridge.h"
>   
>   static int
> -nfp_net_pf_read_mac(struct nfp_pf_dev *pf_dev, int port)
> +nfp_net_pf_read_mac(struct nfp_app_nic *app_nic, int port)
>   {
>   	struct nfp_eth_table *nfp_eth_table;
>   	struct nfp_net_hw *hw = NULL;
>   
>   	/* Grab a pointer to the correct physical port */
> -	hw = pf_dev->ports[port];
> +	hw = app_nic->ports[port];
>   
> -	nfp_eth_table = nfp_eth_read_ports(pf_dev->cpp);
> +	nfp_eth_table = nfp_eth_read_ports(app_nic->pf_dev->cpp);
>   
>   	nfp_eth_copy_mac((uint8_t *)&hw->mac_addr,
>   			 (uint8_t *)&nfp_eth_table->ports[port].mac_addr);
> @@ -64,6 +64,7 @@
>   	uint32_t new_ctrl, update = 0;
>   	struct nfp_net_hw *hw;
>   	struct nfp_pf_dev *pf_dev;
> +	struct nfp_app_nic *app_nic;
>   	struct rte_eth_conf *dev_conf;
>   	struct rte_eth_rxmode *rxmode;
>   	uint32_t intr_vector;
> @@ -71,6 +72,7 @@
>   
>   	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>   	pf_dev = NFP_NET_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	app_nic = NFP_APP_PRIV_TO_APP_NIC(pf_dev->app_priv);
>   
>   	PMD_INIT_LOG(DEBUG, "Start");
>   
> @@ -82,7 +84,7 @@
>   
>   	/* check and configure queue intr-vector mapping */
>   	if (dev->data->dev_conf.intr_conf.rxq != 0) {
> -		if (pf_dev->multiport) {
> +		if (app_nic->multiport) {
>   			PMD_INIT_LOG(ERR, "PMD rx interrupt is not supported "
>   					  "with NFP multiport PF");
>   				return -EINVAL;
> @@ -250,6 +252,7 @@
>   	struct nfp_net_hw *hw;
>   	struct rte_pci_device *pci_dev;
>   	struct nfp_pf_dev *pf_dev;
> +	struct nfp_app_nic *app_nic;
>   	int i;
>   
>   	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> @@ -260,6 +263,7 @@
>   	pf_dev = NFP_NET_DEV_PRIVATE_TO_PF(dev->data->dev_private);
>   	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>   	pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	app_nic = NFP_APP_PRIV_TO_APP_NIC(pf_dev->app_priv);
>   
>   	/*
>   	 * We assume that the DPDK application is stopping all the
> @@ -280,12 +284,12 @@
>   	/* Only free PF resources after all physical ports have been closed */
>   	/* Mark this port as unused and free device priv resources*/
>   	nn_cfg_writeb(hw, NFP_NET_CFG_LSC, 0xff);
> -	pf_dev->ports[hw->idx] = NULL;
> +	app_nic->ports[hw->idx] = NULL;
>   	rte_eth_dev_release_port(dev);
>   
> -	for (i = 0; i < pf_dev->total_phyports; i++) {
> +	for (i = 0; i < app_nic->total_phyports; i++) {
>   		/* Check to see if ports are still in use */
> -		if (pf_dev->ports[i])
> +		if (app_nic->ports[i])
>   			return 0;
>   	}
>   
> @@ -296,6 +300,7 @@
>   	free(pf_dev->hwinfo);
>   	free(pf_dev->sym_tbl);
>   	nfp_cpp_free(pf_dev->cpp);
> +	rte_free(app_nic);
>   	rte_free(pf_dev);
>   
>   	rte_intr_disable(pci_dev->intr_handle);
> @@ -404,6 +409,7 @@
>   {
>   	struct rte_pci_device *pci_dev;
>   	struct nfp_pf_dev *pf_dev;
> +	struct nfp_app_nic *app_nic;
>   	struct nfp_net_hw *hw;
>   	struct rte_ether_addr *tmp_ether_addr;
>   	uint64_t rx_bar_off = 0;
> @@ -420,6 +426,9 @@
>   	/* Use backpointer here to the PF of this eth_dev */
>   	pf_dev = NFP_NET_DEV_PRIVATE_TO_PF(eth_dev->data->dev_private);
>   
> +	/* Use backpointer to the CoreNIC app struct */
> +	app_nic = NFP_APP_PRIV_TO_APP_NIC(pf_dev->app_priv);
> +
>   	/* NFP can not handle DMA addresses requiring more than 40 bits */
>   	if (rte_mem_check_dma_mask(40)) {
>   		RTE_LOG(ERR, PMD,
> @@ -438,7 +447,7 @@
>   	 * Use PF array of physical ports to get pointer to
>   	 * this specific port
>   	 */
> -	hw = pf_dev->ports[port];
> +	hw = app_nic->ports[port];
>   
>   	PMD_INIT_LOG(DEBUG, "Working with physical port number: %d, "
>   			"NFP internal port number: %d", port, hw->nfp_idx);
> @@ -568,7 +577,7 @@
>   		goto dev_err_queues_map;
>   	}
>   
> -	nfp_net_pf_read_mac(pf_dev, port);
> +	nfp_net_pf_read_mac(app_nic, port);
>   	nfp_net_write_mac(hw, (uint8_t *)&hw->mac_addr);
>   
>   	tmp_ether_addr = (struct rte_ether_addr *)&hw->mac_addr;
> @@ -718,25 +727,67 @@
>   }
>   
>   static int
> -nfp_init_phyports(struct nfp_pf_dev *pf_dev)
> +nfp_init_app_nic(struct nfp_pf_dev *pf_dev,
> +		struct nfp_eth_table *nfp_eth_table)
>   {
>   	int i;
> -	int ret = 0;
> +	int ret;
> +	int err = 0;
> +	int total_vnics;
>   	struct nfp_net_hw *hw;
> +	unsigned int numa_node;
>   	struct rte_eth_dev *eth_dev;
> -	struct nfp_eth_table *nfp_eth_table;
> +	struct nfp_app_nic *app_nic;
> +	char port_name[RTE_ETH_NAME_MAX_LEN];
>   
> -	nfp_eth_table = nfp_eth_read_ports(pf_dev->cpp);
> -	if (nfp_eth_table == NULL) {
> -		PMD_INIT_LOG(ERR, "Error reading NFP ethernet table");
> -		return -EIO;
> +	PMD_INIT_LOG(INFO, "Total physical ports: %d", nfp_eth_table->count);
> +
> +	/* Allocate memory for the CoreNIC app */
> +	app_nic = rte_zmalloc("nfp_app_nic", sizeof(*app_nic), 0);
> +	if (app_nic == NULL)
> +		return -ENOMEM;
> +
> +	/* Point the app_priv pointer in the PF to the coreNIC app */
> +	pf_dev->app_priv = app_nic;
> +
> +	/* Read the number of vNIC's created for the PF */
> +	total_vnics = nfp_rtsym_read_le(pf_dev->sym_tbl, "nfd_cfg_pf0_num_ports", &err);
> +	if (err || total_vnics <= 0 || total_vnics > 8) {

DPDK coding style says to compare integers with 0 explicitly.
Since both ways are already present in net/nfp code and there is no
clear preference in the driver itself, please, following DPDK coding
style in a new code.

> +		PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value");
> +		ret = -ENODEV;
> +		goto app_cleanup;
>   	}
>   
> -	/* Loop through all physical ports on PF */
> -	for (i = 0; i < pf_dev->total_phyports; i++) {
> -		const unsigned int numa_node = rte_socket_id();
> -		char port_name[RTE_ETH_NAME_MAX_LEN];
> +	/*
> +	 * For coreNIC the number of vNICs exposed should be the same as the
> +	 * number of physical ports
> +	 */
> +	if (total_vnics != (int)nfp_eth_table->count) {
> +		PMD_INIT_LOG(ERR, "Total physical ports do not match number of vNICs");
> +		ret = -ENODEV;
> +		goto app_cleanup;
> +	}
>   
> +	/* Populate coreNIC app properties*/
> +	app_nic->total_phyports = total_vnics;
> +	app_nic->pf_dev = pf_dev;
> +	if (total_vnics > 1)
> +		app_nic->multiport = true;
> +
> +	/* Map the symbol table */
> +	pf_dev->ctrl_bar = nfp_rtsym_map(pf_dev->sym_tbl, "_pf0_net_bar0",
> +			app_nic->total_phyports * 32768, &pf_dev->ctrl_area);
> +	if (pf_dev->ctrl_bar == NULL) {
> +		PMD_INIT_LOG(ERR, "nfp_rtsym_map fails for _pf0_net_ctrl_bar");
> +		ret = -EIO;
> +		goto app_cleanup;
> +	}
> +
> +	PMD_INIT_LOG(DEBUG, "ctrl bar: %p", pf_dev->ctrl_bar);
> +
> +	/* Loop through all physical ports on PF */
> +	numa_node = rte_socket_id();
> +	for (i = 0; i < app_nic->total_phyports; i++) {
>   		snprintf(port_name, sizeof(port_name), "%s_port%d",
>   			 pf_dev->pci_dev->device.name, i);
>   
> @@ -760,7 +811,7 @@
>   		hw = NFP_NET_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
>   
>   		/* Add this device to the PF's array of physical ports */
> -		pf_dev->ports[i] = hw;
> +		app_nic->ports[i] = hw;
>   
>   		hw->pf_dev = pf_dev;
>   		hw->cpp = pf_dev->cpp;
> @@ -783,20 +834,21 @@
>   		rte_eth_dev_probing_finish(eth_dev);
>   
>   	} /* End loop, all ports on this PF */
> -	ret = 0;
> -	goto eth_table_cleanup;
> +
> +	return 0;
>   
>   port_cleanup:
> -	for (i = 0; i < pf_dev->total_phyports; i++) {
> -		if (pf_dev->ports[i] && pf_dev->ports[i]->eth_dev) {
> +	for (i = 0; i < app_nic->total_phyports; i++) {
> +		if (app_nic->ports[i] && app_nic->ports[i]->eth_dev) {
>   			struct rte_eth_dev *tmp_dev;
> -			tmp_dev = pf_dev->ports[i]->eth_dev;
> +			tmp_dev = app_nic->ports[i]->eth_dev;
>   			rte_eth_dev_release_port(tmp_dev);
> -			pf_dev->ports[i] = NULL;
> +			app_nic->ports[i] = NULL;
>   		}
>   	}
> -eth_table_cleanup:
> -	free(nfp_eth_table);
> +	nfp_cpp_area_free(pf_dev->ctrl_area);
> +app_cleanup:
> +	rte_free(app_nic);
>   
>   	return ret;
>   }
> @@ -804,11 +856,11 @@
>   static int
>   nfp_pf_init(struct rte_pci_device *pci_dev)
>   {
> -	int err;
> -	int ret = 0;
> +	int ret;
> +	int err = 0;
>   	uint64_t addr;
> -	int total_ports;
>   	struct nfp_cpp *cpp;
> +	enum nfp_app_id app_id;
>   	struct nfp_pf_dev *pf_dev;
>   	struct nfp_hwinfo *hwinfo;
>   	char name[RTE_ETH_NAME_MAX_LEN];
> @@ -840,9 +892,10 @@
>   	if (hwinfo == NULL) {
>   		PMD_INIT_LOG(ERR, "Error reading hwinfo table");
>   		ret = -EIO;
> -		goto error;
> +		goto cpp_cleanup;
>   	}
>   
> +	/* Read the number of physical ports from hardware */
>   	nfp_eth_table = nfp_eth_read_ports(cpp);
>   	if (nfp_eth_table == NULL) {
>   		PMD_INIT_LOG(ERR, "Error reading NFP ethernet table");
> @@ -865,20 +918,14 @@
>   		goto eth_table_cleanup;
>   	}
>   
> -	total_ports = nfp_rtsym_read_le(sym_tbl, "nfd_cfg_pf0_num_ports", &err);
> -	if (total_ports != (int)nfp_eth_table->count) {
> -		PMD_DRV_LOG(ERR, "Inconsistent number of ports");
> +	/* Read the app ID of the firmware loaded */
> +	app_id = nfp_rtsym_read_le(sym_tbl, "_pf0_net_app_id", &err);
> +	if (err) {

Compare vs 0

> +		PMD_INIT_LOG(ERR, "Couldn't read app_id from fw");
>   		ret = -EIO;
>   		goto sym_tbl_cleanup;
>   	}
>   
> -	PMD_INIT_LOG(INFO, "Total physical ports: %d", total_ports);
> -
> -	if (total_ports <= 0 || total_ports > 8) {
> -		PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value");
> -		ret = -ENODEV;
> -		goto sym_tbl_cleanup;
> -	}
>   	/* Allocate memory for the PF "device" */
>   	snprintf(name, sizeof(name), "nfp_pf%d", 0);
>   	pf_dev = rte_zmalloc(name, sizeof(*pf_dev), 0);
> @@ -888,27 +935,12 @@
>   	}
>   
>   	/* Populate the newly created PF device */
> +	pf_dev->app_id = app_id;
>   	pf_dev->cpp = cpp;
>   	pf_dev->hwinfo = hwinfo;
>   	pf_dev->sym_tbl = sym_tbl;
> -	pf_dev->total_phyports = total_ports;
> -
> -	if (total_ports > 1)
> -		pf_dev->multiport = true;
> -
>   	pf_dev->pci_dev = pci_dev;
>   
> -	/* Map the symbol table */
> -	pf_dev->ctrl_bar = nfp_rtsym_map(pf_dev->sym_tbl, "_pf0_net_bar0",
> -			pf_dev->total_phyports * 32768, &pf_dev->ctrl_area);
> -	if (pf_dev->ctrl_bar == NULL) {
> -		PMD_INIT_LOG(ERR, "nfp_rtsym_map fails for _pf0_net_ctrl_bar");
> -		ret = -EIO;
> -		goto pf_cleanup;
> -	}
> -
> -	PMD_INIT_LOG(DEBUG, "ctrl bar: %p", pf_dev->ctrl_bar);
> -
>   	/* configure access to tx/rx vNIC BARs */
>   	switch (pci_dev->id.device_id) {
>   	case PCI_DEVICE_ID_NFP3800_PF_NIC:
> @@ -923,7 +955,7 @@
>   	default:
>   		PMD_INIT_LOG(ERR, "nfp_net: no device ID matching");
>   		err = -ENODEV;
> -		goto ctrl_area_cleanup;
> +		goto pf_cleanup;
>   	}
>   
>   	pf_dev->hw_queues = nfp_cpp_map_area(pf_dev->cpp, 0, 0,
> @@ -932,18 +964,27 @@
>   	if (pf_dev->hw_queues == NULL) {
>   		PMD_INIT_LOG(ERR, "nfp_rtsym_map fails for net.qc");
>   		ret = -EIO;
> -		goto ctrl_area_cleanup;
> +		goto pf_cleanup;
>   	}
>   
>   	PMD_INIT_LOG(DEBUG, "tx/rx bar address: 0x%p", pf_dev->hw_queues);
>   
>   	/*
> -	 * Initialize and prep physical ports now
> -	 * This will loop through all physical ports
> +	 * PF initialization has been done at this point. Call app specific
> +	 * init code now
>   	 */
> -	ret = nfp_init_phyports(pf_dev);
> -	if (ret) {
> -		PMD_INIT_LOG(ERR, "Could not create physical ports");
> +	switch (pf_dev->app_id) {
> +	case NFP_APP_CORE_NIC:
> +		PMD_INIT_LOG(INFO, "Initializing coreNIC");
> +		ret = nfp_init_app_nic(pf_dev, nfp_eth_table);
> +		if (ret) {

Compare vs 0

> +			PMD_INIT_LOG(ERR, "Could not initialize coreNIC!");
> +			goto hwqueues_cleanup;
> +		}
> +		break;
> +	default:
> +		PMD_INIT_LOG(ERR, "Unsupported Firmware loaded");
> +		ret = -EINVAL;
>   		goto hwqueues_cleanup;
>   	}
>   
> @@ -954,8 +995,6 @@
>   
>   hwqueues_cleanup:
>   	nfp_cpp_area_free(pf_dev->hwqueues_area);
> -ctrl_area_cleanup:
> -	nfp_cpp_area_free(pf_dev->ctrl_area);
>   pf_cleanup:
>   	rte_free(pf_dev);
>   sym_tbl_cleanup:
> @@ -964,6 +1003,8 @@
>   	free(nfp_eth_table);
>   hwinfo_cleanup:
>   	free(hwinfo);
> +cpp_cleanup:
> +	nfp_cpp_free(cpp);
>   error:
>   	return ret;
>   }
> @@ -977,7 +1018,8 @@
>   nfp_pf_secondary_init(struct rte_pci_device *pci_dev)
>   {
>   	int i;
> -	int err;
> +	int err = 0;
> +	int ret = 0;
>   	int total_ports;
>   	struct nfp_cpp *cpp;
>   	struct nfp_net_hw *hw;
> @@ -1015,6 +1057,11 @@
>   	}
>   
>   	total_ports = nfp_rtsym_read_le(sym_tbl, "nfd_cfg_pf0_num_ports", &err);
> +	if (err || total_ports <= 0 || total_ports > 8) {

Compare err vs 0

> +		PMD_INIT_LOG(ERR, "nfd_cfg_pf0_num_ports symbol with wrong value");
> +		ret = -ENODEV;
> +		goto sym_tbl_cleanup;
> +	}
>   
>   	for (i = 0; i < total_ports; i++) {
>   		struct rte_eth_dev *eth_dev;
> @@ -1028,7 +1075,8 @@
>   		if (eth_dev == NULL) {
>   			RTE_LOG(ERR, EAL,
>   				"secondary process attach failed, ethdev doesn't exist");
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			break;
>   		}
>   
>   		hw = NFP_NET_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
> @@ -1041,10 +1089,16 @@
>   		rte_eth_dev_probing_finish(eth_dev);
>   	}
>   
> +	if (ret)

Compare vs 0

> +		goto sym_tbl_cleanup;
> +
>   	/* Register the CPP bridge service for the secondary too */
>   	nfp_register_cpp_service(cpp);
>   
> -	return 0;
> +sym_tbl_cleanup:
> +	free(sym_tbl);
> +
> +	return ret;
>   }
>   
>   static int