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

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-01-11 at 04:29:27, Jakub Kicinski (kuba@kernel.org) wrote:
> 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?
ACK.

>
> > +
> > +		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]?
ACK.

>
> > +	}
> > +
> > +	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?
ACK.
>
> [ ... ]
>
> > +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?
ACK.