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

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

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?

> 
> Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@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.

> +
> +/*#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? :)

> +#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?

<...>

> +	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?

<...>

> +#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?

<...>

> +/**
> + @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.