Re: [dpdk-dev] [PATCH v4 07/11] net/hinic/base: add various headers

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 6/12/2019 3:24 PM, Xuanziyang (William, Chip Application Design Logic and
Hardware Development Dept IT_Products & Solutions) wrote:
> 
>> On 6/6/2019 12:06 PM, Ziyang Xuan wrote:
>>> Add various headers that define mgmt commands, cmdq commands, rx
>> data
>>> structures, tx data structures and basic defines for use in the code.
>>>
>>> Signed-off-by: Ziyang Xuan <xuanziyang2@huawei.com>
>>
>> <...>
>>
>>> +#define PMD_DRV_LOG(level, fmt, args...) \
>>> +	rte_log(RTE_LOG_ ## level, hinic_logtype, \
>>> +		HINIC_DRIVER_NAME": " fmt "\n", ##args)
>>> +
>>> +#define HINIC_ASSERT_EN
>>> +
>>> +#ifdef HINIC_ASSERT_EN
>>> +#define HINIC_ASSERT(exp)	\
>>> +	do {			\
>>> +		if (!(exp)) {	\
>>> +			rte_panic("line%d\tassert \"" #exp "\" failed\n", \
>>> +				  __LINE__);	\
>>> +		}		\
>>> +	} while (0)
>>> +#else
>>> +#define HINIC_ASSERT(exp)	do {} while (0)
>>> +#endif
>>
>> So you are enabling asserting by default? Which can cause "rte_panic()" ?
>>
>> Please make sure asserting is disabled by default, and please tie this to the
>> "CONFIG_RTE_ENABLE_ASSERT" config option. So it that option is disabled
>> hinic also should disable the assertions.
> 
> I checked the places where use rte_panic, most of them can use code logic to guarantee correctness. And I have referenced other PMDs like mlx5, they use
> rte_panic directly but use custom encapsulation, so I delete custom encapsulation  above and the most rte_panic usage, and use directly like mlx5.
> 
> Is it OK?
> 

Also does it make enable 'HINIC_ASSERT' when global 'CONFIG_RTE_ENABLE_ASSERT'
config enabled?