|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

I'll review this patch series against the DPDK coding standards and contribution guidelines.

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 APIs to the
cnxk common driver. The implementation adds two new ROC APIs for setting
FEC mode and querying supported FEC capabilities from firmware.

> 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)

Coding style: Does the NULL pointer comparison follow the implicit style?
Per the coding standards, comparisons with NULL should be done implicitly.
This should be written as:

	if (!req)

> +		goto exit;
> +	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;
> +	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);
> +	if (rc)
> +		goto exit;
> +
> +	*supported_fec = rsp->fwdata.supported_fec;

Does this code dereference rsp safely? In roc_nix_mac_fec_supported_get(),
what happens if mbox_process_msg() returns 0 but rsp is still NULL? Should
there be a NULL check on rsp before dereferencing rsp->fwdata.supported_fec?

[ ... ]

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,
adding support for querying FEC capabilities, getting current FEC mode,
and setting FEC configuration.

> 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);
> +	}
> +}

In cnxk_roc_fec_to_ethdev_capa(), does this handle all possible roc_fec
values correctly? Should there be a specific case for ROC_FEC_NONE to
distinguish it from default/unknown values?

> +
> +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;
> +}

In cnxk_ethdev_fec_to_roc(), what happens if both RS and BASER bits are
set in fec_capa? Does this priority order (RS preferred) match the
hardware capabilities and user expectations?

> +
> +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;
> +}

Coding style: In cnxk_fec_capa_from_supported(), the line with capa
initialization exceeds 79 characters. Does this line need to be wrapped?

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

Coding style: Does this function signature line exceed the 79 character
limit? The function declaration should be wrapped appropriately.

> +{
> +	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) {

Coding style: Are these comparisons using implicit style correctly? Per
the coding standards, these should be:

	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)

Coding style: Does this NULL comparison follow the implicit style? 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;

In cnxk_nix_fec_get_capability(), what return value is used when the
firmware query fails or supported_fec is 0? Does the rc value correctly
reflect success/failure for all paths through this function?

> +}
> +
> +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;
> +}
> +
> +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);
> +}