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

[Jakub Kicinski]

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 support for CN20K,
enabling defragmentation by allowing entries to be moved while
maintaining stable virtual indexes for callers.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> index 62b4d657c369..642336b0d476 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_modify(struct rvu *rvu, u16 vidx, u16 new_midx)
> +{
> +	u16 old_midx;
> +	void *map;
> +	int rc;
> +
> +	if (!npc_is_vidx(vidx)) {
> +		dev_err(rvu->dev,
> +			"%s: vidx(%u) does not map to proper mcam idx\n",
> +			__func__, vidx);
> +		return -ESRCH;
> +	}
> +
> +	map = xa_erase(&npc_priv.xa_vidx2idx_map, vidx);
> +	if (!map) {
> +		dev_err(rvu->dev,
> +			"%s: vidx(%u) could not be deleted from vidx2idx map\n",
> +			__func__, vidx);
> +		return -ESRCH;
> +	}
> +
> +	old_midx = xa_to_value(map);
> +
> +	rc = xa_insert(&npc_priv.xa_vidx2idx_map, vidx,
> +		       xa_mk_value(new_midx), GFP_KERNEL);
> +	if (rc) {
> +		dev_err(rvu->dev,
> +			"%s: vidx(%u) cannot be added to vidx2idx map\n",
> +			__func__, vidx);
> +		goto fail1;
> +	}
> +
> +	map = xa_erase(&npc_priv.xa_idx2vidx_map, old_midx);
> +	if (!map) {
> +		dev_err(rvu->dev,
> +			"%s: old_midx(%u, vidx(%u)) cannot be added to idx2vidx map\n",
> +			__func__, old_midx, vidx);
> +		rc = -ESRCH;
> +		goto fail2;
> +	}
> +
> +	rc = xa_insert(&npc_priv.xa_idx2vidx_map, new_midx,
> +		       xa_mk_value(vidx), GFP_KERNEL);
> +	if (rc) {
> +		dev_err(rvu->dev,
> +			"%s: new_midx(%u, vidx(%u)) cannot be added to idx2vidx map\n",
> +			__func__, new_midx, vidx);
> +		goto fail3;
> +	}
> +
> +	return 0;
> +
> +fail3:
> +	if (xa_insert(&npc_priv.xa_idx2vidx_map, old_midx,
> +		      xa_mk_value(vidx), GFP_KERNEL))
> +		dev_err(rvu->dev,
> +			"%s: Error to roll back idx2vidx old_midx=%u vidx=%u\n",
> +			__func__, old_midx, vidx);
> +fail2:
> +	if (xa_erase(&npc_priv.xa_vidx2idx_map, vidx))
> +		dev_err(rvu->dev,
> +			"%s: Failed to roll back vidx2idx vidx=%u\n",
> +			__func__, vidx);
> +
> +fail1:
> +	if (xa_insert(&npc_priv.xa_idx2vidx_map, old_midx,
> +		      xa_mk_value(vidx), GFP_KERNEL))

The rollback at fail1 appears to operate on the wrong XArray. When
xa_insert() fails above and we jump to fail1, the xa_idx2vidx_map still
contains old_midx since the erase of xa_idx2vidx_map has not executed yet.
This xa_insert() will return -EBUSY because the key already exists.

Should this instead restore vidx to xa_vidx2idx_map with the original
old_midx value, since that was the map modified before the failure?

> +		dev_err(rvu->dev,
> +			"%s: Faled to roll back idx2vidx to old_midx=%u, vidx=%u\n",
> +			__func__, old_midx, vidx);
> +
> +	return rc;
> +}

[ ... ]