Re: [dpdk-dev] [PATCH v5 02/15] ethdev: add support for hairpin queue

[Ori Kam <orika@mellanox.com>]

Hi Andrew, Thomas

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 24, 2019 6:35 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; Ori Kam <orika@mellanox.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; jingjing.wu@intel.com;
> stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH v5 02/15] ethdev: add support for hairpin queue
> 
> 24/10/2019 17:30, Andrew Rybchenko:
> > On 10/24/19 6:17 PM, Thomas Monjalon wrote:
> > > 24/10/2019 16:47, Andrew Rybchenko:
> > >> On 10/24/19 11:29 AM, Ori Kam wrote:
> > >>> Hi Andrew,
> > >>>
> > >>> When writing the new function I thought about using bool, but
> > >>> I decided against it for the following reasons:
> > >>> 1. There is no use of bool any where in the code, and there is not special
> reason to add it now.
> > >> rte_ethdev.c includes stdbool.h and uses bool
> > >>
> > >>> 2. Other functions of this kind already returns int. for example
> (rte_eth_dev_is_valid_port / rte_eth_is_valid_owner_id)
> > > I agree with Ori here for 2 reasons:
> > > 1. It is better to be consistent in the API
> > > 2. I remember having some issues with some drivers when introducing
> stdbool in the API.
> > >
> > > I think it may be nice to convert all such API to bool in one patch,
> > > and check if there are some remaining issues with bool usage in drivers or
> with PPC.
> > > But I suggest to do such API change in DPDK 20.11.
> >
> > OK, no problem. Does it prevent to avoid comparison == 1? Just to
> > avoid changes in these lines in the future.
> 
> Yes probably better to avoid explicit comparison, but prefer boolean operator
> (!).
> 
> 

Thomas I understand your comments but from Andrew comment on my V2-01 patch:
"
>>> +	ret = (*dev->dev_ops->rx_hairpin_queue_setup)(dev, rx_queue_id,
>>> +						      nb_rx_desc, conf);
>>> +	if (!ret)
>> Please, compare with 0
>>
> Will do, but again just for my knowledge why?

https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html%23function-calls&amp;data=02%7C01%7Corika%40mellanox.com%7C022cd953964f4a20d50508d7508a259f%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637066426584029629&amp;sdata=G8ZxEWFFsv1kLWc6L7sKT8O6CSiBcj5ZuwQqmK0Q6nY%3D&amp;reserved=0

"
I don't see any relevant info in the link, but maybe I'm missing something .
What are the rules? 
Thomas also keep in mind that in most cases the condition that is tested is the positive, meaning  it will look something like this:
if (rte_eth_dev_is_rx_hairpin_queue(dev, queue_id)) {     
        rte_errno = EINVAL;                               
        return NULL;                                      
}                                       

What do you think?