DPDK patches and discussions
 help / color / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Hemant Agrawal <hemant.agrawal@nxp.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Sachin Saxena <sachin.saxena@nxp.com>
Subject: Re: [dpdk-dev] [PATCH 15/37] net/dpaa: add support for fmlib in dpdk
Date: Wed, 1 Jul 2020 08:35:09 +0100
Message-ID: <61fe3b69-1870-ad80-6315-9aff8d606519@intel.com> (raw)
In-Reply-To: <AM6PR04MB4456678D568C643C554A8CC8896C0@AM6PR04MB4456.eurprd04.prod.outlook.com>

On 7/1/2020 5:18 AM, Hemant Agrawal wrote:
> 
> On 30-Jun-20 10:30 PM, Ferruh Yigit wrote:
>> 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?
> 
> Yes, fmlib means FMAN config library. It is a base code used by many projects, we have integrated it into DPDK.

Thanks, can you please put this into commit log in next version? And even FMAN
is not familiar to me, so even a little more details can help.

>>
>>
>>>
>>> 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.
> 
> ok
>>
>>
>>> +
>>> +/*#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? :)
> 
> The same code goes to kernel as well, so for kernel portions they are using printk

For DPDK this is dead code, can it be possible to strip kernel related ones in
the DPDK code? With some kind of OS layer perhaps (that is what Intel does).

>>
>>
>>> +#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?
>>
>> <...>
> 
> We have tried to correct few of these. But this code is an independent base library used by multiple projects
> 
> If we tried to align it with dpdk style, it will become a maintenance issue for us to get any future upgrades.
> 
> What you suggest?

I agree that it is more practical to avoid maintenance cost for style issues in
this kind of shared code.
But please at least ensure the style is consistent in the file/module with its
initial style.

>>
>>
>>> +	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?
> 
> Same as explained above.  If we make manual changes in this code, it's future upgrade from base library releases will become a maintenance issue for us.
>>
>>
>> <...>
>>
>>> +#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?
> 
> ok
>>
>>
>> <...>
>>
>>> +/**
>>> + @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.


  reply index

Thread overview: 81+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-27 13:22 [dpdk-dev] [PATCH 00/37] NXP DPAAx enhancements Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 01/37] bus/fslmc: fix getting the FD error Hemant Agrawal
2020-05-27 18:07   ` Akhil Goyal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 02/37] net/dpaa: fix fd offset data type Hemant Agrawal
2020-05-27 18:08   ` Akhil Goyal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 03/37] net/dpaa2: enable timestamp for Rx offload case as well Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 04/37] bus/fslmc: combine thread specific variables Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 05/37] bus/fslmc: rework portal allocation to a per thread basis Hemant Agrawal
2020-07-01  7:23   ` Ferruh Yigit
2020-05-27 13:22 ` [dpdk-dev] [PATCH 06/37] bus/fslmc: support handle portal alloc failure Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 07/37] bus/fslmc: support portal migration Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 08/37] bus/fslmc: rename the cinh read functions used for ls1088 Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 09/37] net/dpaa: enable Tx queue taildrop Hemant Agrawal
2020-05-27 13:22 ` [dpdk-dev] [PATCH 10/37] net/dpaa: add 2.5G support Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 11/37] net/dpaa: update process specific device info Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 12/37] drivers: optimize thread local storage for dpaa Hemant Agrawal
2020-05-27 18:13   ` Akhil Goyal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 13/37] bus/dpaa: enable link state interrupt Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 14/37] bus/dpaa: enable set link status Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 15/37] net/dpaa: add support for fmlib in dpdk Hemant Agrawal
2020-06-30 17:00   ` Ferruh Yigit
2020-07-01  4:18     ` Hemant Agrawal
2020-07-01  7:35       ` Ferruh Yigit [this message]
2020-05-27 13:23 ` [dpdk-dev] [PATCH 16/37] net/dpaa: add VSP support in FMLIB Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 17/37] net/dpaa: add support for fmcless mode Hemant Agrawal
2020-06-30 17:01   ` Ferruh Yigit
2020-07-01  4:04     ` Hemant Agrawal
2020-07-01  7:37       ` Ferruh Yigit
2020-05-27 13:23 ` [dpdk-dev] [PATCH 18/37] bus/dpaa: add shared MAC support Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 19/37] bus/dpaa: add Virtual Storage Profile port init Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 20/37] net/dpaa: add support for Virtual Storage Profile Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 21/37] net/dpaa: add fmc parser support for VSP Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 22/37] net/dpaa: add RSS update func with FMCless Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 23/37] net/dpaa2: dynamic flow control support Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 24/37] net/dpaa2: key extracts of flow API Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 25/37] net/dpaa2: sanity check for flow extracts Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 26/37] net/dpaa2: free flow rule memory Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 27/37] net/dpaa2: flow QoS or FS table entry indexing Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 28/37] net/dpaa2: define the size of table entry Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 29/37] net/dpaa2: log of flow extracts and rules Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 30/37] net/dpaa2: discrimination between IPv4 and IPv6 Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 31/37] net/dpaa2: distribution size set on multiple TCs Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 32/37] net/dpaa2: index of queue action for flow Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 33/37] net/dpaa2: flow data sanity check Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 34/37] net/dpaa2: flow API QoS setup follows FS setup Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 35/37] net/dpaa2: flow API FS miss action configuration Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 36/37] net/dpaa2: configure per class distribution size Hemant Agrawal
2020-05-27 13:23 ` [dpdk-dev] [PATCH 37/37] net/dpaa2: support raw flow classification Hemant Agrawal
2020-06-30 17:01 ` [dpdk-dev] [PATCH 00/37] NXP DPAAx enhancements Ferruh Yigit
2020-07-01  4:08   ` Hemant Agrawal
2020-07-07  9:22 ` [dpdk-dev] [PATCH v2 00/29] " Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 01/29] bus/fslmc: fix getting the FD error Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 02/29] net/dpaa: fix fd offset data type Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 03/29] net/dpaa2: enable timestamp for Rx offload case as well Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 04/29] bus/fslmc: combine thread specific variables Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 05/29] bus/fslmc: rework portal allocation to a per thread basis Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 06/29] bus/fslmc: support handle portal alloc failure Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 07/29] bus/fslmc: support portal migration Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 08/29] bus/fslmc: rename the cinh read functions used for ls1088 Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 09/29] net/dpaa: enable Tx queue taildrop Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 10/29] net/dpaa: add 2.5G support Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 11/29] net/dpaa: update process specific device info Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 12/29] drivers: optimize thread local storage for dpaa Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 13/29] bus/dpaa: enable link state interrupt Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 14/29] bus/dpaa: enable set link status Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 15/29] net/dpaa2: support dynamic flow control Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 16/29] net/dpaa2: support key extracts of flow API Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 17/29] net/dpaa2: add sanity check for flow extracts Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 18/29] net/dpaa2: free flow rule memory Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 19/29] net/dpaa2: support QoS or FS table entry indexing Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 20/29] net/dpaa2: define the size of table entry Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 21/29] net/dpaa2: add logging of flow extracts and rules Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 22/29] net/dpaa2: support iscrimination between IPv4 and IPv6 Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 23/29] net/dpaa2: support distribution size set on multiple TCs Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 24/29] net/dpaa2: support ndex of queue action for flow Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 25/29] net/dpaa2: add flow data sanity check Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 26/29] net/dpaa2: modify flow API QoS setup to follow FS setup Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 27/29] net/dpaa2: support flow API FS miss action configuration Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 28/29] net/dpaa2: configure per class distribution size Hemant Agrawal
2020-07-07  9:22   ` [dpdk-dev] [PATCH v2 29/29] net/dpaa2: support raw flow classification Hemant Agrawal
2020-07-09  1:54   ` [dpdk-dev] [PATCH v2 00/29] NXP DPAAx enhancements Ferruh Yigit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=61fe3b69-1870-ad80-6315-9aff8d606519@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=dev@dpdk.org \
    --cc=hemant.agrawal@nxp.com \
    --cc=sachin.saxena@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox