Re: [net-next,v8,01/13] octeontx2-af: npc: cn20k: Index management

[Simon Horman <horms@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: Index management

This patch adds MCAM index allocation and management for CN20K silicon.
The implementation supports contiguous and non-contiguous allocation
schemes with reference, limit, and priority parameters.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h
> new file mode 100644
> index 000000000000..eb1f54d2bfce
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h

[ ... ]

> + * MCAM resource is divided horizontally into mutltiple subbanks and

This isn't a bug, but "mutltiple" should be "multiple".

[ ... ]

> + * @xa_pf2idx_map:	PF to mcam index map.
> + * @xa_idx2pf_map:	Mcam index to PF map.
> + * @xa_pf_map:		Pcifunc to index map.
> + * @pf_cnt:		Number of PFs.A

This isn't a bug, but there's a spurious trailing 'A' after "PFs." in
the kernel-doc comment.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> new file mode 100644
> index 000000000000..9b5da2665b54
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c

[ ... ]

> +int npc_cn20k_ref_idx_alloc(struct rvu *rvu, int pcifunc, int key_type,
> +			    int prio, u16 *mcam_idx, int ref, int limit,
> +			    bool contig, int count)
> +{

[ ... ]

> +add2map:
> +	for (i = 0; i < count; i++) {
> +		rc = npc_add_to_pf_maps(rvu, mcam_idx[i], pcifunc);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}

If npc_add_to_pf_maps() fails partway through the loop (when i > 0), does
this leak the MCAM indexes that were previously allocated?

The allocators (npc_subbank_ref_alloc, npc_subbank_noref_alloc,
npc_multi_subbank_ref_alloc) have already claimed the MCAM entries before
reaching add2map. When the loop fails at iteration i, the function returns
rc without freeing the allocated MCAM indexes or removing the map entries
that were successfully added in iterations 0 through i-1.

[ ... ]

> +static int
> +npc_priv_init(struct rvu *rvu)
> +{

[ ... ]

> +	if (num_subbanks & (num_subbanks - 1)) {
> +		dev_err(rvu->dev,
> +			"subbanks cnt(%u) should be a multiple of 2\n",
> +			num_subbanks);
> +		return -EINVAL;
> +	}

The error message says "multiple of 2" but the check
(num_subbanks & (num_subbanks - 1)) tests for power of 2. For example,
6 is a multiple of 2 but not a power of 2. Should the message say
"power of 2" instead?

[ ... ]

> +	npc_priv.xa_pf2idx_map = kcalloc(npc_priv.pf_cnt,
> +					 sizeof(struct xarray),
> +					 GFP_KERNEL);
> +	if (!npc_priv.xa_pf2idx_map)
> +		goto fail2;

[ ... ]

> +fail2:
> +	kfree(subbank_srch_order);
> +
> +fail1:
> +	xa_destroy(&npc_priv.xa_sb_used);
> +	xa_destroy(&npc_priv.xa_sb_free);
> +	xa_destroy(&npc_priv.xa_idx2pf_map);
> +	xa_destroy(&npc_priv.xa_pf_map);
> +	kfree(npc_priv.sb);
> +	return -ENOMEM;
> +}
> +
> +void npc_cn20k_deinit(struct rvu *rvu)
> +{
> +	int i;
> +
> +	xa_destroy(&npc_priv.xa_sb_used);
> +	xa_destroy(&npc_priv.xa_sb_free);
> +	xa_destroy(&npc_priv.xa_idx2pf_map);
> +	xa_destroy(&npc_priv.xa_pf_map);
> +
> +	for (i = 0; i < npc_priv.pf_cnt; i++)
> +		xa_destroy(&npc_priv.xa_pf2idx_map[i]);
> +
> +	kfree(npc_priv.xa_pf2idx_map);
> +	/* No need to destroy mutex lock as it is
> +	 * part of subbank structure
> +	 */
> +	kfree(npc_priv.sb);
> +	kfree(subbank_srch_order);
> +}

Can npc_cn20k_deinit() be called when npc_priv_init() fails at fail2?

When kcalloc for xa_pf2idx_map fails, fail2 frees subbank_srch_order
and npc_priv.sb. Then rvu_npc_freemem() calls npc_cn20k_deinit() which:

1. Iterates over npc_priv.xa_pf2idx_map[i] when xa_pf2idx_map is NULL
2. Calls kfree() on already-freed npc_priv.sb and subbank_srch_order

Should npc_cn20k_deinit() check npc_priv.init_done before cleanup, or
should fail2 set the freed pointers to NULL?