Re: [PATCH v2 net-next] octeontx2-af: add tracepoints for NPC MCAM entry programming

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-09-27 at 10:16:17, netdev-bot+sashiko@kernel.org (netdev-bot+sashiko@kernel.org) wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 1 · Low: 3
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] In rvu_mbox_handler_npc_cn20k_mcam_alloc_and_write_entry()
>   (drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c), the patch…
Pre-existing issue.

> - [Medium] The commit message says the events are emitted from 'the
>   legacy and CN20K NPC alloc, config, read, and shift paths'.
Some cn20k path are covered as both uses same path. We will add more
traces in future if needed.

> - [Low] The otx2_npc_cam event (rvu_trace.h) defines and prints its
>   payload as w0_cam0/w0_cam1/w1_cam0/w1_cam1 and has no word-offset…
This is ok. We can debug with these word prints.

> - [Low] In the submitted patch, the 'v1 -> v2' changelog begins with a
>   '---' separator, and both Signed-off-by tags (Suman Ghosh, Ratheesh…
Hmm. My bad. should i respin ?

> - [Low] On CN20K, npc_cn20k_ref_idx_alloc() replaces the physical indices
>   in the returned mcam_idx list with virtual indices ('Return vidx to…
We have debugfs entries to map virtual to physical (and viceversa). so this
is ok.

>
> --
>
> octeontx2-af: add tracepoints for NPC MCAM entry programming
>
> This commit adds five ftrace events to the octeontx2 AF driver:
> otx2_npc_mcam_alloc_entries, otx2_npc_mcam_copy,
> otx2_npc_enable_mcam_entry, otx2_npc_cam and otx2_npc_action. They are
> emitted from the legacy and CN20K NPC MCAM alloc, config, read, enable
> and copy paths. The commit also zero-initializes the
> npc_mcam_alloc_entry_req used by the two alloc-and-write mailbox handlers.
>
> > Add ftrace events for MCAM allocation, CAM keyword writes, action/tag
> > programming, enable/disable, and entry copies. Emit them from the legacy
> > and CN20K NPC alloc, config, read, and shift paths so mailbox-driven
> > rule changes can be correlated with the programmed MCAM state.
>
> [Severity: Medium]
> Is the CN20K coverage described here complete?
>
> The copy and enable events are emitted only from the generic wrappers
> npc_copy_mcam_entry() and npc_enable_mcam_entry() in rvu_npc.c. The
> cam/action read events come only from the legacy npc_read_mcam_entry().
> Several CN20K paths call the raw helpers directly.
>
> npc_defrag_move_vdx_to_free() in cn20k/npc.c:
>
>     if (npc_cn20k_enable_mcam_entry(rvu, blkaddr, old_midx, false)) {
>     ...
>     if (npc_cn20k_copy_mcam_entry(rvu, blkaddr, old_midx, new_midx)) {
>     ...
>     if (npc_cn20k_enable_mcam_entry(rvu, blkaddr, new_midx, true)) {
>
> A defrag move therefore leaves no otx2_npc_mcam_copy or
> otx2_npc_enable_mcam_entry record. The trace still shows the rule at
> old_midx, but the hardware now holds it at new_midx.
>
> npc_cn20k_config_mcam_entry() does its disable-before-update and its
> final enable through npc_cn20k_enable_mcam_entry(). So it emits no
> enable/disable events, unlike the legacy npc_config_mcam_entry().
>
> npc_cn20k_dft_rules_free() disables default entries through
> npc_cn20k_enable_mcam_entry() without a trace.
>
> npc_cn20k_read_mcam_entry() is not instrumented at all. Its callers are
> rvu_mbox_handler_npc_cn20k_mcam_read_entry(),
> rvu_mbox_handler_npc_cn20k_read_base_steer_rule() and
> npc_update_dmac_value().
>
> The v2 changelog also says otx2_npc_mcam_copy was added "to cover MCAM
> entry shifts". Should these CN20K paths be traced too, or should the
> commit message be narrowed?
>
> [Severity: Low]
> This isn't a bug, but the applied commit has no Signed-off-by tags.
>
> In the submitted patch, the "v1 -> v2" changelog begins with a "---"
> separator. Both Signed-off-by lines (Suman Ghosh, Ratheesh Kannoth) come
> after it, so git am drops them along with the changelog.
>
> Could the Signed-off-by lines go above the first "---", with the version
> changelog after it?
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> > index 7c829b1da3e52..0b485728f3990 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
>
> [ ... ]
>
> > @@ -1018,43 +1021,46 @@ static void npc_cn20k_config_kw_x2(struct rvu *rvu, struct npc_mcam *mcam,
>
> [ ... ]
>
> > -	npc_cn20k_get_keyword(entry, kw + 3, &cam0, &cam1);
> > +	npc_cn20k_get_keyword(entry, kw + 3, &w3_cam0, &w3_cam1);
> >  	rvu_write64(rvu, blkaddr,
> >  		    NPC_AF_CN20K_MCAMEX_BANKX_CAMX_W3_EXT(index, bank, 1),
> > -		    cam1);
> > +		    w3_cam1);
> >  	rvu_write64(rvu, blkaddr,
> >  		    NPC_AF_CN20K_MCAMEX_BANKX_CAMX_W3_EXT(index, bank, 0),
> > -		    cam0);
> > +		    w3_cam0);
> > +
> > +	trace_otx2_npc_cam(actindex, bank, w0_cam0, w0_cam1, w1_cam0, w1_cam1);
> > +	trace_otx2_npc_cam(actindex, bank, w2_cam0, w2_cam1, w3_cam0, w3_cam1);
>
> [Severity: Low]
> The otx2_npc_cam event labels its payload as w0_cam0/w0_cam1/w1_cam0/
> w1_cam1 and has no word-offset field. Won't the second call here print
> the W2/W3 CAM values under the "w0_cam0 ... w1_cam1" labels, with the
> same mcam_index and bank as the first record?
>
> For X4 entries, npc_cn20k_config_kw_x4() emits four of these records per
> entry. The only way to tell the W0/W1 records from the W2/W3 records is
> emission order.
>
> Would a word index field, or a separate event for W2/W3, make these
> records unambiguous?
>
> [ ... ]
>
> > @@ -1434,7 +1445,7 @@ int rvu_mbox_handler_npc_cn20k_mcam_alloc_and_write_entry(struct rvu *rvu,
> >  {
> >  	struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, req->hdr.pcifunc);
> >  	struct npc_mcam_free_entry_req free_req = { 0 };
> > -	struct npc_mcam_alloc_entry_req entry_req;
> > +	struct npc_mcam_alloc_entry_req entry_req = { 0 };
>
> [Severity: High]
> With entry_req now zeroed, can NPC_CN20K_MCAM_ALLOC_AND_WRITE_ENTRY ever
> succeed on CN20K?
>
> The handler sets hdr.pcifunc, contig, ref_prio, ref_entry, count and
> virt, but never entry_req.kw_type. So kw_type is always 0, which is
> NPC_MCAM_KEY_X1 / NPC_MCAM_KEY_DYN.
>
> rvu_mbox_handler_npc_mcam_alloc_entry() passes req through unchanged,
> and npc_mcam_alloc_entries() rejects it on CN20K:
>
> rvu_mbox_handler_npc_cn20k_mcam_alloc_and_write_entry()
>   rvu_mbox_handler_npc_mcam_alloc_entry()
>     npc_mcam_alloc_entries()
> 	/* Only x2 or x4 key types are accepted */
> 	if (req->kw_type != NPC_MCAM_KEY_X2 && req->kw_type != NPC_MCAM_KEY_X4)
> 		return NPC_MCAM_INVALID_REQ;
>
> Before this patch, kw_type was uninitialized stack, so the outcome
> depended on whatever happened to be there. Now every request fails with
> NPC_MCAM_INVALID_REQ before any entry is allocated or programmed.
>
> The missing kw_type assignment is older than this patch. However, the
> v2 change to zero the struct is what makes the failure deterministic.
>
> Should entry_req.kw_type be set here instead, for example from
> req->req_kw_type or from the profile key type?
>
> The legacy rvu_mbox_handler_npc_mcam_alloc_and_write_entry() in
> rvu_npc.c got the same "= { 0 }" change and also never sets kw_type. It
> would fail the same way if reached on CN20K silicon.
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_trace.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu_trace.h
> > index 4cd0fc4b0d203..ea1c5d7f10076 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_trace.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_trace.h
> > @@ -159,6 +159,120 @@ TRACE_EVENT(otx2_parse_dump,
> >  		      __entry->w3, __entry->w4, __entry->w5)
> >  );
> >
> > +TRACE_EVENT(otx2_npc_mcam_alloc_entries,
>
> [ ... ]
>
> > +	    TP_fast_assign(__entry->pcifunc = pcifunc;
> > +			   __entry->contig = req->contig;
> > +			   __entry->ref_prio = req->ref_prio;
> > +			   __entry->ref_entry = req->ref_entry;
> > +			   __entry->req_count = req->count;
> > +			   __entry->kw_type = req->kw_type;
> > +			   __entry->virt = req->virt;
> > +			   __entry->start_mcam_idx = req->contig ? rsp->entry :
> > +				(rsp->count ? rsp->entry_list[0] :
> > +				 NPC_MCAM_ENTRY_INVALID);
>
> [Severity: Low]
> On CN20K, can rsp->entry_list[0] be a virtual index here rather than a
> physical MCAM index?
>
> npc_cn20k_ref_idx_alloc() returns vidx values when defrag_candidate
> (!ref_valid && !contig && virt) is true:
>
> 	/* Return vidx to caller */
> 	mcam_idx[i] = vidx;
>
> npc_mcam_alloc_entries() converts these with npc_cn20k_vidx2idx() for
> its own bookkeeping, but it passes the unconverted rsp to
> trace_otx2_npc_mcam_alloc_entries(). In that case rsp_start_mcam_idx
> holds a vidx.
>
> Meanwhile otx2_npc_cam, otx2_npc_action, otx2_npc_mcam_copy and
> otx2_npc_enable_mcam_entry all record physical indices. No event records
> the vidx to physical mapping.
>
> Could the event record the physical index, or both, so the alloc record
> can be matched to the later programming records?
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923034858.1764461-1-rkannoth%40marvell.com