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 <[email protected]> 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/[email protected]?part=5
