|SUCCESS| [2/2] net/cnxk: add FEC get set and capability ops

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163122

_AI Code Review_

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

git sha: 20260416120031.3553798-1-rkudurumalla at marvell.com
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>
Subject: [PATCH 1/2] common/cnxk: add FEC configuration support

This patch adds Forward Error Correction (FEC) configuration and query
support through new ROC APIs. The implementation uses CGX mailbox
messages to interact with firmware for FEC parameter management on PF
interfaces.

> diff --git a/drivers/common/cnxk/roc_nix.h b/drivers/common/cnxk/roc_nix.h
> index 8ba8b3e0b6..6130e4c42b 100644
> --- a/drivers/common/cnxk/roc_nix.h
> +++ b/drivers/common/cnxk/roc_nix.h
> @@ -975,6 +975,8 @@ int __roc_api roc_nix_mac_link_info_set(struct roc_nix *roc_nix,
>  					struct roc_nix_link_info *link_info);
>  int __roc_api roc_nix_mac_link_info_get(struct roc_nix *roc_nix,
>  					struct roc_nix_link_info *link_info);
> +int __roc_api roc_nix_mac_fec_set(struct roc_nix *roc_nix, int fec);
> +int __roc_api roc_nix_mac_fec_supported_get(struct roc_nix *roc_nix, uint64_t *supported_fec);

Does this line exceed 79 characters? The coding style guide requires
wrapping at 79 characters.

[ ... ]

> diff --git a/drivers/common/cnxk/roc_nix_mac.c b/drivers/common/cnxk/roc_nix_mac.c
> index 376ff48522..4f856677e0 100644
> --- a/drivers/common/cnxk/roc_nix_mac.c
> +++ b/drivers/common/cnxk/roc_nix_mac.c
> @@ -257,6 +257,57 @@ roc_nix_mac_link_state_set(struct roc_nix *roc_nix, uint8_t up)
>  	return rc;
>  }
>  
> +int
> +roc_nix_mac_fec_set(struct roc_nix *roc_nix, int fec)
> +{
> +	struct nix *nix = roc_nix_to_nix_priv(roc_nix);
> +	struct dev *dev = &nix->dev;
> +	struct mbox *mbox = mbox_get(dev->mbox);
> +	struct fec_mode *req;
> +	int rc = -ENOSPC;
> +
> +	if (roc_nix_is_vf_or_sdp(roc_nix)) {
> +		rc = NIX_ERR_OP_NOTSUP;
> +		goto exit;
> +	}
> +
> +	req = mbox_alloc_msg_cgx_set_fec_param(mbox);
> +	if (req == NULL)
> +		goto exit;

Does this code use explicit NULL comparison? The coding style guide
requires if (!req) instead of if (req == NULL).

> +	req->fec = fec;
> +
> +	rc = mbox_process(mbox);
> +exit:
> +	mbox_put(mbox);
> +	return rc;
> +}
> +
> +int
> +roc_nix_mac_fec_supported_get(struct roc_nix *roc_nix, uint64_t *supported_fec)
> +{
> +	struct nix *nix = roc_nix_to_nix_priv(roc_nix);
> +	struct dev *dev = &nix->dev;
> +	struct mbox *mbox = mbox_get(dev->mbox);
> +	struct cgx_fw_data *rsp = NULL;

Does this code use explicit NULL initialization? According to coding
style guidelines, this should be avoided unless necessary.

> +	int rc;
> +
> +	if (roc_nix_is_vf_or_sdp(roc_nix)) {
> +		rc = NIX_ERR_OP_NOTSUP;
> +		goto exit;
> +	}
> +
> +	mbox_alloc_msg_cgx_get_aux_link_info(mbox);
> +	rc = mbox_process_msg(mbox, (void *)&rsp);

Does this code check the return value of
mbox_alloc_msg_cgx_get_aux_link_info()? Can this allocation fail,
potentially causing a crash in mbox_process_msg()?

> +	if (rc)
> +		goto exit;
> +
> +	*supported_fec = rsp->fwdata.supported_fec;

Does this code validate the rsp pointer before dereferencing it?
Can mbox_process_msg() return 0 with rsp still NULL?

> +	rc = 0;
> +exit:
> +	mbox_put(mbox);
> +	return rc;
> +}

[ ... ]

git sha: 20260416120031.3553798-2-rkudurumalla at marvell.com
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>
Subject: [PATCH 2/2] net/cnxk: add FEC get set and capability ops

This patch implements the ethdev FEC operations for the cnxk NIX driver,
including capability reporting, current mode query, and FEC mode
configuration functionality.

> diff --git a/drivers/net/cnxk/cnxk_ethdev_ops.c b/drivers/net/cnxk/cnxk_ethdev_ops.c
> index 49e77e49a6..a45721d414 100644
> --- a/drivers/net/cnxk/cnxk_ethdev_ops.c
> +++ b/drivers/net/cnxk/cnxk_ethdev_ops.c
> @@ -1414,3 +1414,97 @@ cnxk_nix_tx_descriptor_dump(const struct rte_eth_dev *eth_dev, uint16_t qid, uin
>  
>  	return roc_nix_sq_desc_dump(nix, qid, offset, num, file);
>  }
> +
> +static uint32_t
> +cnxk_roc_fec_to_ethdev_capa(int roc_fec)
> +{
> +	switch (roc_fec) {
> +	case ROC_FEC_BASER:
> +		return RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +	case ROC_FEC_RS:
> +		return RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +	default:
> +		return RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
> +	}
> +}

Does this function handle all possible roc_fec values? Are there other
ROC_FEC_* values that should be handled explicitly rather than falling
through to the default case?

> +
> +static int
> +cnxk_ethdev_fec_to_roc(uint32_t fec_capa)
> +{
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(RS))
> +		return ROC_FEC_RS;
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(BASER))
> +		return ROC_FEC_BASER;
> +	return ROC_FEC_NONE;
> +}

Does this function validate that only one FEC mode bit is set in
fec_capa? Can multiple bits be set simultaneously, and if so, what is
the expected behavior?

> +
> +static uint32_t
> +cnxk_fec_capa_from_supported(uint64_t supported_fec)
> +{
> +	uint32_t capa = RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) | RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
> +
> +	if (supported_fec & (1ULL << ROC_FEC_BASER))
> +		capa |= RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
> +	if (supported_fec & (1ULL << ROC_FEC_RS))
> +		capa |= RTE_ETH_FEC_MODE_CAPA_MASK(RS);
> +
> +	return capa;
> +}

Does this line exceed 79 characters?

> +
> +int
> +cnxk_nix_fec_get_capability(struct rte_eth_dev *eth_dev, struct rte_eth_fec_capa *speed_fec_capa,
> +			    unsigned int num)

Does this line exceed 79 characters?

> +{
> +	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> +	struct roc_nix *nix = &dev->nix;
> +	struct roc_nix_link_info link_info;
> +	uint64_t supported_fec = 0;
> +	int rc;
> +
> +	rc = roc_nix_mac_fec_supported_get(nix, &supported_fec);
> +	if (rc == 0 && supported_fec != 0) {

Does this code use explicit comparison with 0? The coding style guide
prefers if (!rc && supported_fec).

> +		rc = roc_nix_mac_link_info_get(nix, &link_info);
> +		if (rc)
> +			return rc;
> +
> +		if (speed_fec_capa == NULL || num == 0)

Does this code use explicit NULL comparison? Should be
if (!speed_fec_capa || !num).

> +			return 1;
> +
> +		speed_fec_capa[0].speed = link_info.speed;
> +		speed_fec_capa[0].capa = cnxk_fec_capa_from_supported(supported_fec);
> +		return 1;
> +	}
> +
> +	return rc;

Does this function always return meaningful error codes? When
roc_nix_mac_fec_supported_get() succeeds but supported_fec is 0,
does it return 0, and is that the correct return value for indicating
no capabilities?

> +}
> +
> +int
> +cnxk_nix_fec_get(struct rte_eth_dev *eth_dev, uint32_t *fec_capa)
> +{
> +	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> +	struct roc_nix *nix = &dev->nix;
> +	struct roc_nix_link_info link_info;
> +	int rc;
> +
> +	rc = roc_nix_mac_link_info_get(nix, &link_info);
> +	if (rc)
> +		return rc;
> +
> +	*fec_capa = cnxk_roc_fec_to_ethdev_capa(link_info.fec);
> +	return 0;
> +}

Does this function validate the fec_capa pointer before dereferencing it?

> +
> +int
> +cnxk_nix_fec_set(struct rte_eth_dev *eth_dev, uint32_t fec_capa)
> +{
> +	struct cnxk_eth_dev *dev = cnxk_eth_pmd_priv(eth_dev);
> +	struct roc_nix *nix = &dev->nix;
> +	int roc_fec;
> +
> +	if (fec_capa & RTE_ETH_FEC_MODE_CAPA_MASK(AUTO))
> +		roc_fec = ROC_FEC_RS;
> +	else
> +		roc_fec = cnxk_ethdev_fec_to_roc(fec_capa);
> +
> +	return roc_nix_mac_fec_set(nix, roc_fec);
> +}

Does this function validate that the requested FEC mode is supported
before attempting to set it? Can unsupported modes cause unexpected
behavior in roc_nix_mac_fec_set()?