Re: [PATCH 1/1] octeontx2-af: Fix npc_delete_flow counter read for cn20k

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (2) · Low: 1 (1)

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] The fix is incomplete.
- [Low] The Fixes tag `Fixes: 5f83b4729440 ("octeontx2-af: Do not access
  match stats for cn20k")` does not resolve.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but it sits on
  the line the patch edits.
- [Medium] This problem wasn't introduced by this patch, but the patched
  assignment `rsp->cntr_val = rvu_read64(...)` in…
- [Low] In rvu_mbox_handler_npc_delete_flow() (rvu_npc_fs.c:1961-1962),
  the result of rvu_get_blkaddr() is checked for truthiness (`if…

--

octeontx2-af: Fix npc_delete_flow counter read for cn20k

This commit makes rvu_mbox_handler_npc_delete_flow() skip the
NPC_AF_MATCH_STATX read on cn20k silicon. On cn20k the MCAM match counter
cannot be read from that register, and counter support for cn20k is left
for later.

> Fixes: 5f83b4729440 ("octeontx2-af: Do not access match stats for cn20k")

[Severity: Low]
Is this Fixes: tag correct? 5f83b4729440 doesn't resolve to any commit in
the tree.

The unguarded read in rvu_mbox_handler_npc_delete_flow() came from
ec87f05402f5 ("octeontx2-af: Install TC filter rules in hardware based on
priority"). The is_cn20k() guards on the other NPC_AF_MATCH_STATX users
blame to 4e527f1e5c15 ("octeontx2-af: npc: cn20k: Add new mailboxes for
CN20K silicon").

Could this be a hash from an internal tree? If so, should the tag point to
one of the upstream commits above?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> index ec004ea5154cb..948a30c8fe008 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> @@ -1959,7 +1959,7 @@ int rvu_mbox_handler_npc_delete_flow(struct rvu *rvu,
>  			/* single rule */
>  			} else if (req->entry == iter->entry) {
>  				blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> -				if (blkaddr)
> +				if (blkaddr && !is_cn20k(rvu->pdev))

[Severity: High]
Is this fix complete for cn20k? rvu_dbg_npc_rx_miss_stats_display() still
reads NPC_AF_MATCH_STATX without an is_cn20k() check:

drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:rvu_dbg_npc_rx_miss_stats_display() {
    ...
	seq_printf(filp, "\t\tStat %d: \t%lld\n", mcam->rx_miss_act_cntr,
		   rvu_read64(rvu, blkaddr,
			      NPC_AF_MATCH_STATX(mcam->rx_miss_act_cntr)));
    ...
}

rvu_dbg_npc_init() creates the rx_miss_act_stats file on every silicon,
cn20k included.

On cn20k, rvu_npc_setup_interfaces() takes the cn20k branch and never
reserves a counter, so mcam->rx_miss_act_cntr stays 0. The RX loop also
hits "if (is_cn20k(rvu->pdev)) continue;" before it writes
NPC_AF_INTFX_MISS_STAT_ACT.

So reading this file on cn20k accesses STATX(0). The commit message says
that register cannot be read on cn20k, and the value printed is not an RX
miss count.

After this patch, this debugfs reader seems to be the only
NPC_AF_MATCH_STATX user in af/ that cn20k can still reach. Should it get
the same guard, or should the file not be created on cn20k?

[Severity: Low]
This isn't a bug introduced by this patch, but it is on the line being
changed. rvu_get_blkaddr() returns -ENODEV on failure, never 0, so
"if (blkaddr && ...)" lets a negative value through. Other callers in
these files check "blkaddr < 0".

I don't think it can trigger here. Rules only get onto mcam->mcam_rules
through rvu_mbox_handler_npc_install_flow() and
npc_install_mcam_drop_rule(), and both return early when blkaddr < 0.

Would it still be clearer to use blkaddr < 0 here, to match the other
callers?

>  					rsp->cntr_val = rvu_read64(rvu, blkaddr,
>  								   NPC_AF_MATCH_STATX(iter->cntr));

[Severity: Medium]
This is a pre-existing issue, but should this read also check
iter->has_cntr?

__rvu_mcam_remove_counter_from_rule() frees the counter and clears
has_cntr, but rule->cntr keeps the old index:

drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c:__rvu_mcam_remove_counter_from_rule() {
    ...
	free_req.cntr = rule->cntr;

	__npc_mcam_free_counter(rvu, &free_req, &free_rsp);
	rule->has_cntr = false;
}

That happens when a PF/VF installs a rule with set_cntr=1 and then
re-installs it with set_cntr=0, which reaches this in npc_install_flow():

	if (!req->set_cntr && rule->has_cntr)
		rvu_mcam_remove_counter_from_rule(rvu, owner, rule);

Another PF/VF can then get the freed index through the
npc_mcam_alloc_counter mailbox. When the first function deletes its rule,
rsp->cntr_val gets the other function's live match count.

A rule that never had a counter has cntr == 0, so it reads counter 0 the
same way.

rvu_dbg_npc_mcam_show_rules() already does "if (!iter->has_cntr)
continue;" before the same register read.

[Severity: Medium]
This isn't a bug introduced by this patch, but does this assignment
truncate the hardware counter? The value goes into a u16:

drivers/net/ethernet/marvell/octeontx2/af/mbox.h:
struct npc_delete_flow_rsp {
	struct mbox_msghdr hdr;
	u16 cntr_val;
};

rvu_mbox_handler_npc_mcam_counter_stats() treats the same register as
48 bits wide:

	rsp->stat = rvu_read64(rvu, blkaddr, NPC_AF_MATCH_STATX(req->cntr));
	rsp->stat &= BIT_ULL(48) - 1;

The PF TC code passes this value from otx2_del_mcam_flow_entry() back
through npc_install_flow_req's "u16 cntr_val; /* old counter value */".
npc_install_flow() then writes it into NPC_AF_MATCH_STATX(rule->cntr).

So any TC flower rule with more than 65535 hits loses the upper bits of
its count each time its entry is shifted or re-installed. Fixing this
would need a change to the mailbox message layout.

>  				list_move_tail(&iter->list, &del_list);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026035140.666684-1-vmukunda%40marvell.com