From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ferruh Yigit <ferruh.yigit@intel.com>
Subject: Re: [PATCH 22/38] net/dpaa: add NXP DPAA PMD driver
	skeleton
Date: Wed, 28 Jun 2017 16:41:56 +0100
Message-ID: <39ded621-6575-fdbe-8d7c-9f1f5dd6be6e@intel.com>
References: <1497591668-3320-1-git-send-email-shreyansh.jain@nxp.com>
 <1497591668-3320-23-git-send-email-shreyansh.jain@nxp.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: hemant.agrawal@nxp.com
To: Shreyansh Jain <shreyansh.jain@nxp.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
 by dpdk.org (Postfix) with ESMTP id CA7E02904
 for <dev@dpdk.org>; Wed, 28 Jun 2017 17:41:58 +0200 (CEST)
In-Reply-To: <1497591668-3320-23-git-send-email-shreyansh.jain@nxp.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 6/16/2017 6:40 AM, Shreyansh Jain wrote:
> A skeleton which would be called after bus device scan. It currently
> fails to identify the device>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>

<...>

> +
> +/* Initialise a network interface */
> +static int dpaa_eth_dev_init(struct rte_eth_dev *eth_dev __rte_unused)

__rte_unused can be removed

<...>

> +
> +static int
> +rte_dpaa_probe(struct rte_dpaa_driver *dpaa_drv __rte_unused,
> +			   struct rte_dpaa_device *dpaa_dev)
> +{
> +	int diag;
> +	int ret;
> +	struct rte_eth_dev *eth_dev;
> +	char ethdev_name[RTE_ETH_NAME_MAX_LEN];
> +
> +	PMD_INIT_FUNC_TRACE();
> +
> +	if (!is_global_init) {
> +		/* One time load of Qman/Bman drivers */
> +		ret = qman_global_init();
> +		if (ret) {
> +			PMD_DRV_LOG(ERR, "QMAN initialization failed: %d",
> +				    ret);
> +			return ret;
> +		}
> +		ret = bman_global_init();
> +		if (ret) {
> +			PMD_DRV_LOG(ERR, "BMAN initialization failed: %d",
> +				    ret);
> +			return ret;
> +		}
> +
> +		is_global_init = 1;
> +	}
> +
> +	sprintf(ethdev_name, "%s", dpaa_dev->name);

snprintf can be preferred

> +
> +	ret = rte_dpaa_portal_init((void *)1);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "Unable to initialize portal");
> +		return ret;
> +	}
> +
> +	eth_dev = rte_eth_dev_allocate(ethdev_name);

If this is done without RTE_PROC_PRIMARY check, this will cause
secondary to memset all device data.

I am adding this because of below check, it multi process support is
intended, this also be protected.

> +	if (eth_dev == NULL)
> +		return -ENOMEM;
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev->data->dev_private = rte_zmalloc(
> +						"ethdev private structure",
> +						sizeof(struct dpaa_if),
> +						RTE_CACHE_LINE_SIZE);
> +		if (!eth_dev->data->dev_private) {
> +			PMD_INIT_LOG(CRIT, "Cannot allocate memzone for"
> +				     " private port data\n");
> +			rte_eth_dev_release_port(eth_dev);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	eth_dev->device = &dpaa_dev->device;
> +	dpaa_dev->eth_dev = eth_dev;

I thought "struct rte_dpaa_device" is bus device, like "struct
rte_pci_device", if so why it has link to the eth_dev?

> +	eth_dev->data->rx_mbuf_alloc_failed = 0;

not required, data already memset via rte_eth_dev_allocate()

> +
> +	/* Invoke PMD device initialization function */
> +	diag = dpaa_eth_dev_init(eth_dev);
> +	if (diag) {
> +		PMD_DRV_LOG(ERR, "Eth dev initialization failed: %d", ret);
> +		return diag;
> +	}
> +
> +	PMD_DRV_LOG(DEBUG, "Eth dev initialized: %d\n", diag);
> +
> +	return 0;
> +}
> +
> +static int
> +rte_dpaa_remove(struct rte_dpaa_device *dpaa_dev)
> +{
> +	struct rte_eth_dev *eth_dev;
> +
> +	PMD_INIT_FUNC_TRACE();
> +
> +	eth_dev = dpaa_dev->eth_dev;

can be:
eth_dev = rte_eth_dev_allocated(dpaa_dev->device.name);

> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +		rte_free(eth_dev->data->dev_private);
> +

no pmd uninit() ?

> +	rte_eth_dev_release_port(eth_dev);
> +
> +	return 0;
> +}

<...>