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

[Hemant Agrawal <hemant.agrawal@nxp.com>]

HI Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, August 26, 2020 8:22 PM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>; dev@dpdk.org
> Subject: Re: [PATCH v5 1/8] net/dpaa: add support for fmlib in dpdk
> 
> On 8/26/2020 2:54 PM, Ferruh Yigit wrote:
> > 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.
> 
> 
> Hi Hemant,
> 
> This patch is missing some serious documentation, fmlib support depends on
> some kernel module(s) which doesn't mention anywhere as dependency,
> and what are those modules, where can we find them, what is the licensing
> of them etc all missing.

[Hemant] ok
> 
> Also there is some code in the patchset that assumes there is a generated
> binary in "/tmp/fmc.bin", the documentation of how this binary generated,
> where can we find tool to generate binary is missing.
> Should we rely on a binary should exist in a tmp folder is something else..
> 
> There is some licensing concerns, some code is GPL-2 dual licensed, can't
> they licensed as BSD-3?
> 
[Hemant] We can change the licensing of this code as we are anyway changing the files. 

> Last thing is there is still some clutter in the code from linux, and there are
> still formatting/syntax issues...

[Hemant] ok
> 
> >>
> >> 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'.
> >
[Hemant] ok

> > 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.
[Hemant]  We have changed the Hungarian notation to all small case. 
Will you please help me with one example here.  How you want to see it?


> >
> > <...>
> >
> >> +#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?
> >
[Hemant] We were trying to avoid changes in common files with Linux.