From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DAEA343B3C; Wed, 14 Feb 2024 18:21:17 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9828942E1F; Wed, 14 Feb 2024 18:21:17 +0100 (CET) Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.17]) by mails.dpdk.org (Postfix) with ESMTP id 7006840697 for ; Wed, 14 Feb 2024 18:21:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1707931276; x=1739467276; h=message-id:date:subject:to:cc:references:from: in-reply-to:mime-version; bh=F8k/IVBq73b8XN/nRHiJYLYBsQIx0473/xhB/U2qwEQ=; b=j2iEGWiPWCx3XjtSRe1LD34PPO5R0PywXDU10cmq98p0ADQZX4vE92aV Tajp/9/8P8t02SLlrvKABF4cEcBlxB7i01YMpxmWh+03kTW3k8oqTEVib WdUtE8zT2rrKNZ2o1UlPskBPA1un7TxOf2DmKvwPeUoay1u4NvLRKCk6g pqqXK8JBuhsZhFyYepRlgNOM4x5Qcp1GT+MAP+q0BESB27HzhVPD94I/K aDW1r4RDuOBH/b1zYqrXt8SyeIlvQy8BPd7TvDu1I1tYRkC1V0hYGT6dw b6femU2cYaqPTh0S3XRIXaau46Bw/t9Jiz0M6ApYs+j1o1BasM7IlIHVC w==; X-IronPort-AV: E=McAfee;i="6600,9927,10984"; a="2128248" X-IronPort-AV: E=Sophos;i="6.06,160,1705392000"; d="scan'208,217";a="2128248" Received: from orviesa009.jf.intel.com ([10.64.159.149]) by orvoesa109.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Feb 2024 09:21:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,160,1705392000"; d="scan'208,217";a="3255195" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by orviesa009.jf.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 14 Feb 2024 09:21:14 -0800 Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Wed, 14 Feb 2024 09:21:13 -0800 Received: from fmsedg601.ED.cps.intel.com (10.1.192.135) by fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35 via Frontend Transport; Wed, 14 Feb 2024 09:21:13 -0800 Received: from NAM04-BN8-obe.outbound.protection.outlook.com (104.47.74.40) by edgegateway.intel.com (192.55.55.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Wed, 14 Feb 2024 09:21:10 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=lKhRW2AED/d/yfOygZ9o5UINywO4FJIAnxtm+YrCEpfbbKdKAvfZzjZD6ei0fN0arpBAbQ4g0YVJuFBdKYu6fSXm7xFRUzSu5RjCILCZyL+ZclCJeDItLNsKHTnNxcyMKxRuV7iuB+jgynU+/PZ2UFJ/5kBEYdRZJTqV3kA7QmdgPF6JmgAoijLbRQdUIv0THhj2NigoujuMI886IVpTF1m0kHWGvKlJ/pnzUsZdKKWj2VuNj8PKdqCzuOaazBhbYh7nXiHLHVz43eCY5aaCb7m32p/jOsfrx63ezKxx6sz3xvkLvNAh6wQESLwzYixJy83aWjFCG0GM559TllQIcw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=l0KbODR69APJl3RX4Ok51jgCyqUkU3F2sfGZe89DwWE=; b=e21qtnX9ChBlKpza7WjOTXJ1oABkOjs+65mtinrEjoiV02VFxh850ntcwq9itiWgNlARtW9wFFpEyKnympQE4Gn3l3psRGQ40SfLE/5oJZ+kEFkL2VPqebU+EX7s+yJ30z47/qbMvanevkP1r4cnf/+G+8hs1jUqs2T1yIvKHRd9izJBhScRCP22unZfujKPE0sHvg+9PdcsyBUNvRH+q3qAdqoekLeZUJODIxfYH5Ry9Rz44xZrXUQk1STOelQDgo3iybAMvCuqZjT0xifp6tEiK9SYA8f6OP6Sd/ornI1X82ORLsDm9Gg0033Z2IjdFR0t+aenuGbM3KqUJBEZFw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; Received: from SJ0PR11MB5772.namprd11.prod.outlook.com (2603:10b6:a03:422::8) by DM6PR11MB4562.namprd11.prod.outlook.com (2603:10b6:5:2a8::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7270.39; Wed, 14 Feb 2024 17:21:06 +0000 Received: from SJ0PR11MB5772.namprd11.prod.outlook.com ([fe80::fc4:cc9e:c910:f4b9]) by SJ0PR11MB5772.namprd11.prod.outlook.com ([fe80::fc4:cc9e:c910:f4b9%2]) with mapi id 15.20.7270.036; Wed, 14 Feb 2024 17:21:06 +0000 Content-Type: multipart/alternative; boundary="------------PVPUTNGAjORIU8SWZwtb8Mpc" Message-ID: <894b32a1-00b1-4ece-8b5a-c2f1094fc05c@intel.com> Date: Wed, 14 Feb 2024 17:21:01 +0000 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] net/cpfl: add TDI to flow engine Content-Language: en-US To: , , , CC: References: <20231222100837.260493-2-wenjing.qiao@intel.com> <20240105081649.530384-3-wenjing.qiao@intel.com> From: "Medvedkin, Vladimir" In-Reply-To: <20240105081649.530384-3-wenjing.qiao@intel.com> X-ClientProxiedBy: DUZPR01CA0154.eurprd01.prod.exchangelabs.com (2603:10a6:10:4bd::24) To SJ0PR11MB5772.namprd11.prod.outlook.com (2603:10b6:a03:422::8) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: SJ0PR11MB5772:EE_|DM6PR11MB4562:EE_ X-MS-Office365-Filtering-Correlation-Id: 67b50ea3-dd84-4a52-840e-08dc2d8153a6 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: tPl0tRhJkt9EVYfPEMoEiH9rMfI/JX9pkYg69Nff8Yn1XgUJaa8Y7liCFHMteRgBv3s5bGrSXyxphvgnlxM3oUov06AltImSNWCYF9I0a5tCilhdxyyoaYDqHzoYFAmtZa2r+MnMdM7zzOcyaKxcach7PzkHrDfVbZh3q+vmXOeYx7PYfyCWiFvxfer6R0yPqHlI0128AzjJNGXHKqroQwdiu1iFIb06GEuitnPvIsy8I6J49lGaby2tE+kkaSgFb9e2Yaco/M0a7nhOGl0lT2SBqtVFeBAQt/agBG/2s3ICmRlBVI7XpwQJ1rKVI9puKGlur1JloICQUaA2AfJYe8e7dmrejGzv6ELeJ5WEnLo+LaiMKA39Hx4FXCM1t8NbGSFyvU+wmI+a6h9KHCg3J8aEn9jyED9OLMAf+kKAoIAr5P23Rdc0AscKGoro/taUOCfBsUpwA/VOl5p0DdG/HUpU6ShJaxSR9rGWb/ZhTBqXlavfIRyUm+WBfcojlCQNTBY0lUMuCDuOJgcg8krZtkRoe+x0FKDBJWZOtKp3EO/mgeeK+0lzbmdaFUhVzg3x X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:SJ0PR11MB5772.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230031)(346002)(136003)(39860400002)(376002)(366004)(396003)(230922051799003)(186009)(1800799012)(64100799003)(451199024)(8676002)(4326008)(8936002)(5660300002)(30864003)(2906002)(66946007)(36756003)(2616005)(83380400001)(26005)(86362001)(82960400001)(38100700002)(31696002)(316002)(33964004)(6506007)(66476007)(6636002)(66556008)(53546011)(6666004)(6512007)(6486002)(478600001)(31686004)(41300700001)(579004); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OXdFRW83QTluSGE5RGNGYzBMTUpldVovaXFNZWd1UjA2aTZ0ck1PY2RlYkNa?= =?utf-8?B?RzcybjdZcGRQYWVXZFl6TjEwQkRnVC90SmczWWpGdkdGZHpvdDl0UHJFaktx?= =?utf-8?B?NkxKU3o2aE5lMEEyQzRoOS9Kb1BXK0p6Rkptdzg1b0lHSk5rRHA1QjlPS2RG?= =?utf-8?B?L0l1elBQN3JUZC9UcXhUOTBoNDRPTnprNGpOdW9IckJ0SnZJTTg0TDNzL2Ji?= =?utf-8?B?ci8wY0k2S1QydjJXbEpadzVYUWhkOWxtcjJ4SGRCTHA1NDRDYWN0RmtDcFNU?= =?utf-8?B?bFZOZmRRWjZhcjFYMzQ2QTk5eFBadkRZbzRPWGhuOVA4OUg0VTJFamlIQUw1?= =?utf-8?B?NnVBck5kNG5aenFLbS9VRmRjY25RcHNrKzdiMDBHTmswYkxNWmdwbDF5V0ZC?= =?utf-8?B?SnFHZE94bndnWW5Vcm9pa2kvNlI1RDVkbDZRSUhVaG9scVErdXA5NDl0a2xx?= =?utf-8?B?ZXZCWWo1Q3lSSlZ6UDhyemdhRmFxQk9jMVFyT21MUXZoNFp6WEJ2TUNaRFRz?= =?utf-8?B?dXNVc0dabnB4ZHlBalFDbHlJWVA3T2tYOUNWTDVwc29jTFJ6SGdrOThzbUxX?= =?utf-8?B?Q01WRXVHK05HTzZDdnZaWVZISmNiY1Z5YmtGbEwrRW51REcraVpVejJqRzZ5?= =?utf-8?B?U3QwVG84eHB6QnpXRkYzQ0ZsSU9LY1Q5TThOQ3BCUXBjMGhDN2RUeWxyanhG?= =?utf-8?B?UjMwY2hYTFV2R0psdlBoMW9QM2tOUENOL1VGN0hHQnd1eUY5MTczSFJNNklS?= =?utf-8?B?cGFKbjd6ckI5VDcvRUVqeGVBTytPay9wYndkZkc2czdKZlRWUlRjS3FmeHF0?= =?utf-8?B?cGVpUHRpS2lEaVhPdkliVGpKdWhmdkxvUE9JY3Q4aG9YRjcyRVdwSXE2aSsx?= =?utf-8?B?TXNWWndxbHc5U01FRUpJOXdRd25FKzcwK0lndG1CbmhKY3VISWV1c2w3Yzho?= =?utf-8?B?Z3dFRHVmT1FyRkVET1BxOWlwQVU1RjJXK2tIOUdmYXdZSVZhaXBOb0R4aHNT?= =?utf-8?B?Y0k2TGxjR2FBdWVlaGhZSjdDd1dTOUFtWTV1YXlkN1p4ZzhydHhPNWxLWGZL?= =?utf-8?B?MzgrS0p6bnUxRG9aaXNhWnVBdm1sNVNvVXdLRlBKOE8wRUhXWk4vR0lqZHhx?= =?utf-8?B?anBCTmlKbVdPSVVmbURiQVNuOVpCMTJwM1FmT1czMU81ZkVabVBvYnIvWE1k?= =?utf-8?B?TG9SczZwcHU4aVZveE9hbzBCVHEzM0t6RGpCTU1QanFMSEp3R2cwcFk0Uktv?= =?utf-8?B?aWFnT2dEQktsall6UlA3dFU2YjRpSXBsZ1V0QWxzUDQzYXhFUjFPellmRG9h?= =?utf-8?B?VWVxRWdjOWZkVXZOTGlFSUEvdjFLbzRBQ3NPTEtzc2E2MTdESmsraUdZQXdJ?= =?utf-8?B?aHNmeWtJU2tacWZqd0VYYUxIaGFnVWEyQ3l4anp6cXpTd0Qxb0tJdUpMZm95?= =?utf-8?B?TW5qYkNoMk43YzFFV1lZNGc4UlRreUpwSFpWZDhrd2VSbldSdVZIL1F2dkFO?= =?utf-8?B?RGIzaGw2R1k0VkRUcUlSU3lkdC9JdDlNK21WVE9yTWJTT2lTNWVORHdEcTY1?= =?utf-8?B?b1o0cXBLOWViM3gvay9PcEpKcGpzQW9TTTl6Mkpua3pkMzI1dkoyWWhtYXIy?= =?utf-8?B?VVE2NXYzZnVpL2JveVVpTENvL1pRSnNqMCtFMlgyWE5nV1BIVGJidXZFL0p5?= =?utf-8?B?M2MrRytXS2hOMm9FUXh2em95dzJ1ZkRsZUtyQWNwbmFsM0wxU3Z5ZDd3SjFp?= =?utf-8?B?NWdzUy9pbVhhaTl4OE5RMHNWRUNObVYzeVMyMkNkaXdIMld2bjd5bHFpU1dC?= =?utf-8?B?M2JpeTFRQzAxYlhqcXlzMVh1enFXT0s3L2pIK1lSNUs5bE1vL2ZncmwwbU05?= =?utf-8?B?dlNmalZnNEM2dFExc1l6c1hBZmh0WVJqbGtTeGxFekJGYUJNQXVBVGM2TmRv?= =?utf-8?B?YTcyaEJPQ1EvRzFDbSt2QllzSjdaeHlSTndCZVZSb21qK2tidW9Gb2dDL09w?= =?utf-8?B?Sis5NndlT29xV0VoSTNERzE3ZUlpUW9aK2dVK3dwNUZ3M1BubFVoRmJ3eTUy?= =?utf-8?B?di9vZHIrM0t5djVJQVN5ZURPZWFwMllpS3JhN2l6RWEwK1M5L2ZFaEwvbzR6?= =?utf-8?B?cjNha3FqRlFUMW5vYWxYcXpIeGV5aDBOL3FGMHNPSnMxT3ZwMXFrVVdoYzEy?= =?utf-8?B?RUE9PQ==?= X-MS-Exchange-CrossTenant-Network-Message-Id: 67b50ea3-dd84-4a52-840e-08dc2d8153a6 X-MS-Exchange-CrossTenant-AuthSource: SJ0PR11MB5772.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2024 17:21:06.0488 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: XZfDzBUiEjFtWK348JrZbjnqTihpJh2w3nbIkrCA2WOVHbVXWkZTYmK7L/PQRJ1XPPtkZswkkSJoeKlY0vJuNn1DOKQZFwJk3qxnUnO3rg8= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4562 X-OriginatorOrg: intel.com X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --------------PVPUTNGAjORIU8SWZwtb8Mpc Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit Hi Wenjing, Please find comments inlined On 05/01/2024 08:16, wenjing.qiao@intel.com wrote: > From: Wenjing Qiao > > Add TDI implementation to a flow engine. > > Signed-off-by: Wenjing Qiao > --- > --- /dev/null > +++ b/drivers/net/cpfl/cpfl_tdi.c > @@ -0,0 +1,1282 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2023 Intel Corporation > + */ > +#include > +#include RTE includes should be after system includes > +#include > +#include > + > +#include "cpfl_actions.h" > +#include "cpfl_flow.h" > +#include "cpfl_fxp_rule.h" > +#include "cpfl_tdi.h" > +#include "cpfl_tdi_parser.h" > +#include "rte_branch_prediction.h" > +#include "rte_common.h" > +#include "rte_flow.h" Those includes should be together with other RTE includes and have angle brackets > + > +#define CPFL_NO_FWD 0 > +uint64_t cpfl_tdi_rule_cookie = CPFL_COOKIE_DEF; > + > +/*help function to do left shift on a byte array */ > +static void > +cpfl_tdi_shift_left(uint8_t *buf, int length, uint32_t shift_amount) > +{ > + uint32_t i; > + int j; > + uint32_t carry = 0; > + > + if (shift_amount == 0) > + return; > + > + for (i = 0; i < shift_amount; i += 8) { > + for (j = 0; j < length; j++) { > + uint32_t temp = (buf[j] << (shift_amount - i)) | carry; > + > + carry = (temp >> 8) & 0xff; > + buf[j] = temp & 0xff; > + } > + } > +} This function does not looks correct to me, at least in case when shift_amount is larger than 8. You can try something like: static void cpfl_tdi_shift_left(uint8_t *buf, int length, uint32_t shift_amount) {         int i;         int bit_shift = shift_amount & 0x7;         int byte_shift = shift_amount >> 3;         if (shift_amount >= (length * 8))                 return;         uint8_t prev = 0;         for (i = 0; i < length; i++) {                 uint8_t val = (buf[i] << bit_shift) | prev;                 prev = buf[i] >> (8 - bit_shift);                 buf[i] = val;         }         for (i = length - 1; i >= byte_shift; i--) {                 buf[i] = buf[i - byte_shift];         }         for(; i >= 0; i--)                 buf[i] = 0; } > + > +/* help function to init a mask array with bit_width */ > +static void > +cpfl_tdi_init_msk_buf(uint8_t *buf, int length, uint32_t bit_width) > +{ > + uint32_t i; > + > + memset(buf, 0, length); > + for (i = 0; i < bit_width; i++) { > + cpfl_tdi_shift_left(buf, length, 1); > + buf[0] += 1; > + } > +} It seems like this function sets bit_width bits into the buf starting from the LSB. If so I'd suggest to make it simpler and not use cpfl_tdi_shift_left(). Instead you can just write UINT8/16/32/64_MAX into the corresponding buffer and decrease bit_width accordingly if it is bigger than 8/16/32/64 and finally write (1 << bit_width) - 1. > + > +/* help function to OR a byte array with value and mask */ > +static void > +cpfl_tdi_or_buf(uint8_t *buf, int length, uint8_t *values, uint8_t *mask) > +{ > + int i; > + > + for (i = 0; i < length; i++) > + buf[i] = (values[i] & mask[i]) | (buf[i] & ~mask[i]); I guess "& ~mask[i]" is not necessary here if this function is really OR. > +} > + > +static void > +cpfl_tdi_pack_sem_entry(struct cpfl_tdi_rule_info *rinfo, > + struct cpfl_tdi_ma_hardware_block *hb, > + enum cpfl_tdi_table_entry_op op, > + struct idpf_dma_mem *dma, > + struct idpf_ctlq_msg *msg) > +{ > + union cpfl_rule_cfg_pkt_record *blob; > + struct cpfl_rule_cfg_data cfg = {0}; > + uint16_t cfg_ctrl; > + enum cpfl_ctlq_rule_cfg_opc opc = 0; > + const struct cpfl_tdi_table_key_obj *key = &rinfo->kobj; > + const struct cpfl_tdi_action_obj *action = &rinfo->aobj; > + > + blob = (void *)dma->va; Why not cast to (union cpfl_rule_cfg_pkt_record *)? > + memset(blob, 0, sizeof(*blob)); > + > + cfg_ctrl = CPFL_GET_MEV_SEM_RULE_CFG_CTRL(hb->profile[0], hb->sem.sub_profile, 0, 0); > + > +static int > +cpfl_tdi_rule_process(struct cpfl_itf *itf, > + struct idpf_ctlq_info *tx_cq, > + struct idpf_ctlq_info *rx_cq, > + struct cpfl_tdi_rule_info *rinfo, > + int rule_num, > + enum cpfl_tdi_table_entry_op op) > +{ > + const struct cpfl_tdi_table_key_obj *kobj; > + struct idpf_hw *hw = &itf->adapter->base.hw; > + struct cpfl_tdi_ma_hardware_block *hb; > + const struct cpfl_tdi_table *table; > + int ret = 0; > + > + if (rule_num == 0) > + return 0; Is it necessary to check this value? This function is called with nonzero value always. As a general question, do we need to check input parameters in all these static functions? It seems some of them are callbacks and their parameters are probably checked by API, but for others it may be the case. > + > + kobj = &rinfo->kobj; > + > + table = kobj->tnode->table; > +static int > +cpfl_tdi_fxp_rule_destroy(struct rte_eth_dev *dev, > + struct rte_flow *flow, > + struct rte_flow_error *error) > +{ > + struct cpfl_itf *itf = CPFL_DEV_TO_ITF(dev); > + struct cpfl_adapter_ext *ad = itf->adapter; > + struct cpfl_vport *vport; > + struct cpfl_repr *repr; > + struct cpfl_tdi_rule_info *rinfo = (struct cpfl_tdi_rule_info *)flow->rule; > + int ret = 0; > + uint32_t cpq_id = 0; > + > + if (itf->type == CPFL_ITF_TYPE_VPORT) { > + vport = (struct cpfl_vport *)itf; > + cpq_id = vport->base.devarg_id * 2; > + } else if (itf->type == CPFL_ITF_TYPE_REPRESENTOR) { > + repr = (struct cpfl_repr *)itf; > + cpq_id = ((repr->repr_id.pf_id + repr->repr_id.vf_id) & (CPFL_TX_CFGQ_NUM - 1)) * 2; > + } else { > + rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL, > + "fail to find correct control queue"); > + ret = -rte_errno; > + goto err; > + } > + > + ret = cpfl_tdi_rule_process(itf, ad->ctlqp[cpq_id], ad->ctlqp[cpq_id + 1], rinfo, 1, > + CPFL_TDI_TABLE_ENTRY_OP_DEL); > + if (ret) > + goto err; What's the purpose of this goto here? > + > +err: > + rte_free(rinfo); > + flow->rule = NULL; > + return ret; > +} > + > +void > +cpfl_tdi_free_table_list(struct cpfl_flow_parser *flow_parser) > +{ > + struct cpfl_tdi_table_node *node; Should this parameter be checked since this is non static function? > + > + while ((node = TAILQ_FIRST(&flow_parser->tdi_table_list))) { > + TAILQ_REMOVE(&flow_parser->tdi_table_list, node, next); > + rte_free(node); > + } > +} > + > + > +static int > +cpfl_tdi_table_key_create(struct rte_eth_dev *dev, > + uint32_t table_id, > + struct cpfl_tdi_table_node **table_node, > + struct cpfl_tdi_table_key_obj *kobj) > +{ > + int ret; > + > + if (!kobj) > + return -EINVAL; is it required for static function? in cpfl_tdi_parse_pattern_action() kobj is zero inited, so in can not be NULL here. Otherwise it would make sense to check all other arguments as well. I'd suggest to replace runtime if check with RTE_ASSERT on all input arguments instead. > + > + ret = cpfl_tdi_table_info_get(dev, table_id, table_node); > + if (ret != 0) > + return -EINVAL; > + > + ret = cpfl_tdi_table_key_node_init(dev, *table_node, kobj); > + if (ret != 0) > + return -EINVAL; > + > + return 0; > +} > + > +static int > +cpfl_tdi_table_key_field_info_get(__rte_unused struct rte_eth_dev *dev, why not remove this argument is it is unused? Same applied to several other functions > + struct cpfl_tdi_table_node *node, > + uint32_t field_id, > + struct cpfl_tdi_table_key_field_info **key_field_info) > +{ > +static int > +cpfl_tdi_table_key_field_set(struct rte_eth_dev *dev, > + struct cpfl_tdi_table_node *table_node, > + struct cpfl_tdi_table_key_obj *kobj, > + uint32_t field_id, > + const uint8_t *value, > + uint16_t size) > +{ > + struct cpfl_tdi_table_key_field_info *key_field_info; > + int ret; > + > + if (!kobj || !value) > + return -EINVAL; > + > + ret = cpfl_tdi_table_key_field_info_get(dev, table_node, field_id, &key_field_info); > + if (ret != 0) > + return -EINVAL; > + > + if (key_field_info->field->match_type != CPFL_TDI_MATCH_TYPE_EXACT) > + return -EINVAL; possible memory leak since cpfl_tdi_table_key_field_info_get() allocated memory for key_field_info. Also it would be better to name properly functions where memory allocation was done. > + > + if (unlikely(key_field_info->field->is_vsi)) { No need to use branch predictor hints since this is not a fast path > + struct cpfl_itf *src_itf; > + uint16_t vsi_id, port_id; > + uint8_t ids[2]; > + > + if (size != 1 && size != 2) /* input port id should be 8/16 bits */ > + return -EINVAL; > + port_id = size == 1 ? value[0] : value[0] << 8 | value[1]; please use parenthesis for easier reading > + src_itf = cpfl_get_itf_by_port_id(port_id); > + if (!src_itf) > + return -EINVAL; > + vsi_id = cpfl_get_vsi_id(src_itf); > + if (vsi_id == CPFL_INVALID_HW_ID) > + return -EINVAL; > + ids[0] = (uint8_t)vsi_id; > + ids[1] = (uint8_t)vsi_id >> 8; > + > + ret = _cpfl_tdi_table_key_field_set(dev, kobj, key_field_info, ids, size); I think here it would be safer to set size = 2. otherwise stack overflow can happen > + if (ret != 0) > + return -EINVAL; > + } else { > + ret = _cpfl_tdi_table_key_field_set(dev, kobj, key_field_info, value, size); > + if (ret != 0) > + return -EINVAL; > + } > + > + return 0; > +} > + > + > +static inline bool __rte_unused is this function necessary in this patch series? > +verify_action(struct cpfl_tdi_table_node *table_node, uint32_t spec_id) > +{ > + int i; > + const struct cpfl_tdi_table *table = table_node->table; > + > + for (i = 0; i < table->action_num; i++) > + if (spec_id == table->actions[i].handle) > + return true; > + > + return false; > +} > + > +static int > +cpfl_tdi_action_node_get_by_spec_id(struct rte_eth_dev *dev, > + uint32_t table_id __rte_unused, > + uint32_t spec_id, > + struct cpfl_tdi_action_node **action_node) > +{ > + return cpfl_tdi_action_spec_info_get(dev, spec_id, action_node); > +} What's the purpose of this static wrapper function? > + > +static int > +cpfl_tdi_action_spec_field_info_get(struct rte_eth_dev *dev __rte_unused, > + struct cpfl_tdi_action_node *anode, > + uint32_t field_id, > + struct cpfl_tdi_action_spec_field_info **info) > +{ > + int ret = -EINVAL; > + int i, j; > + > + if (anode->format == NULL) > + goto err; > + > + for (i = 0; i < anode->format->immediate_field_num; i++) { > + struct cpfl_tdi_immediate_field *field = &anode->format->immediate_fields[i]; > + struct cpfl_tdi_action_spec_field_info *asfinfo; > + > + if (field->param_handle != field_id) > + continue; > + > + asfinfo = rte_malloc(NULL, sizeof(struct cpfl_tdi_action_spec_field_info), 0); > + if (asfinfo == NULL) { > + ret = -ENOMEM; > + goto err; why not just return from here? > + } > + > + asfinfo->field = field; > + asfinfo->param = anode->params[i]; > + > +static int > +_cpfl_tdi_action_field_set(struct rte_eth_dev *dev __rte_unused, > + struct cpfl_tdi_action_obj *aobj, > + struct cpfl_tdi_action_spec_field_info *asfinfo, > + const uint8_t *value, > + uint16_t size, > + enum cpfl_act_fwd_type fwd_type) > +{ > + struct cpfl_tdi_action_node *node = aobj->node; > + struct cpfl_tdi_param_info *pi = &asfinfo->param; > + uint8_t val_buf[CPFL_TDI_VALUE_SIZE_MAX] = {0}; > + uint8_t msk_buf[CPFL_TDI_VALUE_SIZE_MAX] = {0}; > + > + memcpy(val_buf, value, size); > + > + if (node->format->mod_content_format.mod_field_num > 0) { > + struct cpfl_tdi_mod_field *mf = asfinfo->mod_field; > + > + cpfl_tdi_shift_left(val_buf, pi->size, mf->start_bit_offset); how it is guaranteed that pi->size is less than CPFL_TDI_VALUE_SIZE_MAX? What is the difference between size argument and pi->size here? > + cpfl_tdi_init_msk_buf(msk_buf, pi->size, mf->bit_width); > + cpfl_tdi_shift_left(msk_buf, pi->size, mf->start_bit_offset); > + cpfl_tdi_or_buf(&aobj->buf[pi->offset], pi->size, val_buf, msk_buf); > + } else { > + struct cpfl_tdi_hw_action *ha = asfinfo->hw_action; > + uint32_t action_code = cpfl_tdi_to_action_code(ha, val_buf, fwd_type); > + > + memcpy(&aobj->buf[pi->offset], &action_code, 4); > + } > + > + return 0; why this function returns anything when it always returns 0? > +} > + > +static int > +cpfl_tdi_action_field_set(struct rte_eth_dev *dev, > + struct cpfl_tdi_action_obj *aobj, > + struct cpfl_tdi_action_node *action_node, > + uint32_t field_id, > + const struct rte_flow_action_prog_argument *arg) > +{ > + struct cpfl_tdi_action_spec_field_info *asfinfo; > + enum rte_flow_action_type action_type; > + /* used when action is PORT_REPRESENTOR type */ > + struct cpfl_itf *dst_itf; > + uint16_t dev_id; /* vsi id */ > + uint16_t port_id; > + uint8_t value; > + bool is_vsi; > + int ret; > + enum cpfl_act_fwd_type fwd_type; > + > + if (!aobj || !arg) > + return -EINVAL; > + > + ret = cpfl_tdi_action_spec_field_info_get(dev, action_node, field_id, &asfinfo); it seems like asfinfo that was allocated inside cpfl_tdi_action_spec_field_info_get() is only used locally inside the cpfl_tdi_action_field_set(). It should be freed somewhere in this function before it returns. Or as a better solution why not just have this struct as stack allocated, don't allocate memory for it at all and just init it inside the cpfl_tdi_action_spec_field_info_get()? Otherwise you'll have problems with possible memory leak. > + if (ret != 0) > + return -EINVAL; > + > + if (!strcmp(arg->name, "port_representor")) > + action_type = RTE_FLOW_ACTION_TYPE_PORT_REPRESENTOR; > + else if (!strcmp(arg->name, "represented_port")) > + action_type = RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT; > + else > + return _cpfl_tdi_action_field_set(dev, aobj, asfinfo, arg->value, arg->size, > + CPFL_NO_FWD); > + > + if (arg->size != 1 && arg->size != 2) /* input port_id should be 8/16 bits */ > + return -EINVAL; same problem with memory leak after cpfl_tdi_action_spec_field_info_get() > + > + port_id = arg->size == 1 ? arg->value[0] : arg->value[0] << 8 | arg->value[1]; > + dst_itf = cpfl_get_itf_by_port_id(port_id); > + if (!dst_itf) > + goto err; > + > + is_vsi = (action_type == RTE_FLOW_ACTION_TYPE_PORT_REPRESENTOR || > + dst_itf->type == CPFL_ITF_TYPE_REPRESENTOR); > + if (is_vsi) > + dev_id = cpfl_get_vsi_id(dst_itf); > + else > + dev_id = cpfl_get_port_id(dst_itf); > + > + if (dev_id == CPFL_INVALID_HW_ID) > + goto err; > + > + value = (uint8_t)dev_id; > + fwd_type = is_vsi ? CPFL_ACT_FWD_VSI : CPFL_ACT_FWD_PORT; > + > + return _cpfl_tdi_action_field_set(dev, aobj, asfinfo, &value, arg->size, fwd_type); > +err: > + PMD_DRV_LOG(ERR, "Can not get dev id."); > + return -EINVAL; > +} > + > +int > +cpfl_tdi_build(struct cpfl_flow_parser *flow_parser) > +{ > + int ret; > + for non static function input arguments should probably be checked. > + ret = cpfl_tdi_build_table_list(flow_parser); > + if (ret != 0) { > + PMD_INIT_LOG(ERR, "Failed to build tdi table list"); > + return ret; > + } > + > +static int > +cpfl_tdi_parse_action(struct rte_eth_dev *dev, > + const struct rte_flow_action actions[], > + uint32_t table_id, > + struct cpfl_tdi_table_node *table_node, > + struct cpfl_tdi_action_node **action_node, > + struct cpfl_tdi_action_obj *aobj) > +{ > + const struct rte_flow_action_prog *prog; > + const struct rte_flow_action_prog_argument *arg; > + uint32_t action_spec_id; > + int i, ret; > + uint32_t j; > + > + for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) { > + if (actions[i].type != RTE_FLOW_ACTION_TYPE_PROG) > + continue; > + > + prog = actions[i].conf; > + action_spec_id = atoi(prog->name); > + /* Get action node */ > + ret = > + cpfl_tdi_action_node_get_by_spec_id(dev, table_id, action_spec_id, action_node); > + if (ret) { > + PMD_INIT_LOG(ERR, "Failed to get action node."); > + return -EINVAL; > + } > + > + ret = cpfl_tdi_action_obj_init(dev, table_node, *action_node, aobj); > + if (ret != 0) { nothing can happen inside cpfl_tdi_action_obj_init(), it always returns 0 > + PMD_INIT_LOG(ERR, "Failed to init action obj."); > + return -EINVAL; > + } > + > + for (j = 0; j < prog->args_num; j++) { > + arg = &prog->args[j]; > + /* Set the action fields */ > + ret = cpfl_tdi_action_field_set(dev, aobj, *action_node, j, arg); > + if (ret) { > + PMD_INIT_LOG(ERR, "Failed to set action field."); > + return -EINVAL; > + } > + } > + } > + return 0; > +} > + > -- Regards, Vladimir --------------PVPUTNGAjORIU8SWZwtb8Mpc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: 8bit

Hi Wenjing,

Please find comments inlined

On 05/01/2024 08:16, wenjing.qiao@intel.com wrote:
From: Wenjing Qiao <wenjing.qiao@intel.com>

Add TDI implementation to a flow engine.

Signed-off-by: Wenjing Qiao <wenjing.qiao@intel.com>
---
<snip>
--- /dev/null
+++ b/drivers/net/cpfl/cpfl_tdi.c
@@ -0,0 +1,1282 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Intel Corporation
+ */
+#include <rte_hash_crc.h>
+#include <rte_tailq.h>
RTE includes should be after system includes
+#include <stdint.h>
+#include <string.h>
+
+#include "cpfl_actions.h"
+#include "cpfl_flow.h"
+#include "cpfl_fxp_rule.h"
+#include "cpfl_tdi.h"
+#include "cpfl_tdi_parser.h"
+#include "rte_branch_prediction.h"
+#include "rte_common.h"
+#include "rte_flow.h"
Those includes should be together with other RTE includes and have angle brackets
+
+#define CPFL_NO_FWD 0
+uint64_t cpfl_tdi_rule_cookie = CPFL_COOKIE_DEF;
+
+/*help function to do left shift on a byte array */
+static void
+cpfl_tdi_shift_left(uint8_t *buf, int length, uint32_t shift_amount)
+{
+	uint32_t i;
+	int j;
+	uint32_t carry = 0;
+
+	if (shift_amount == 0)
+		return;
+
+	for (i = 0; i < shift_amount; i += 8) {
+		for (j = 0; j < length; j++) {
+			uint32_t temp = (buf[j] << (shift_amount - i)) | carry;
+
+			carry = (temp >> 8) & 0xff;
+			buf[j] = temp & 0xff;
+		}
+	}
+}

This function does not looks correct to me, at least in case when shift_amount is larger than 8.

You can try something like:

static void
cpfl_tdi_shift_left(uint8_t *buf, int length, uint32_t shift_amount)
{
        int i;
        int bit_shift = shift_amount & 0x7;
        int byte_shift = shift_amount >> 3;

        if (shift_amount >= (length * 8))
                return;

        uint8_t prev = 0;
        for (i = 0; i < length; i++) {
                uint8_t val = (buf[i] << bit_shift) | prev;
                prev = buf[i] >> (8 - bit_shift);
                buf[i] = val;
        }

        for (i = length - 1; i >= byte_shift; i--) {
                buf[i] = buf[i - byte_shift];
        }

        for(; i >= 0; i--)
                buf[i] = 0;
}

+
+/* help function to init a mask array with bit_width */
+static void
+cpfl_tdi_init_msk_buf(uint8_t *buf, int length, uint32_t bit_width)
+{
+	uint32_t i;
+
+	memset(buf, 0, length);
+	for (i = 0; i < bit_width; i++) {
+		cpfl_tdi_shift_left(buf, length, 1);
+		buf[0] += 1;
+	}
+}

It seems like this function sets bit_width bits into the buf starting from the LSB. If so I'd suggest to make it simpler and not use cpfl_tdi_shift_left(). Instead you can just write UINT8/16/32/64_MAX into the corresponding buffer and decrease bit_width accordingly if it is bigger than 8/16/32/64 and finally write (1 << bit_width) - 1.

+
+/* help function to OR a byte array with value and mask */
+static void
+cpfl_tdi_or_buf(uint8_t *buf, int length, uint8_t *values, uint8_t *mask)
+{
+	int i;
+
+	for (i = 0; i < length; i++)
+		buf[i] = (values[i] & mask[i]) | (buf[i] & ~mask[i]);
I guess "& ~mask[i]" is not necessary here if this function is really OR. 
+}
<snip>
+
+static void
+cpfl_tdi_pack_sem_entry(struct cpfl_tdi_rule_info *rinfo,
+			struct cpfl_tdi_ma_hardware_block *hb,
+			enum cpfl_tdi_table_entry_op op,
+			struct idpf_dma_mem *dma,
+			struct idpf_ctlq_msg *msg)
+{
+	union cpfl_rule_cfg_pkt_record *blob;
+	struct cpfl_rule_cfg_data cfg = {0};
+	uint16_t cfg_ctrl;
+	enum cpfl_ctlq_rule_cfg_opc opc = 0;
+	const struct cpfl_tdi_table_key_obj *key = &rinfo->kobj;
+	const struct cpfl_tdi_action_obj *action = &rinfo->aobj;
+
+	blob = (void *)dma->va;
Why not cast to (union cpfl_rule_cfg_pkt_record *)?
+	memset(blob, 0, sizeof(*blob));
+
+	cfg_ctrl = CPFL_GET_MEV_SEM_RULE_CFG_CTRL(hb->profile[0], hb->sem.sub_profile, 0, 0);
+
<snip>
+static int
+cpfl_tdi_rule_process(struct cpfl_itf *itf,
+		      struct idpf_ctlq_info *tx_cq,
+		      struct idpf_ctlq_info *rx_cq,
+		      struct cpfl_tdi_rule_info *rinfo,
+		      int rule_num,
+		      enum cpfl_tdi_table_entry_op op)
+{
+	const struct cpfl_tdi_table_key_obj *kobj;
+	struct idpf_hw *hw = &itf->adapter->base.hw;
+	struct cpfl_tdi_ma_hardware_block *hb;
+	const struct cpfl_tdi_table *table;
+	int ret = 0;
+
+	if (rule_num == 0)
+		return 0;

Is it necessary to check this value? This function is called with nonzero value always.

As a general question, do we need to check input parameters in all these static functions? It seems some of them are callbacks and their parameters are probably checked by API, but for others it may be the case.

+
+	kobj = &rinfo->kobj;
+
+	table = kobj->tnode->table;
<snip>
+static int
+cpfl_tdi_fxp_rule_destroy(struct rte_eth_dev *dev,
+			  struct rte_flow *flow,
+			  struct rte_flow_error *error)
+{
+	struct cpfl_itf *itf = CPFL_DEV_TO_ITF(dev);
+	struct cpfl_adapter_ext *ad = itf->adapter;
+	struct cpfl_vport *vport;
+	struct cpfl_repr *repr;
+	struct cpfl_tdi_rule_info *rinfo = (struct cpfl_tdi_rule_info *)flow->rule;
+	int ret = 0;
+	uint32_t cpq_id = 0;
+
+	if (itf->type == CPFL_ITF_TYPE_VPORT) {
+		vport = (struct cpfl_vport *)itf;
+		cpq_id = vport->base.devarg_id * 2;
+	} else if (itf->type == CPFL_ITF_TYPE_REPRESENTOR) {
+		repr = (struct cpfl_repr *)itf;
+		cpq_id = ((repr->repr_id.pf_id + repr->repr_id.vf_id) & (CPFL_TX_CFGQ_NUM - 1)) * 2;
+	} else {
+		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
+				   "fail to find correct control queue");
+		ret = -rte_errno;
+		goto err;
+	}
+
+	ret = cpfl_tdi_rule_process(itf, ad->ctlqp[cpq_id], ad->ctlqp[cpq_id + 1], rinfo, 1,
+				    CPFL_TDI_TABLE_ENTRY_OP_DEL);
+	if (ret)
+		goto err;
What's the purpose of this goto here?
+
+err:
+	rte_free(rinfo);
+	flow->rule = NULL;
+	return ret;
+}
+
+void
+cpfl_tdi_free_table_list(struct cpfl_flow_parser *flow_parser)
+{
+	struct cpfl_tdi_table_node *node;
Should this parameter be checked since this is non static function?
+
+	while ((node = TAILQ_FIRST(&flow_parser->tdi_table_list))) {
+		TAILQ_REMOVE(&flow_parser->tdi_table_list, node, next);
+		rte_free(node);
+	}
+}
+
<snip>
+
+static int
+cpfl_tdi_table_key_create(struct rte_eth_dev *dev,
+			  uint32_t table_id,
+			  struct cpfl_tdi_table_node **table_node,
+			  struct cpfl_tdi_table_key_obj *kobj)
+{
+	int ret;
+
+	if (!kobj)
+		return -EINVAL;

is it required for static function? in cpfl_tdi_parse_pattern_action() kobj is zero inited, so in can not be NULL here. Otherwise it would make sense to check all other arguments as well. I'd suggest to replace runtime if check with RTE_ASSERT on all input arguments instead.

+
+	ret = cpfl_tdi_table_info_get(dev, table_id, table_node);
+	if (ret != 0)
+		return -EINVAL;
+
+	ret = cpfl_tdi_table_key_node_init(dev, *table_node, kobj);
+	if (ret != 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int
+cpfl_tdi_table_key_field_info_get(__rte_unused struct rte_eth_dev *dev,
why not remove this argument is it is unused? Same applied to several other functions
+				  struct cpfl_tdi_table_node *node,
+				  uint32_t field_id,
+				  struct cpfl_tdi_table_key_field_info **key_field_info)
+{
<snip>
+static int
+cpfl_tdi_table_key_field_set(struct rte_eth_dev *dev,
+			     struct cpfl_tdi_table_node *table_node,
+			     struct cpfl_tdi_table_key_obj *kobj,
+			     uint32_t field_id,
+			     const uint8_t *value,
+			     uint16_t size)
+{
+	struct cpfl_tdi_table_key_field_info *key_field_info;
+	int ret;
+
+	if (!kobj || !value)
+		return -EINVAL;
+
+	ret = cpfl_tdi_table_key_field_info_get(dev, table_node, field_id, &key_field_info);
+	if (ret != 0)
+		return -EINVAL;
+
+	if (key_field_info->field->match_type != CPFL_TDI_MATCH_TYPE_EXACT)
+		return -EINVAL;
possible memory leak since cpfl_tdi_table_key_field_info_get() allocated memory for key_field_info. Also it would be better to name properly functions where memory allocation was done.
+
+	if (unlikely(key_field_info->field->is_vsi)) {
No need to use branch predictor hints since this is not a fast path
+		struct cpfl_itf *src_itf;
+		uint16_t vsi_id, port_id;
+		uint8_t ids[2];
+
+		if (size != 1 && size != 2) /* input port id should be 8/16 bits */
+			return -EINVAL;
+		port_id = size == 1 ? value[0] : value[0] << 8 | value[1];
please use parenthesis for easier reading
+		src_itf = cpfl_get_itf_by_port_id(port_id);
+		if (!src_itf)
+			return -EINVAL;
+		vsi_id = cpfl_get_vsi_id(src_itf);
+		if (vsi_id == CPFL_INVALID_HW_ID)
+			return -EINVAL;
+		ids[0] = (uint8_t)vsi_id;
+		ids[1] = (uint8_t)vsi_id >> 8;
+
+		ret = _cpfl_tdi_table_key_field_set(dev, kobj, key_field_info, ids, size);
I think here it would be safer to set size = 2. otherwise stack overflow can happen
+		if (ret != 0)
+			return -EINVAL;
+	} else {
+		ret = _cpfl_tdi_table_key_field_set(dev, kobj, key_field_info, value, size);
+		if (ret != 0)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
<snip>
+
+static inline bool __rte_unused
is this function necessary in this patch series?
+verify_action(struct cpfl_tdi_table_node *table_node, uint32_t spec_id)
+{
+	int i;
+	const struct cpfl_tdi_table *table = table_node->table;
+
+	for (i = 0; i < table->action_num; i++)
+		if (spec_id == table->actions[i].handle)
+			return true;
+
+	return false;
+}
+
+static int
+cpfl_tdi_action_node_get_by_spec_id(struct rte_eth_dev *dev,
+				    uint32_t table_id __rte_unused,
+				    uint32_t spec_id,
+				    struct cpfl_tdi_action_node **action_node)
+{
+	return cpfl_tdi_action_spec_info_get(dev, spec_id, action_node);
+}
What's the purpose of this static wrapper function?
+
+static int
+cpfl_tdi_action_spec_field_info_get(struct rte_eth_dev *dev __rte_unused,
+				    struct cpfl_tdi_action_node *anode,
+				    uint32_t field_id,
+				    struct cpfl_tdi_action_spec_field_info **info)
+{
+	int ret = -EINVAL;
+	int i, j;
+
+	if (anode->format == NULL)
+		goto err;
+
+	for (i = 0; i < anode->format->immediate_field_num; i++) {
+		struct cpfl_tdi_immediate_field *field = &anode->format->immediate_fields[i];
+		struct cpfl_tdi_action_spec_field_info *asfinfo;
+
+		if (field->param_handle != field_id)
+			continue;
+
+		asfinfo = rte_malloc(NULL, sizeof(struct cpfl_tdi_action_spec_field_info), 0);
+		if (asfinfo == NULL) {
+			ret = -ENOMEM;
+			goto err;
why not just return from here?
+		}
+
+		asfinfo->field = field;
+		asfinfo->param = anode->params[i];
+
<snip>
+static int
+_cpfl_tdi_action_field_set(struct rte_eth_dev *dev __rte_unused,
+			   struct cpfl_tdi_action_obj *aobj,
+			   struct cpfl_tdi_action_spec_field_info *asfinfo,
+			   const uint8_t *value,
+			   uint16_t size,
+			   enum cpfl_act_fwd_type fwd_type)
+{
+	struct cpfl_tdi_action_node *node = aobj->node;
+	struct cpfl_tdi_param_info *pi = &asfinfo->param;
+	uint8_t val_buf[CPFL_TDI_VALUE_SIZE_MAX] = {0};
+	uint8_t msk_buf[CPFL_TDI_VALUE_SIZE_MAX] = {0};
+
+	memcpy(val_buf, value, size);
+
+	if (node->format->mod_content_format.mod_field_num > 0) {
+		struct cpfl_tdi_mod_field *mf = asfinfo->mod_field;
+
+		cpfl_tdi_shift_left(val_buf, pi->size, mf->start_bit_offset);

how it is guaranteed that pi->size is less than CPFL_TDI_VALUE_SIZE_MAX?

What is the difference between size argument and pi->size here?

+		cpfl_tdi_init_msk_buf(msk_buf, pi->size, mf->bit_width);
+		cpfl_tdi_shift_left(msk_buf, pi->size, mf->start_bit_offset);
+		cpfl_tdi_or_buf(&aobj->buf[pi->offset], pi->size, val_buf, msk_buf);
+	} else {
+		struct cpfl_tdi_hw_action *ha = asfinfo->hw_action;
+		uint32_t action_code = cpfl_tdi_to_action_code(ha, val_buf, fwd_type);
+
+		memcpy(&aobj->buf[pi->offset], &action_code, 4);
+	}
+
+	return 0;
why this function returns anything when it always returns 0?
+}
+
+static int
+cpfl_tdi_action_field_set(struct rte_eth_dev *dev,
+			  struct cpfl_tdi_action_obj *aobj,
+			  struct cpfl_tdi_action_node *action_node,
+			  uint32_t field_id,
+			  const struct rte_flow_action_prog_argument *arg)
+{
+	struct cpfl_tdi_action_spec_field_info *asfinfo;
+	enum rte_flow_action_type action_type;
+	/* used when action is PORT_REPRESENTOR type */
+	struct cpfl_itf *dst_itf;
+	uint16_t dev_id; /* vsi id */
+	uint16_t port_id;
+	uint8_t value;
+	bool is_vsi;
+	int ret;
+	enum cpfl_act_fwd_type fwd_type;
+
+	if (!aobj || !arg)
+		return -EINVAL;
+
+	ret = cpfl_tdi_action_spec_field_info_get(dev, action_node, field_id, &asfinfo);

it seems like asfinfo that was allocated inside cpfl_tdi_action_spec_field_info_get() is only used locally inside the cpfl_tdi_action_field_set(). It should be freed somewhere in this function before it returns.

Or as a better solution why not just have this struct as stack allocated, don't allocate memory for it at all and just init it inside the cpfl_tdi_action_spec_field_info_get()? Otherwise you'll have problems with possible memory leak.

+	if (ret != 0)
+		return -EINVAL;
+
+	if (!strcmp(arg->name, "port_representor"))
+		action_type = RTE_FLOW_ACTION_TYPE_PORT_REPRESENTOR;
+	else if (!strcmp(arg->name, "represented_port"))
+		action_type = RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT;
+	else
+		return _cpfl_tdi_action_field_set(dev, aobj, asfinfo, arg->value, arg->size,
+						  CPFL_NO_FWD);
+
+	if (arg->size != 1 && arg->size != 2) /* input port_id should be 8/16 bits */
+		return -EINVAL;
same problem with memory leak after cpfl_tdi_action_spec_field_info_get()
+
+	port_id = arg->size == 1 ? arg->value[0] : arg->value[0] << 8 | arg->value[1];
+	dst_itf = cpfl_get_itf_by_port_id(port_id);
+	if (!dst_itf)
+		goto err;
+
+	is_vsi = (action_type == RTE_FLOW_ACTION_TYPE_PORT_REPRESENTOR ||
+		  dst_itf->type == CPFL_ITF_TYPE_REPRESENTOR);
+	if (is_vsi)
+		dev_id = cpfl_get_vsi_id(dst_itf);
+	else
+		dev_id = cpfl_get_port_id(dst_itf);
+
+	if (dev_id == CPFL_INVALID_HW_ID)
+		goto err;
+
+	value = (uint8_t)dev_id;
+	fwd_type = is_vsi ? CPFL_ACT_FWD_VSI : CPFL_ACT_FWD_PORT;
+
+	return _cpfl_tdi_action_field_set(dev, aobj, asfinfo, &value, arg->size, fwd_type);
+err:
+	PMD_DRV_LOG(ERR, "Can not get dev id.");
+	return -EINVAL;
+}
+
+int
+cpfl_tdi_build(struct cpfl_flow_parser *flow_parser)
+{
+	int ret;
+
for non static function input arguments should probably be checked.
+	ret = cpfl_tdi_build_table_list(flow_parser);
+	if (ret != 0) {
+		PMD_INIT_LOG(ERR, "Failed to build tdi table list");
+		return ret;
+	}
+
<snip>
+static int
+cpfl_tdi_parse_action(struct rte_eth_dev *dev,
+		      const struct rte_flow_action actions[],
+		      uint32_t table_id,
+		      struct cpfl_tdi_table_node *table_node,
+		      struct cpfl_tdi_action_node **action_node,
+		      struct cpfl_tdi_action_obj *aobj)
+{
+	const struct rte_flow_action_prog *prog;
+	const struct rte_flow_action_prog_argument *arg;
+	uint32_t action_spec_id;
+	int i, ret;
+	uint32_t j;
+
+	for (i = 0; actions[i].type != RTE_FLOW_ACTION_TYPE_END; i++) {
+		if (actions[i].type != RTE_FLOW_ACTION_TYPE_PROG)
+			continue;
+
+		prog = actions[i].conf;
+		action_spec_id = atoi(prog->name);
+		/* Get action node */
+		ret =
+		    cpfl_tdi_action_node_get_by_spec_id(dev, table_id, action_spec_id, action_node);
+		if (ret) {
+			PMD_INIT_LOG(ERR, "Failed to get action node.");
+			return -EINVAL;
+		}
+
+		ret = cpfl_tdi_action_obj_init(dev, table_node, *action_node, aobj);
+		if (ret != 0) {
nothing can happen inside cpfl_tdi_action_obj_init(), it always returns 0
+			PMD_INIT_LOG(ERR, "Failed to init action obj.");
+			return -EINVAL;
+		}
+
+		for (j = 0; j < prog->args_num; j++) {
+			arg = &prog->args[j];
+			/* Set the action fields */
+			ret = cpfl_tdi_action_field_set(dev, aobj, *action_node, j, arg);
+			if (ret) {
+				PMD_INIT_LOG(ERR, "Failed to set action field.");
+				return -EINVAL;
+			}
+		}
+	}
+	return 0;
+}
+
<snip>
-- 
Regards,
Vladimir
--------------PVPUTNGAjORIU8SWZwtb8Mpc--