Re: [net-next,v4,09/13] octeontx2-af: npc: cn20k: virtual index support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
octeontx2-af: npc: cn20k: virtual index support

This patch adds virtual MCAM index allocation and improves CN20K MCAM
defragmentation. Virtual indexes allow the hardware to move mapped
MCAM entries internally while presenting stable indexes to callers.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> index d549e75b2cb0..334b80ebec69 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c

[ ... ]

> +static int npc_vidx_maps_add_entry(struct rvu *rvu, u16 mcam_idx, int pcifunc,
> +				   u16 *vidx)
> +{
> +	int rc, max, min;
> +	u32 id;
> +
> +	/* Virtual index start from maximum mcam index + 1 */
> +	max = npc_priv.bank_depth * 2 * 2 - 1;
> +	min = npc_priv.bank_depth * 2;
> +
> +	rc = xa_alloc(&npc_priv.xa_vidx2idx_map, &id,
> +		      xa_mk_value(mcam_idx),
> +		      XA_LIMIT(min, max), GFP_KERNEL);
> +	if (rc) {
> +		dev_err(rvu->dev,
> +			"%s: Failed to add to vidx2idx map (%u)\n",
> +			__func__, mcam_idx);
> +		return rc;
> +	}
> +
> +	rc = xa_insert(&npc_priv.xa_idx2vidx_map, mcam_idx,
> +		       xa_mk_value(id), GFP_KERNEL);
> +	if (rc) {
> +		dev_err(rvu->dev,
> +			"%s: Failed to add to idx2vidx map (%u)\n",
> +			__func__, mcam_idx);
> +		return rc;
                ^^^^^^^^^^

If xa_insert() fails here, the entry allocated by xa_alloc() in
xa_vidx2idx_map is not cleaned up. Should there be an xa_erase() call
to remove the allocated entry before returning the error?

> +	}
> +
> +	if (vidx)
> +		*vidx = id;
> +
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
> index 0f9953eaf1b0..cc83d4fc5724 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c

[ ... ]

> +static const struct devlink_param rvu_af_dl_param_defrag[] = {
> +	DEVLINK_PARAM_DRIVER(RVU_AF_DEVLINK_PARAM_ID_NPC_DEFRAG,
> +			     "npc_defrag", DEVLINK_PARAM_TYPE_STRING,
> +			     BIT(DEVLINK_PARAM_CMODE_RUNTIME),
> +			     rvu_af_npc_defrag_feature_get,
> +			     rvu_af_npc_defrag,
> +			     rvu_af_npc_defrag_feature_validate),
> +};
> +
>  int rvu_register_dl(struct rvu *rvu)
>  {

[ ... ]

> +	if (is_cn20k(rvu->pdev)) {
> +		err = devlink_params_register(dl, rvu_af_dl_param_defrag,
> +					      ARRAY_SIZE(rvu_af_dl_param_defrag));
> +		if (err) {
> +			dev_err(rvu->dev,
> +				"devlink defrag params register failed with error %d",
> +				err);
> +			goto err_dl_exact_match;
> +		}
> +	}

The rvu_af_dl_param_defrag parameters are registered here for CN20K
devices, but rvu_unregister_dl() does not have a corresponding
devlink_params_unregister() call for these parameters. Should there be
a matching unregister call similar to the exact_match handling?