Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The primary default rule allocation still hardcodes X2 keys and unmasked reference indices, completely ignoring the X4 hardware profile for PFs and successful VF attempts. - [Medium] Caching device-specific hardware profile information in a global static variable causes a data race across concurrent device instances. --
commit 0a14e92cf2754f6d4cb9d8447914740b55c1a43e Author: Ratheesh Kannoth <[email protected]> octeontx2: cn20k: Respect NPC MCAM X2/X4 profile in flows and DFT alloc This commit updates the default CN20K NPC rule allocation and flow-install path to respect the active MCAM keyword width, dynamically selecting X2 or X4 and masking reference indices accordingly. It caches the profile information on the PF. > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c > b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c > index 67dfbe5ca903f..15b3f29d60ee9 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c > @@ -4467,11 +4467,18 @@ int npc_cn20k_dft_rules_alloc(struct rvu *rvu, u16 > pcifunc) > * as NPC_DFT_RULE_PRIO - 1 (higher hw priority) > */ > req.contig = false; > - req.kw_type = NPC_MCAM_KEY_X2; > req.count = cnt; > req.hdr.pcifunc = pcifunc; > req.ref_prio = NPC_MCAM_LOWER_PRIO; > - req.ref_entry = eidx + 1; > + > + if (npc_priv.kw == NPC_MCAM_KEY_X4) { > + req.kw_type = NPC_MCAM_KEY_X4; > + req.ref_entry = eidx & (npc_priv.bank_depth - 1); > + } else { > + req.kw_type = NPC_MCAM_KEY_X2; > + req.ref_entry = eidx; > + } > + Does the initial allocation attempt unconditionally hardcode X2 keys? Looking at the code just above this fallback path in npc_cn20k_dft_rules_alloc(), the primary allocation hardcodes the key type: req.contig = false; req.ref_prio = NPC_MCAM_HIGHER_PRIO; req.ref_entry = eidx; req.kw_type = NPC_MCAM_KEY_X2; ... ret = rvu_mbox_handler_npc_mcam_alloc_entry(rvu, &req, &rsp); If this first attempt succeeds, VFs might incorrectly get assigned X2 rules on X4 silicon. Since PFs bypass the fallback path entirely if this fails, will PF default rule allocation fail completely on X4 profiles? [ ... ] > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c > index 38cc539d724d8..5dd0591fed999 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_flows.c > @@ -37,14 +37,13 @@ static void otx2_clear_ntuple_flow_info(struct otx2_nic > *pfvf, struct otx2_flow_ > flow_cfg->max_flows = 0; > } > > -static int otx2_mcam_pfl_info_get(struct otx2_nic *pfvf, bool *is_x2, > - u16 *x4_slots) > +static int otx2_mcam_pfl_info_get(struct otx2_nic *pfvf, u16 *x4_slots, u8 > *kw_type) > { > struct npc_get_pfl_info_rsp *rsp; > struct msg_req *req; > static struct { > bool is_set; > - bool is_x2; > + u8 kw_type; > u16 x4_slots; > } pfl_info; Does caching this profile information in a static structure cause a data race across concurrent device instances? The accesses to this shared cache are protected by mutex_lock(&pfvf->mbox.lock). Since this is a per-device instance lock, multiple network devices could concurrently acquire their own independent locks and read or write the shared pfl_info structure. Could this lack of synchronization lead to torn reads, such as reading x4_slots as 0 if the compiler reorders the write to is_set before x4_slots? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
