Re: [dpdk-dev] [PATCH v5 1/8] net/dpaa: add support for fmlib in dpdk

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

On 8/13/2020 7:01 PM, Hemant Agrawal wrote:
> DPAA platorm MAC interface is known as FMAN i.e. Frame Manager.
> There are two ways to control it.
> 1. Statically configure the queues and classification rules before the
> start of the application using FMC tool.
> 2. Dynamically configure it within application by making API calls of
> fmlib.
> 
> The fmlib or Frame Manager library provides an API on top of the
> Frame Manager driver ioctl calls, that provides a user space application
> with a simple way to configure driver parameters and PCD
> (parse - classify - distribute) rules.
> 
> This patch integrates the base fmlib so that various queue config, RSS
> and classification related features can be supported on DPAA platform.
> 
> Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> v4: adapting to DPDK style
> v5: removing shared compilation issue and spelling errors
> 
>  drivers/net/dpaa/Makefile                 |    4 +-

For this release, v20.11, all Makefile changes can be dropped, since make build
system will be removed soon.

<...>

> +
> +/* #define FM_LIB_DBG */

What about adding this as a config option, to enable & disable the debug,
instead of having as a commented code?
And overall why not use this as runtime configurable logging, instead of compile
time?

> +
> +#if defined(FM_LIB_DBG)
> +	#define _fml_dbg(format, arg...) \
> +	printf("fmlib [%s:%u] - " format, \
> +		__func__, __LINE__, ##arg)

Please use DPDK logging, instead of 'printf'.

Btw, this file has dual license, we have dual license for the code that is
shared between kernel and DPDK, if this is shared with kernel how can have the
'printf'?
Should debug related macros moved to some other header?

<...>

> +
> +uint32_t fm_pcd_disable(t_handle h_fm_pcd)
> +{
> +	t_device	*p_dev = (t_device *)h_fm_pcd;


Since this is not shared but DPDK only code (that was the outcome of the
techboard discussion), can you pleae follow the DPDK coding convention to the
file. There are multiple samples doesn't match the convention, I won't comment
on other instances.

<...>

> +#if defined(CONFIG_COMPAT)
> +#define FM_PCD_IOC_PRS_LOAD_SW_COMPAT \
> +	_IOW(FM_IOC_TYPE_BASE, FM_PCD_IOC_NUM(3), \
> +	     ioc_compat_fm_pcd_prs_sw_params_t)
> +#endif

'CONFIG_COMPAT' is for Linux, do we need it for the DPDK copy?