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

[Ratheesh Kannoth]

On 2026-06-02 at 11:33:52, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) wrote:
> Improve MCAM visibility and field debugging for CN20K NPC.
>
> - Extend "mcam_layout" to show enabled (+) or disabled state per entry
>   so status can be verified without parsing the full "mcam_entry" dump.
> - Add "dstats" debugfs entry: for enabled MCAM indices, print hit deltas
>   since the prior read by comparing hardware counters to a per-entry
>   software baseline and advancing that baseline after each read (hardware
>   counters are not cleared).
> - Add "mismatch" debugfs entry: lists MCAM entries that are enabled
>   but not explicitly allocated, helping diagnose allocation/field issues.
>
> Signed-off-by: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260602060359.1894952-1-rkannoth%40marvell.com says

>> +static DEFINE_MUTEX(stats_lock);
>> +
>> +/* MAX_NUM_BANKS, MAX_SUBBANK_DEPTH and MAX_NUM_SUB_BANKS represent
>> + * hard limit on all silicon variants, preventing any possibility of
>> + * out-of-bounds access.
>> + */
>> +static u64 dstats[MAX_NUM_BANKS][MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS] = {};
>The dstats array is a file-scope static and is never reset on driver
>teardown. npc_cn20k_deinit() now clears npc_priv.en_map but leaves
>dstats alone:
>  void npc_cn20k_deinit(struct rvu *rvu)
>  {
>      ...
>      bitmap_clear(npc_priv.en_map, 0, MAX_NUM_BANKS * MAX_NUM_SUB_BANKS *
>                   MAX_SUBBANK_DEPTH);
>  }
>On a probe -> remove -> probe sequence (or after an NPC reset) within
>the same module load, do the stale baselines from the previous device
>incarnation remain in dstats[][]?
>The wrap-around guard only self-corrects when the new hardware counter
>is smaller than the stored baseline:
>  if (stats < dstats[bank][idx])
>      dstats[bank][idx] = 0;
>If the post-reinit counter is greater than the stale value, would the
>first read of dstats report stats - stale_baseline rather than the true
>post-init delta? The commit message states baselines are advanced
>"after each read", which seems to imply per-incarnation correctness.
>Note: this appears to be addressed later in the same series by the
>patch "octeontx2-af: npc: cn20k: Allocate npc_priv and dstats
>dynamically" (4767c16e8909), which moves dstats to a devm_kzalloc()
>allocation that is freshly zeroed at each probe.
There is only one AF device per system. So this case wont happen.

>> +	mutex_lock(&stats_lock);
>> +	seq_puts(s, "idx\tpfunc\tstats\n");
>> +	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;
>> +
>> +			if (npc_mcam_idx_2_key_type(rvu, mcam_idx, &key_type))
>> +				continue;
>> +
>> +			if (key_type == NPC_MCAM_KEY_X4 && bank != 0)
>> +				continue;
>> +
>> +			if (!test_bit(mcam_idx, npc_priv->en_map))
>> +				continue;
>> +
>> +			stats = rvu_read64(rvu, blkaddr,
>> +					   NPC_AF_CN20K_MCAMEX_BANKX_STAT_EXT(idx, bank));
>Can idx exceed the second dimension of dstats[][] here? The inner loop
>runs idx from npc_priv->bank_depth - 1 down to 0, but dstats is sized
>MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS (8192).
>In npc_priv_init() the new validation only constrains the quotient:
>  subbank_depth = bank_depth / num_subbanks;
>  if (subbank_depth > MAX_SUBBANK_DEPTH) {
>      ...
>      return -EINVAL;
>  }
>Because of integer truncation, bank_depth values in the range
>(MAX_NUM_SUB_BANKS * MAX_SUBBANK_DEPTH, MAX_NUM_SUB_BANKS *
>MAX_SUBBANK_DEPTH + num_subbanks - 1] (for example 8193..8223 with
>num_subbanks=32) pass this check while exceeding 8192.
>mcam->banksize is sourced from a 16-bit hardware-reported field, so the
>value is structurally allowed even if current silicon programs a
>power-of-2 banksize. With such a banksize, would the new
>set_bit(index, npc_priv.en_map) in npc_cn20k_enable_mcam_entry() (which
>only validates index < mcam->total_entries) and the dstats[bank][idx]
>indexing here write past the bitmap and array?
>The new comment in cn20k/npc.h states:
>  /* MAX_NUM_BANKS, MAX_SUBBANK_DEPTH and MAX_NUM_SUB_BANKS represent
>   * hard limit on all silicon variants, preventing any possibility of
>   * out-of-bounds access on matrix defined using these values.
>   */
>Would an explicit check such as bank_depth > MAX_NUM_SUB_BANKS *
>MAX_SUBBANK_DEPTH (or mcam->total_entries against the bitmap size) make
>the comment match the code?
>> +			if (!stats)
>> +				continue;
>> +			if (stats == dstats[bank][idx])
>> +				continue;
>> +
>> +			if (stats < dstats[bank][idx])
>> +				dstats[bank][idx] = 0;
>> +
npc_priv_init() during probe() do check against limits MAX_SUBBANK_DEPTH, MAX_NUM_SUB_BANKS.
So the condition wont happen.