Re: [PATCH v9 net-next 6/6] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-04-02 at 14:24:42, Paolo Abeni (pabeni@redhat.com) wrote:
> On 3/30/26 7:31 AM, Ratheesh Kannoth wrote:
> Also there are several remarks for sashiko, some of them looks valid:
>
> https://sashiko.dev/#/patchset/20260330053105.2722453-1-rkannoth%40marvell.com
>
> please a look.

> +static u64 dstats[MAX_NUM_BANKS][MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS] = {};
> +static int npc_mcam_dstats_show(struct seq_file *s, void *unused)
> +{
[ ... ]
> +			stats = rvu_read64(rvu, blkaddr,
> +					   NPC_AF_CN20K_MCAMEX_BANKX_STAT_EXT(idx, bank));
> +			if (!stats)
> +				continue;
> +			if (stats == dstats[bank][idx])
> +				continue;
[ ... ]
> +			seq_printf(s, "%u\t%#04x\t%llu\n",
> +				   mcam_idx, pf, delta);
> +			dstats[bank][idx] = stats;
> +		}
> +	}
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(npc_mcam_dstats);

"Can this cause truncated output if the printed data exceeds the initial
seq_file buffer?
When seq_read() encounters a buffer overflow (typically at 4KB), it
discards the buffer, allocates a larger one, and re-invokes the show
callback from the beginning. Since the global dstats array is updated
during the first partial run, won't the retry skip those previously
processed entries due to the "stats == dstats[bank][idx]" check?
Also, since the dstats array is modified here without any locks, will
concurrent reads of this debugfs file race and corrupt the statistics?"

dtsats is for debugging. Evenif the first time it fails, second time it will
show the correct stats as live traffic will update stats.

>  free_rules:
> +	blkaddr = rvu_get_blkaddr(rvu, BLKTYPE_NPC, 0);
> +	if (blkaddr < 0)
> +		return;
>
> -	free_req.hdr.pcifunc = pcifunc;
> -	free_req.all = 1;
> -	rc = rvu_mbox_handler_npc_mcam_free_entry(rvu, &free_req, &rsp);
> -	if (rc)
> -		dev_err(rvu->dev,
> -			"%s: Error deleting default entries (pcifunc=%#x\n",
> -			__func__, pcifunc);
> +	for (int i = 0; i < 4; i++) {
> +		if (ptr[i] == USHRT_MAX)
> +			continue;
> +
> +		mutex_lock(&mcam->lock);
> +		npc_mcam_clear_bit(mcam, ptr[i]);
"Could this lead to a double-free corrupting the free-count metadata?
If a VF or DPDK application manually frees a default MCAM entry prior to
teardown using the NPC_MCAM_FREE_ENTRY mailbox message, the bit is cleared.
When NIX LF teardown runs here, it calls npc_mcam_clear_bit() again. Because
npc_mcam_clear_bit() unconditionally increments mcam->bmap_fcnt++, doing so
twice on the same index corrupts the free-count metadata."

default entries are alloated and freed by kernel.

>
> Thanks,
>
> Paolo
>