|SUCCESS| [2/2] net/cnxk: add PMD API to support custom profile setup

dpdklab at iol.unh.edu dpdklab at iol.unh.edu
Tue May 5 21:43:17 CEST 2026
Previous message (by thread): |Smoke |SUCCESS| pw(163596) sid(38052) job(PER_PATCH_BUILD18712-20260506002139) [v2] ring: add cache guard after ring elements table
Next message (by thread): |PENDING| pw163597-163602 [PATCH] [v12,6/6] test: add flow parser fu
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163570

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-05

Commit: [git sha needed - not visible in patch]
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>

Subject: [PATCH 1/2] common/cnxk: add ROC API to support custom profile

This patch adds ROC API support for custom inline profile creation and
configures SA tables for both inline device and NIX device.

> diff --git a/drivers/common/cnxk/hw/nix.h b/drivers/common/cnxk/hw/nix.h
> index a2a372a56a..6abb184d45 100644
> --- a/drivers/common/cnxk/hw/nix.h
> +++ b/drivers/common/cnxk/hw/nix.h
> @@ -2794,4 +2794,48 @@ enum cgx_port_type {
>  	CGX_PORT_OTHER = 0xff,
>  };
>  
> +/* Shift values for NIX_AF_RX_DEF_INLINE register */
> +enum {
> +	NIX_LTYPE_MASK_SHIFT = 0,
> +	NIX_LTYPE_MATCH_SHIFT = 4,
> +	NIX_LID_SHIFT = 8,
> +	NIX_GEN_OFFSET_NG_SHIFT = 11,
> +	NIX_GEN_OFFSET_SHIFT = 12,
> +	NIX_GEN_NZ_SHIFT = 16,
> +	NIX_CPT_L3_LID_SHIFT = 17,
> +	NIX_FLAGS_MASK_SHIFT = 20,
> +	NIX_FLAGS_MATCH_SHIFT = 24,
> +	NIX_MATCH_OIPV4_SHIFT = 28,
> +	NIX_MATCH_OIPV6_SHIFT = 29,
> +	NIX_OIPLEN_ENA_SHIFT = 30,
> +	NIX_INLINE_SHIFT_SHIFT = 32,
> +};

These enumerations lack a distinctive prefix to prevent namespace
collisions. Per the coding style guide section 3.8.1, enumerations should
be prefixed appropriately. Should these use NIX_AF_RX_DEF_INLINE_ prefix?

> diff --git a/drivers/common/cnxk/roc_nix_inl.c b/drivers/common/cnxk/roc_nix_inl.c
> index ecf93ed03e..b515d52534 100644
> --- a/drivers/common/cnxk/roc_nix_inl.c
> +++ b/drivers/common/cnxk/roc_nix_inl.c

[ ... ]

> +uintptr_t
> +roc_nix_inl_inb_prof_sa_base_get(struct roc_nix *roc_nix, bool inb_inl_dev, uint16_t profile_id)
> +{
> +	struct idev_cfg *idev = idev_get_cfg();
> +	struct nix_inl_dev *inl_dev;
> +	struct nix *nix = NULL;
> +
> +	/* Validate profile_id range */
> +	if (profile_id >= ROC_NIX_INL_PROFILE_CNT) {
> +		plt_err("Invalid profile_id %u, must be < %u", profile_id, ROC_NIX_INL_PROFILE_CNT);
> +		return 0;
> +	}
> +
> +	if (idev == NULL)
> +		return 0;
> +
> +	if (!inb_inl_dev && roc_nix == NULL)
> +		return 0;

Does this code properly handle the case where inb_inl_dev is true but
roc_nix is NULL? The function dereferences roc_nix at line "nix =
roc_nix_to_nix_priv(roc_nix)" without checking if roc_nix is NULL when
inb_inl_dev is true.

> +
> +	if (roc_nix) {
> +		nix = roc_nix_to_nix_priv(roc_nix);
> +		if (!nix->inl_inb_ena)
> +			return 0;
> +	}

[ ... ]

> +static int
> +nix_inl_custom_profile_sa_tbl_setup(struct roc_nix *roc_nix, uint32_t sa_size, uint32_t max_sa,
> +				    uint16_t profile_id)
> +{
> +	struct nix *nix = roc_nix_to_nix_priv(roc_nix);
> +	struct idev_cfg *idev = idev_get_cfg();
> +	struct nix_inl_dev *inl_dev = NULL;
> +	struct nix_rx_inl_lf_cfg_req *lf_cfg;
> +	uint64_t res_addr_offset = 0;
> +	uint64_t cpt_cq_ena = 0;
> +	uint32_t inb_sa_sz;
> +	uint32_t lenm1_max;
> +	uint64_t def_cptq = 0;
> +	struct mbox *mbox;
> +	uint8_t sa_pow2_sz;
> +	uint8_t sa_w;
> +	int rc = 0;

Per coding style 4.5, the "reverse Christmas tree" declaration ordering
is preferred. Can the declarations be reordered by line length?

> +	/* Use sa_size parameter if provided, otherwise use default */
> +	if (sa_size > 0)
> +		inb_sa_sz = sa_size;
> +	else
> +		inb_sa_sz = ROC_NIX_INL_OW_IPSEC_INB_SA_SZ;

Per coding style 3.1, avoid unnecessary explicit comparisons. Should this
be "if (sa_size)"?

> +	nix->inb_sa_sz[profile_id] = inb_sa_sz;
> +	nix->inb_sa_base[profile_id] = plt_zmalloc(inb_sa_sz * max_sa, ROC_NIX_INL_SA_BASE_ALIGN);
> +	if (!nix->inb_sa_base[profile_id]) {
> +		plt_err("Failed to allocate memory for Inbound SA for profile %u", profile_id);
> +		rc = -ENOMEM;
> +		goto exit;
> +	}

Does this code check for integer overflow in the multiplication
"inb_sa_sz * max_sa"?

> +	mbox = mbox_get(nix->dev.mbox);
> +	lf_cfg = mbox_alloc_msg_nix_rx_inl_lf_cfg(mbox);
> +	if (lf_cfg == NULL) {
> +		rc = -ENOSPC;
> +		goto free_mem;
> +	}

Does the mbox obtained via mbox_get() need to be released if
mbox_alloc_msg_nix_rx_inl_lf_cfg() fails?

[ ... ]

> +int
> +roc_nix_inl_custom_profile_setup(struct roc_nix *roc_nix, uint64_t def_cfg, uint64_t gen_cfg,
> +				 uint64_t extract_cfg, const uint64_t *prot_field_cfg,
> +				 uint32_t sa_size, uint32_t max_sa, bool is_inline,
> +				 uint16_t *profile_id)
> +{
> +	struct nix_rx_inl_profile_cfg_req *req;
> +	struct nix_rx_inl_profile_cfg_rsp *rsp;
> +	struct nix *nix = roc_nix_to_nix_priv(roc_nix);
> +	struct idev_cfg *idev = idev_get_cfg();
> +	struct nix_inl_dev *inl_dev = NULL;
> +	struct mbox *mbox;
> +	int rc = 0;
> +	int i;

Per coding style 4.5, can the declarations be reordered by line length?

> +	if (roc_nix == NULL || profile_id == NULL)
> +		return -EINVAL;

Per coding style 3.1, avoid explicit NULL comparison. Should this be "if
(!roc_nix || !profile_id)"?

> +	/* Use inline device mbox if is_inline is true, otherwise use nix mbox */
> +	if (is_inline)
> +		mbox = mbox_get(inl_dev->dev.mbox);
> +	else
> +		mbox = mbox_get(nix->dev.mbox);
> +
> +	req = mbox_alloc_msg_nix_rx_inl_profile_cfg(mbox);
> +	if (req == NULL) {
> +		rc = -ENOSPC;
> +		plt_err("Failed to alloc %s profile cfg mbox msg",
> +			is_inline ? "inline device" : "nix device");
> +		mbox_put(mbox);
> +		goto exit;
> +	}

Per coding style 3.1, avoid explicit NULL comparison. Should this be "if
(!req)"?

> +	req->def_cfg = def_cfg;
> +	req->gen_cfg = gen_cfg;
> +	req->extract_cfg = extract_cfg;
> +	for (i = 0; i < NIX_RX_INL_PROFILE_PROTO_CNT; i++)
> +		req->prot_field_cfg[i] = prot_field_cfg[i];

Does this code validate that prot_field_cfg is non-NULL before
dereferencing it?

> +	if (rc) {
> +		plt_err("Failed to configure custom profile %u, rc=%d", *profile_id, rc);
> +		/* Release the allocated profile on failure */
> +		roc_nix_inl_custom_profile_release(roc_nix, *profile_id, is_inline);
> +	}

If the profile configuration fails, does
roc_nix_inl_custom_profile_release() correctly handle a partially
initialized profile?

[ ... ]

> diff --git a/drivers/common/cnxk/roc_nix_inl_dev.c b/drivers/common/cnxk/roc_nix_inl_dev.c
> index 246dd4612f..667209b8a0 100644
> --- a/drivers/common/cnxk/roc_nix_inl_dev.c
> +++ b/drivers/common/cnxk/roc_nix_inl_dev.c

[ ... ]

> +int
> +nix_inl_dev_profile_config(struct nix_inl_dev *inl_dev, uint32_t sa_size, uint32_t max_sa,
> +			   uint16_t profile_id)
>  {
>  	struct mbox *mbox = mbox_get((&inl_dev->dev)->mbox);
> -	uint64_t max_sa, sa_w, sa_pow2_sz, lenm1_max;
> +	uint64_t sa_w, sa_pow2_sz, lenm1_max;

Why is the function signature changing max_sa from uint64_t to uint32_t?
Does this affect callers or create potential truncation issues?

> +	/* Initialize all SAs */
> +	for (i = 0; i < max_sa; i++) {
> +		sa = ((uint8_t *)inl_dev->inb_sa_base[profile_id]) + (i * inb_sa_sz);
> +		roc_ow_reass_inb_sa_init(sa);
> +	}

Does this loop check for integer overflow in the calculation "i *
inb_sa_sz"?

---

Commit: [git sha needed - not visible in patch]
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>

Subject: [PATCH 2/2] net/cnxk: add PMD API to support custom profile setup

This patch adds PMD APIs to create custom profiles and update SA tables
based on profile ID.

> diff --git a/drivers/net/cnxk/cnxk_ethdev.h b/drivers/net/cnxk/cnxk_ethdev.h
> index 8691acc642..ea6a2be30e 100644
> --- a/drivers/net/cnxk/cnxk_ethdev.h
> +++ b/drivers/net/cnxk/cnxk_ethdev.h
> @@ -26,6 +26,10 @@
>  
>  #define CNXK_ETH_DEV_PMD_VERSION "1.0"
>  
> +/* Macros for packing configuration fields */
> +#define CNXK_FIELD_MASK(width)	((1ULL << (width)) - 1)
> +#define PACK(val, shift, width) (((uint64_t)((val) & CNXK_FIELD_MASK(width))) << (shift))

The CNXK_FIELD_MASK macro does not validate that width is less than 64.
Does this code handle the case where width equals or exceeds 64, which
would cause undefined behavior in the left shift?

> diff --git a/drivers/net/cnxk/cnxk_ethdev_sec.c b/drivers/net/cnxk/cnxk_ethdev_sec.c
> index abb50d32de..fa7eacfbe4 100644
> --- a/drivers/net/cnxk/cnxk_ethdev_sec.c
> +++ b/drivers/net/cnxk/cnxk_ethdev_sec.c

[ ... ]

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_cnxk_nix_inl_custom_profile_setup, 25.11)
> +int
> +rte_pmd_cnxk_nix_inl_custom_profile_setup(uint16_t portid,
> +					  const struct rte_pmd_cnxk_profile_cfg_params *cfg,
> +					  uint16_t *profile_id)
> +{
> +	uint64_t prot_field_cfg_val[RTE_PMD_CNXK_MAX_PROT_FIELDS] = {0};

Per coding style 3.1, avoid initializing values to zero unnecessarily.
The array is fully written to in the loop below. Is this initialization
needed?

> +	/* Pack protocol field inline configurations */
> +	for (i = 0; i < RTE_PMD_CNXK_MAX_PROT_FIELDS; i++)
> +		prot_field_cfg_val[i] = cnxk_rx_prot_field_inline_cfg_pack(&cfg->prot_field_cfg[i]);
> +
> +	/* Get SA size and max SA count */
> +	sa_size = cfg->sa_size;
> +	max_sa = cfg->max_sa;

Does this code validate that cfg is non-NULL before dereferencing it in
the for loop above?

[ ... ]

> diff --git a/drivers/net/cnxk/rte_pmd_cnxk.h b/drivers/net/cnxk/rte_pmd_cnxk.h
> index d344137dd5..1960288cef 100644
> --- a/drivers/net/cnxk/rte_pmd_cnxk.h
> +++ b/drivers/net/cnxk/rte_pmd_cnxk.h

[ ... ]

> +/**
> + * R
Previous message (by thread): |Smoke |SUCCESS| pw(163596) sid(38052) job(PER_PATCH_BUILD18712-20260506002139) [v2] ring: add cache guard after ring elements table
Next message (by thread): |PENDING| pw163597-163602 [PATCH] [v12,6/6] test: add flow parser fu
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the test-report mailing list