Re: [PATCH v12 net-next 5/9] octeontx2-af: npc: cn20k: add subbank search order control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing xa_erase() during search order update causes stale duplicate entries in tracking xarrays.
- [High] Missing error rollback in npc_cn20k_search_order_set() leaves internal allocator structures in a permanently corrupted state.
- [Low] Format string mismatch in rvu_af_dl_npc_srch_order_validate() causes potentially garbled logs.
--

commit 360483b7660f8774a4adf83ec93cc5565c7337b4
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2-af: npc: cn20k: add subbank search order control

This commit adds a runtime parameter srch_order to control the search
order of MCAM subbanks during allocation in the octeontx2 driver. It
allows configuring the priority of the 32 subbanks for the CN20K NPC.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> index e9aad0ad3fa68..6f8f42234b068 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
[ ... ]
> @@ -3906,6 +3906,56 @@ static void npc_unlock_all_subbank(void)
>  		mutex_unlock(&npc_priv.sb[i].lock);
>  }
>  
> +int npc_cn20k_search_order_set(struct rvu *rvu,
> +			       u64 narr[MAX_NUM_SUB_BANKS], int cnt)
> +{
> +	struct npc_mcam *mcam = &rvu->hw->mcam;
> +	struct npc_subbank *sb;
> +	struct xarray *xa;
> +	int sb_idx, rc;
> +
> +	if (cnt != npc_priv.num_subbanks) {
> +		dev_err(rvu->dev, "Number of entries(%u) != %u\n",
> +			cnt, npc_priv.num_subbanks);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&mcam->lock);
> +	npc_lock_all_subbank();
> +	restrict_valid = false;
> +
> +	for (sb_idx = 0; sb_idx < cnt; sb_idx++) {
> +		sb = &npc_priv.sb[sb_idx];
> +
> +		xa = &npc_priv.xa_sb_free;
> +		if (sb->flags & NPC_SUBBANK_FLAG_USED)
> +			xa = &npc_priv.xa_sb_used;
> +
> +		sb->arr_idx = narr[sb_idx];
> +
> +		rc = xa_err(xa_store(xa, sb->arr_idx,
> +				     xa_mk_value(sb_idx), GFP_KERNEL));

If the new index assignment swaps indices between a free subbank and a used
subbank, is the old index in the original xarray ever overwritten?

Since xa_erase() is not called on the old arr_idx, this might leave the
subbank mapped at both its old and new arr_idx if they don't overwrite each
other.

Can this result in stale duplicate entries?

> +		if (rc)
> +			goto fail;

If xa_store() fails (for example, due to -ENOMEM from GFP_KERNEL) and returns
an error, the code jumps to the fail label. Are the successful modifications
made to previously processed subbanks rolled back?

If not, could this leave the system in a torn state where restrict_valid is
false, and some subbanks have updated indices while others keep their old
ones?

> +	}
> +
> +	for (int i = 0; i < cnt; i++)
> +		subbank_srch_order[i] = (u32)narr[i];
> +
> +fail:
> +	npc_unlock_all_subbank();
> +	mutex_unlock(&mcam->lock);
> +
> +	return rc;
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
> index a42404e6db7cc..aa3ecab5ebd88 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_devlink.c
[ ... ]
> @@ -1619,12 +1620,83 @@ static int rvu_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
[ ... ]
> +static int rvu_af_dl_npc_srch_order_validate(struct devlink *devlink, u32 id,
> +					     union devlink_param_value *val,
> +					     struct netlink_ext_ack *extack)
> +{
> +	struct rvu_devlink *rvu_dl = devlink_priv(devlink);
> +	struct rvu *rvu = rvu_dl->rvu;
> +	bool restricted_order;
> +	unsigned long w = 0;
> +	u64 *arr;
> +	u32 sz;
> +
> +	npc_cn20k_search_order_get(&restricted_order, &sz);
> +	if (sz != val->u64arr.size) {
> +		dev_err(rvu->dev,
> +			"Wrong size %llu, should be %u\n",
> +			val->u64arr.size, sz);

The size field in devlink array values is a 32-bit unsigned integer (u32),
but the error message uses the %llu format specifier.

On some architectures, could this cause printk to consume 8 bytes from the
arguments and read garbage from adjacent memory?

> +		return -EINVAL;
> +	}
> +
> +	arr = val->u64arr.val;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508034912.4082520-1-rkannoth@marvell.com?part=5