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

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 <redacted>
Signed-off-by: Hemant Agrawal <redacted>

<...>

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