[dpdk-dev] [PATCH 22/38] net/dpaa: add NXP DPAA PMD driver skeleton

[Shreyansh Jain]

Hello Ferruh,

I was almost wondering if this series has been forgotten. Thanks for
the comprehensive review.

My comments inline (and in some other mails):

On Wednesday 28 June 2017 09:11 PM, Ferruh Yigit wrote:
> 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 at nxp.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain at 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

I will correct this.


> 
> <...>
> 
>> +
>> +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

Ok. Will fix this.

> 
>> +
>> +	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.

Agree. I will correct this.

> 
> 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?

Yes, rte_dpaa_device ~ rte_pci_device.
This is used to extract the eth_dev back while de-initializing
the device.

  driver->remove = rte_dpaa_remove(rte_dpaa_device)
  // fetch rte_eth_dev from rte_dpaa_device
   `-> .eth_dev_stop(eth_dev)

So, essentially reusing the internal eth_ops for cleaning up the
device.

> 
>> +	eth_dev->data->rx_mbuf_alloc_failed = 0;
> 
> not required, data already memset via rte_eth_dev_allocate()

Ok. I will remove this.

> 
>> +
>> +	/* 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);

Hmm, ok, now I understand why you are inquiring about
eth_dev being assigned in rte_dpaa_device. I will have a
relook at this and fix if required.

> 
>> +
>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>> +		rte_free(eth_dev->data->dev_private);
>> +
> 
> no pmd uninit() ?

I will fix this. There is an internal commit that we had very
recently (a miss in previous series).

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