[dpdk-dev] [PATCH 24/38] net/dpaa: add support for Tx and Rx queue setup

[Shreyansh Jain]

On Thursday 29 June 2017 09:11 PM, Ferruh Yigit wrote:
> On 6/29/2017 3:55 PM, Shreyansh Jain wrote:
>> On Wednesday 28 June 2017 09:15 PM, Ferruh Yigit wrote:
>>> On 6/16/2017 6:40 AM, Shreyansh Jain wrote:
>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>>>> Signed-off-by: Shreyansh Jain <shreyansh.jain at nxp.com>
>>>> ---
> 
> <...>
> 
>>>
>>>> +
>>>> +	/* Initialize Rx FQ's */
>>>> +	if (getenv("DPAA_NUM_RX_QUEUES"))
>>>
>>> I think this was disscussed before, should a PMD get config options from
>>> enviroment variable? Altough this works, I am for a more explicit
>>> method, like dev_args.
>>
>> Well, I do remember that discussion and still continued with it because 
>> 1) I am not done with that dev_args changes and 2) I think this is more 
>> non-intrusive as this is specific to DPAA without need for expanding it 
>> towards dev_args (and impacting application arg list).
>> You think this is no-go? If so, I will fix this.
> 
> Proving argument looks more clear to me, it is more visible, and for
> example if multiple process will be run, environment variables can be
> confusing.
> 
> But this is not no-go, I would like to hear other comments. Also I
> recognized that mlx and ark drivers are also using this.
> 
> But however this is implemented, this should be clearly documented,
> right now this is a hidden config.

Agree, I will fix the documentation and add information about this.

> 
> <...>
>>>> +uint16_t dpaa_eth_tx_drop_all(void *q  __rte_unused,
>>>> +			      struct rte_mbuf **bufs __rte_unused,
>>>> +		uint16_t nb_bufs __rte_unused)
>>>> +{
>>>> +	PMD_TX_LOG(DEBUG, "Drop all packets");
>>>
>>> Should mbufs freed here?
>>>
>>>> +
>>>> +	/* Drop all incoming packets. No need to free packets here
>>>> +	 * because the rte_eth f/w frees up the packets through tx_buffer
>>>> +	 * callback in case this functions returns count less than nb_bufs
>>>> +	 */
>>
>> Ah, actually I was banking on logic that in case a driver doesn't 
>> release memory, the API caller (on getting less than nb_bufs) would do 
>> that. This is case for stopped interface.
>>
>> But, I agree, this is dirty fix. I will change this.
> 
> I missed your logic here indeed, this looks a valid option too, its your
> call.
> 
>>
>>>> +	return 0;
>>>> +}
>>>
>>> <...>
>>>
>>>
>>
> 
>