Re: [net-next,v7,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 support for virtual MCAM index allocation and improves
CN20K MCAM defragmentation handling. Virtual indexes allow the hardware
to move mapped MCAM entries internally, enabling defragmentation.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> --- 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;
> +	}

[ ... ]

> +fail1:
> +	if (xa_insert(&npc_priv.xa_idx2vidx_map, old_midx,
> +		      xa_mk_value(vidx), GFP_KERNEL))
> +		dev_err(rvu->dev,
> +			"%s: Faled to roll back vidx2idx to old_midx=%u, vidx=%u\n",
> +			__func__, old_midx, vidx);
> +
> +	return rc;
> +}

When we jump to fail1, it's because the xa_insert() into xa_vidx2idx_map
failed (around line 352). At that point, xa_idx2vidx_map hasn't been touched
yet - only xa_vidx2idx_map had the vidx erased. The rollback here tries to
insert into xa_idx2vidx_map, but shouldn't this restore vidx -> old_midx
in xa_vidx2idx_map instead? This appears to be operating on the wrong xarray
and would leave the mapping inconsistent on error.

This was also raised in lore review feedback:
https://lore.kernel.org/all/20260129051307.376632-1-kuba@kernel.org/

[ ... ]

> +static
> +int npc_defrag_move_vdx_to_free(struct rvu *rvu,
> +				struct npc_defrag_node *f,
> +				struct npc_defrag_node *v,
> +				int cnt, u16 *save)
> +{
> +	struct npc_mcam *mcam = &rvu->hw->mcam;

[ ... ]

> +		/* save pcifunc */
> +		map = xa_load(&npc_priv.xa_idx2pf_map, old_midx);
> +		pcifunc = xa_to_value(map);

[ ... ]

> +		/* Remove from mcam maps */
> +		mcam->entry2pfvf_map[old_midx] = NPC_MCAM_INVALID_MAP;
> +		mcam->entry2cntr_map[old_midx] = NPC_MCAM_INVALID_MAP;
> +		npc_mcam_clear_bit(mcam, old_midx);
> +
> +		mcam->entry2pfvf_map[new_midx] = pcifunc;
> +		mcam->entry2cntr_map[new_midx] = pcifunc;
                                            ^^^^^^^

Should entry2cntr_map[new_midx] be set to pcifunc here? Looking at all other
usages in the codebase, entry2cntr_map holds either NPC_MCAM_INVALID_MAP or
a counter index (set via npc_map_mcam_entry_and_cntr()). Setting it to pcifunc
appears to be a semantic type error since pcifunc is a PF/VF identifier, not
a counter index.

Later code in rvu_mbox_handler_npc_mcam_free_entry() retrieves this value and
uses it as a counter index:

    cntr = mcam->entry2cntr_map[req->entry];
    if (cntr != NPC_MCAM_INVALID_MAP)
        npc_unmap_mcam_entry_and_cntr(rvu, mcam, blkaddr, req->entry, cntr);

If the old entry had a counter mapped, that counter reference would be lost
during defragmentation, and if the pcifunc value happens to look like a valid
counter index, it would corrupt counter reference counts.

> +		npc_mcam_set_bit(mcam, new_midx);

[ ... ]