Re: [PATCH v12 net-next 1/9] octeontx2-af: npc: cn20k: debugfs enhancements

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] High-order kmalloc via single_open_size() will fail due to memory fragmentation.
- [Low] Unnecessary acquisition of `stats_lock` in `npc_mcam_mismatch_show()`.
- [Low] Unreachable dead code in `npc_mcam_dstats_show()` due to confusing logic.
--

commit 90a879ef1db5083da555645f4bdb9581acc62dc8
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2-af: npc: cn20k: debugfs enhancements

This commit improves MCAM visibility and field debugging for CN20K NPC.
It extends the mcam_layout debugfs entry, adds a dstats entry to report
recently hit MCAM indices, and adds a mismatch entry to list allocated
but unmapped entries.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> index 6f13296303cb5..9a4c2ea5b19e4 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c

[ ... ]

> +#define DEFINE_OCTEONTX2_DEBUGFS_ATTRIBUTE_WITH_SIZE(__name, __size)		\
> +static int __name ## _open(struct inode *inode, struct file *file)		\
> +{										\
> +	return single_open_size(file, __name ## _show, inode->i_private,	\
> +				__size);					\
> +}										\
> +__OCTEONTX2_DEBUGFS_ATTRIBUTE_FOPS(__name)
> +
> +static DEFINE_MUTEX(stats_lock);
> +
> +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)
> +{

[ ... ]

> +			if (!stats)
> +				continue;
> +			if (stats == dstats[bank][idx])
> +				continue;
> +
> +			if (stats < dstats[bank][idx])
> +				dstats[bank][idx] = 0;
> +
> +			pf = 0xFFFF;
> +			map = xa_load(&npc_priv->xa_idx2pf_map, mcam_idx);
> +			if (map)
> +				pf = xa_to_value(map);
> +
> +			if (stats > dstats[bank][idx])
> +				delta = stats - dstats[bank][idx];
> +			else
> +				delta = stats;

Is this else block reachable?

The loop previously skips to the next iteration if stats == dstats[bank][idx].
If stats < dstats[bank][idx], the code sets dstats[bank][idx] to 0. Since
stats is also checked to be non-zero earlier, stats will always be strictly
greater than dstats[bank][idx] by the time it reaches this condition.

> +
> +			snprintf(buff, sizeof(buff), "%u\t%#04x\t%llu\n",
> +				 mcam_idx, pf, delta);
> +			seq_puts(s, buff);
> +
> +			dstats[bank][idx] = stats;
> +		}
> +	}
> +
> +	mutex_unlock(&stats_lock);
> +	return 0;
> +}
> +
> +/*  "%u\t%#04x\t%llu\n" needs less than 64 characters to print */
> +#define TOTAL_SZ (MAX_NUM_BANKS * MAX_NUM_SUB_BANKS * MAX_SUBBANK_DEPTH * 64)
> +DEFINE_OCTEONTX2_DEBUGFS_ATTRIBUTE_WITH_SIZE(npc_mcam_dstats, TOTAL_SZ);

Will this single_open_size() allocation reliably succeed on a running system?

Because single_open_size() uses kmalloc(), it requires a physically
contiguous memory block. TOTAL_SZ is the product of multiple maximum
hardware limits, which can be hundreds of kilobytes. This could result
in a high-order allocation that fails with an out of memory error due to
fragmentation.

Would standard seq_open() be safer here, as it allows the buffer to grow
dynamically using kvmalloc()?

> +
> +static int npc_mcam_mismatch_show(struct seq_file *s, void *unused)
> +{
> +	struct npc_priv_t *npc_priv;
> +	struct npc_subbank *sb;
> +	int mcam_idx, sb_off;
> +	struct rvu *rvu;
> +	char buff[64];
> +	void *map;
> +	int rc;
> +
> +	npc_priv = npc_priv_get();
> +	rvu = s->private;
> +
> +	seq_puts(s, "index\tsb idx\tkw type\n");
> +	mutex_lock(&stats_lock);

Does this function need to acquire stats_lock?

The stats_lock mutex protects the static dstats array during dstats_show.
This loop only reads npc_priv->en_map (which is manipulated safely via
atomic bitops) and xa_idx2pf_map (which uses internal XArray RCU for safe
lookups). It does not appear to access dstats or any other shared state
that requires this mutex.

> +	for (int bank = npc_priv->num_banks - 1; bank >= 0; bank--) {
> +		for (int idx = npc_priv->bank_depth - 1; idx >= 0; idx--) {
> +			mcam_idx = bank * npc_priv->bank_depth + idx;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508034912.4082520-1-rkannoth@marvell.com?part=1