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 <[email protected]> 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/[email protected]?part=1
