[dpdk-dev] [PATCH v4 25/41] net/dpaa: add support for Tx and Rx queue setup

[Shreyansh Jain]

Hello Ferruh,

Apologies for delay in response for these, I am already working to get 
the next version based on your comments. Meanwhile, some comments inline...

On Monday 18 September 2017 08:25 PM, Ferruh Yigit wrote:
> On 9/9/2017 12:21 PM, Shreyansh Jain wrote:
>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
> 
> <...>
> 
>> +	for (loop = 0; loop < num_cores; loop++) {
>> +		ret = dpaa_tx_queue_init(&dpaa_intf->tx_queues[loop],
>> +					 fman_intf);
>> +		if (ret)
>> +			return ret;
>> +		dpaa_intf->tx_queues[loop].dpaa_intf = dpaa_intf;
>> +	}
>> +	dpaa_intf->nb_tx_queues = num_cores;
> 
> Is number of the tx_queues always same as core count?

Number of cores decides the max number of queues that we support. It is 
our internal design to use this as maximum number of available tx queue.

With the above variable, we are only limiting what we will copy in the 
dev_info->max_tx_queues to be reported to application from eth_dev_info. 
Application would still continue to use its own Tx queue ids.

But yes, we don't yet support limiting the number of queues to a user 
count, if less than number of cores.

> 
>> +
>> +	DPAA_PMD_DEBUG("All frame queues created");
>> +
>> +	/* reset bpool list, initialize bpool dynamically */
>> +	list_for_each_entry_safe(bp, tmp_bp, &cfg->fman_if->bpool_list, node) {
>> +		list_del(&bp->node);
>> +		rte_free(bp);
> 
> Why freeing them during initialization ?

Just in case if there is anything still in the list.
This is possible in case where multiple devie

> 
>> +	}
>> +
>> +	/* Populate ethdev structure */
>>   	eth_dev->dev_ops = &dpaa_devops;
>> +	eth_dev->rx_pkt_burst = dpaa_eth_queue_rx;
>> +	eth_dev->tx_pkt_burst = dpaa_eth_tx_drop_all;
>> +
>> +	/* Allocate memory for storing MAC addresses */
>> +	eth_dev->data->mac_addrs = rte_zmalloc("mac_addr",
>> +		ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER, 0);
>> +	if (eth_dev->data->mac_addrs == NULL) {
>> +		DPAA_PMD_ERR("Failed to allocate %d bytes needed to "
>> +						"store MAC addresses",
>> +				ETHER_ADDR_LEN * DPAA_MAX_MAC_FILTER);
> 
> free dpaa_intf->rx_queues, tx_queues ?

yes, certainly an issue. I will fix it.

> 
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* copy the primary mac address */
>> +	memcpy(eth_dev->data->mac_addrs[0].addr_bytes,
>> +		fman_intf->mac_addr.addr_bytes,
>> +		ETHER_ADDR_LEN);
> 
> Instead can use ether_addr_copy() instead.

:) Yes, I can.

> 
> <...>
>