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

[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: Index management

This patch adds MCAM index management for CN20K silicon, implementing
contiguous and non-contiguous allocation schemes across the new subbank
architecture with reference, limit, and priority parameters.

> 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..de3d5ad762d1
> --- /dev/null
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c

[ ... ]

> +static int __npc_subbank_alloc(struct rvu *rvu, struct npc_subbank *sb,
> +			       int key_type, int ref, int limit, int prio,
> +			       bool contig, int count, u16 *mcam_idx,
> +			       int idx_sz, bool max_alloc, int *alloc_cnt)
> +{
> +	int cnt, t, b, i, blkaddr;
> +	bool new_sub_bank = false;
> +	unsigned long *bmap;
> +	u16 *save = NULL;
> +	int sidx, eidx;
> +	bool diffbank;
> +	int bw, bfree;
> +	int rc = 0;
> +	bool ret;

[ ... ]

> +	/* Mark new subbank bank as used */
> +	if (new_sub_bank) {
> +		blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> +		if (blkaddr < 0) {
> +			dev_err(rvu->dev,
> +				"%s: NPC block not implemented\n", __func__);
> +			goto err;
                        ^^^^^^^^

When rvu_get_blkaddr() fails here, the code jumps to the err label but rc
is still 0 from initialization. Every other error path in this function
sets rc before going to err. Should rc be set to blkaddr (or another error
code) before the goto?

> +		}
> +
> +		rc =  __npc_subbank_mark_used(rvu, sb, key_type);

[ ... ]

> +err:
> +	kfree(save);
> +	return rc;
> +}

[ ... ]

> +static void npc_subbank_init(struct rvu *rvu, struct npc_subbank *sb, int idx)
> +{
> +	mutex_init(&sb->lock);

[ ... ]

> +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);
> +	kfree(npc_priv.sb);

The subbank mutexes are initialized in npc_subbank_init() but are not
destroyed here. Is mutex_destroy() needed for each sb->lock before freeing
npc_priv.sb? The same issue applies to the fail2 error path in
npc_priv_init().

> +	kfree(subbank_srch_order);
> +}
-- 
pw-bot: cr