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

[Ratheesh Kannoth]

On 2026-05-08 at 09:19:08, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) wrote:
> CN20K NPC MCAM is split into 32 subbanks that are searched in a
> predefined order during allocation. Lower-numbered subbanks have
> higher priority than higher-numbered ones.
pw-bot: changes-requested

> +	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?
No. npc_cn20k_search_order_set() is only rearranging indexes in free and used subbank.
indexes wont move from free subbank to used subbank.

>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?
ACK, but, There is no way, we can gracefully rollback, as rollback can also fail due to memory
pressurem while reinserting old index using xa_store. This is fatal error, will add
a err print to alert user.

>> +					     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;
>> +	}
No. %llu is used for val->u64arr.size, which is 64bit.