HI Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Wednesday, August 26, 2020 8:22 PM
> To: Hemant Agrawal <hemant.agra...@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.sax...@nxp.com>
> >> Signed-off-by: Hemant Agrawal <hemant.agra...@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.
