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.sax...@nxp.com>
>>> Signed-off-by: Hemant Agrawal <hemant.agra...@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.
