Re: [net-next,v3,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 pausible.

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
allocation schemes for contiguous and non-contiguous MCAM entry
allocation across 32 subbanks.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> index 498968bf4cf5..9360886c88e2 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c

[ ... ]

> +static int
> +npc_subbank_srch_order_parse_n_fill(struct rvu *rvu, char *options,
> +				    int num_subbanks)
> +{
> +	unsigned long w1 = 0, w2 = 0;
> +	char *p, *t1, *t2;
> +	int (*arr)[2];
> +	int idx, val;
> +	int cnt, ret;
> +
> +	cnt = 0;
> +
> +	options[strcspn(options, "\r\n")] = 0;
> +
> +	arr = kcalloc(num_subbanks, sizeof(*arr), GFP_KERNEL);
> +	if (!arr)
> +		return -ENOMEM;
> +
> +	while ((p = strsep(&options, " ,")) != NULL) {
> +		if (!*p)
> +			continue;
> +
> +		t1 = strsep(&p, "=");
> +		t2 = strsep(&p, "");

If the user input does not contain an '=' character, strsep() will return
NULL for t2 here.  The subsequent kstrtoint(t2, ...) call would then
dereference a NULL pointer.  Should there be a check for t2 being NULL
before using it?

> +
> +		if (strlen(t1) < 3) {
> +			dev_err(rvu->dev,
> +				"Token length is less than 3 (%s, %s)\n",
> +				t1, t2);
> +			goto err;
> +		}

[ ... ]

> +		ret = kstrtoint(t2, 10, &val);
> +		if (ret) {
> +			dev_err(rvu->dev,
> +				"Could not convert token to integer (%s)\n",
> +				t2);
> +			goto err;
> +		}
> +
> +		arr[cnt][0] = idx;
> +		arr[cnt][1] = val;
> +
> +		cnt++;

The loop writes to arr[cnt] before checking whether cnt exceeds
num_subbanks.  If a user provides more tokens than expected, this could
overflow the arr[] buffer.  The bounds check occurs after the loop
completes, but by then the overflow may have already happened.  Would
it make sense to check cnt < num_subbanks before writing to arr[cnt]?

> +	}
> +
> +	if (cnt != num_subbanks) {
> +		dev_err(rvu->dev,
> +			"Could find %u tokens, but exact %u tokens needed\n",
> +			cnt, num_subbanks);
> +		goto err;
> +	}

[ ... ]

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

[ ... ]

> +static int __maybe_unused
> +npc_add_to_pf_maps(struct rvu *rvu, u16 mcam_idx, int pcifunc)
> +{
> +	int rc, idx;
> +	void *map;
> +
> +	dev_dbg(rvu->dev,
> +		"%s:%d add2maps mcam_idx(%u) to xa_idx2pf map pcifunc=%#x\n",
> +		__func__, __LINE__, mcam_idx, pcifunc);
> +
> +	rc = xa_insert(&npc_priv.xa_idx2pf_map, mcam_idx,
> +		       xa_mk_value(pcifunc), GFP_KERNEL);
> +
> +	if (rc) {
> +		map = xa_load(&npc_priv.xa_idx2pf_map, mcam_idx);
> +		dev_err(rvu->dev,
> +			"%s:%d failed to insert mcam_idx(%u) to xa_idx2pf map, existing value=%lu\n",
> +			__func__, __LINE__, mcam_idx, xa_to_value(map));
> +		return -EFAULT;
> +	}
> +
> +	map = xa_load(&npc_priv.xa_pf_map, pcifunc);
> +	idx = xa_to_value(map);
> +
> +	rc = xa_insert(&npc_priv.xa_pf2idx_map[idx], mcam_idx,
> +		       xa_mk_value(pcifunc), GFP_KERNEL);
> +
> +	if (rc) {
> +		map = xa_load(&npc_priv.xa_pf2idx_map[idx], mcam_idx);
> +		dev_err(rvu->dev,
> +			"%s:%d failed to insert mcam_idx(%u) to xa_pf2idx_map map, earlier value=%lu idx=%u\n",
> +			__func__, __LINE__, mcam_idx, xa_to_value(map), idx);
> +		return -EFAULT;
> +	}

If the second xa_insert() fails, the entry inserted by the first
xa_insert() into xa_idx2pf_map is not cleaned up.  This leaves the two
maps in an inconsistent state.  Should the first entry be erased when
the second insert fails?

[ ... ]

> +static int npc_priv_init(struct rvu *rvu)
> +{
> +	struct npc_mcam *mcam = &rvu->hw->mcam;
> +	int blkaddr, num_banks, bank_depth;
> +	int num_subbanks, subbank_depth;
> +	u64 npc_const1, npc_const2 = 0;
> +	struct npc_subbank *sb;
> +	u64 cfg;
> +	int i;
> +
> +	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> +	if (blkaddr < 0) {
> +		dev_err(rvu->dev, "%s:%d NPC block not implemented\n",
> +			__func__, __LINE__);
> +		return -ENODEV;
> +	}
> +
> +	npc_const1 = rvu_read64(rvu, blkaddr, NPC_AF_CONST1);
> +	if (npc_const1 & BIT_ULL(63))
> +		npc_const2 = rvu_read64(rvu, blkaddr, NPC_AF_CONST2);
> +
> +	num_banks = mcam->banks;
> +	bank_depth = mcam->banksize;
> +
> +	num_subbanks = FIELD_GET(GENMASK_ULL(39, 32), npc_const2);
> +	npc_priv.num_subbanks = num_subbanks;
> +
> +	subbank_depth =	bank_depth / num_subbanks;

If npc_const1 does not have bit 63 set, npc_const2 remains 0, and
num_subbanks will be 0.  This would cause a division by zero here.
Should there be a check that num_subbanks is non-zero before this
division?