[dpdk-dev] [PATCH 15/37] net/dpaa: add support for fmlib in dpdk

[Ferruh Yigit]

On 7/1/2020 5:18 AM, Hemant Agrawal wrote:
> 
> On 30-Jun-20 10:30 PM, Ferruh Yigit wrote:
>> On 5/27/2020 2:23 PM, Hemant Agrawal wrote:
>>>  This library is required for configuring FMAN for
>>>  various flow configurations.
>>
>> This is a big patch with new files, looks like a new base code drop.
>> Can you please give more explanation on the patch and what 'fmlib' is?
> 
> Yes, fmlib means FMAN config library. It is a base code used by many projects, we have integrated it into DPDK.

Thanks, can you please put this into commit log in next version? And even FMAN
is not familiar to me, so even a little more details can help.

>>
>>
>>>
>>> Signed-off-by: Sachin Saxena <sachin.saxena at nxp.com>
>>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>>
>> <...>
>>
>>> +#if defined(FM_LIB_DBG)
>>> +	#define _fml_dbg(format, arg...) \
>>> +	printf("fmlib [%s:%u] - " format, \
>>> +		__func__, __LINE__, ##arg)
>>> +#else
>>> +	#define _fml_dbg(arg...)
>>> +#endif
>>
>> Shouldn't use 'printf' directly, this prevents using dynamic logging and our log
>> APIs. Please use a registered logtype instead.
> 
> ok
>>
>>
>>> +
>>> +/*#define FM_IOCTL_DBG*/
>>> +
>>> +#if defined(FM_IOCTL_DBG)
>>> +	#define _fm_ioctl_dbg(format, arg...) \
>>> +	printk("fm ioctl [%s:%u](cpu:%u) - " format, \
>>> +		__func__, __LINE__, smp_processor_id(), ##arg)
>>
>> printk? :)
> 
> The same code goes to kernel as well, so for kernel portions they are using printk

For DPDK this is dead code, can it be possible to strip kernel related ones in
the DPDK code? With some kind of OS layer perhaps (that is what Intel does).

>>
>>
>>> +#else
>>> +#   define _fm_ioctl_dbg(arg...)
>>> +#endif
>>> +
>>> +/**
>>> + @Group	lnx_ioctl_ncsw_grp	NetCommSw Linux User-Space (IOCTL) API
>>> + @{
>>> +*/
>>> +
>>> +#define NCSW_IOC_TYPE_BASE	0xe0
>>> +	/**< defines the IOCTL type for all the NCSW Linux module commands */
>>> +
>>> +/**
>>> + @Group	lnx_usr_FM_grp Frame Manager API
>>> +
>>> + @Description   FM API functions, definitions and enums.
>>> +
>>> + @{
>>> +*/
>>
>> There are lots of checkpatch warning in the block comment syntax, about missing
>> " * " on each line.
>> Other base dpaa/dpaa2 base code seems have it in the block comments, if this
>> won't create a maintance problem, what do you think to fix the syntax on comments?
>>
>> <...>
> 
> We have tried to correct few of these. But this code is an independent base library used by multiple projects
> 
> If we tried to align it with dpdk style, it will become a maintenance issue for us to get any future upgrades.
> 
> What you suggest?

I agree that it is more practical to avoid maintenance cost for style issues in
this kind of shared code.
But please at least ensure the style is consistent in the file/module with its
initial style.

>>
>>
>>> +	e_IOC_FM_PCD_PRS_COUNTERS_SHIM_PARSE_RESULT_RETURNED_WITH_ERR,
>>> +	/**< Parser counter - counts the number of times SHIM parse result is returned with errors. */
>>> +	e_IOC_FM_PCD_PRS_COUNTERS_SOFT_PRS_CYCLES,
>>> +	/**< Parser counter - counts the number of cycles spent executing soft parser instruction (including stall cycles). */
>>> +	e_IOC_FM_PCD_PRS_COUNTERS_SOFT_PRS_STALL_CYCLES,
>>> +	/**< Parser counter - counts the number of cycles stalled waiting for parser internal memory reads while executing soft parser instruction. */
>>
>> Can you please break long lines?
> 
> Same as explained above.  If we make manual changes in this code, it's future upgrade from base library releases will become a maintenance issue for us.
>>
>>
>> <...>
>>
>>> +#if 0
>>> +TODO: unused IOCTL
>>> +/**
>>> + @Function	FM_PCD_ModifyCounter
>>> +
>>> + @Description   Writes a value to an enabled counter. Use "0" to reset the counter.
>>> +
>>> + @Param[in]	ioc_fm_pcd_counters_params_t - The requested counter parameters.
>>> +
>>> + @Return	0 on success; Error code otherwise.
>>> +*/
>>> +#define FM_PCD_IOC_MODIFY_COUNTER   _IOW(FM_IOC_TYPE_BASE, FM_PCD_IOC_NUM(10), ioc_fm_pcd_counters_params_t)
>>> +#define FM_PCD_IOC_SET_COUNTER	FM_PCD_IOC_MODIFY_COUNTER
>>> +#endif
>>
>> Can you please remove dead code?
> 
> ok
>>
>>
>> <...>
>>
>>> +/**
>>> + @Description   Enumeration type for selecting the policer profile packet frame length selector
>>> +*/
>>> +typedef enum ioc_fm_pcd_plcr_frame_length_select {
>>> +  e_IOC_FM_PCD_PLCR_L2_FRM_LEN,	/**< L2 frame length */
>>> +  e_IOC_FM_PCD_PLCR_L3_FRM_LEN,	/**< L3 frame length */
>>> +  e_IOC_FM_PCD_PLCR_L4_FRM_LEN,	/**< L4 frame length */
>>> +  e_IOC_FM_PCD_PLCR_FULL_FRM_LEN	/**< Full frame length */
>>> +} ioc_fm_pcd_plcr_frame_length_select;
>>> +
>>> +/**
>>> + @Description   Enumeration type for selecting roll-back frame
>>> +*/
>>> +typedef enum ioc_fm_pcd_plcr_roll_back_frame_select {
>>> +  e_IOC_FM_PCD_PLCR_ROLLBACK_L2_FRM_LEN,	/**< Rollback L2 frame length */
>>> +  e_IOC_FM_PCD_PLCR_ROLLBACK_FULL_FRM_LEN   /**< Rollback Full frame length */
>>> +} ioc_fm_pcd_plcr_roll_back_frame_select;
>>
>> Please fix the leading whitespace for above two enums.