Re: [PATCH v18 net-next 8/8] octeontx2-af: npc: cn20k: Allocate npc_priv and dstats dynamically.

[Ratheesh Kannoth]

On 2026-06-02 at 11:33:59, Ratheesh Kannoth (rkannoth@xxxxxxxxxxx) wrote:
> Replace the file-scope static npc_priv with a kcalloc'd struct filled
> from hardware bank/subbank geometry at init (num_banks is no longer a
> const compile-time constant; drop init_done and use a non-NULL
> npc_priv pointer for liveness). Thread npc_priv_get() / pointer access
> through the CN20K NPC code paths, extend teardown to kfree the root
> struct on failure and in npc_cn20k_deinit, and adjust MCAM section
> setup to use the discovered subbank count.
>
> Allocate MCAM debugfs dstats via devm_kzalloc instead of a static matrix,
> and use the allocated backing store consistently when computing deltas
> (including the counter rollover compare).
>
> Signed-off-by: Ratheesh Kannoth <rkannoth@xxxxxxxxxxx>

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

>> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
>[ ... ]
>> @@ -397,6 +398,10 @@ int npc_cn20k_debugfs_init(struct rvu *rvu)
>>  	debugfs_create_file("vidx2idx", 0444, rvu->rvu_dbg.npc,
>>  			    npc_priv, &npc_vidx2idx_map_fops);
>>
>> +	dstats = devm_kzalloc(rvu->dev, sizeof(*dstats), GFP_KERNEL);
>> +	if (!dstats)
>> +		return -ENOMEM;
>> +
>>  	debugfs_create_file("dstats", 0444, rvu->rvu_dbg.npc, rvu,
>>  			    &npc_mcam_dstats_fops);
>Should the dstats allocation move to the top of npc_cn20k_debugfs_init()?
>By the time the kzalloc fails, three debugfs files (mcam_layout,
>mcam_default, vidx2idx) have already been created, and on -ENOMEM the
>function returns without unwinding them. The sole caller in
>rvu_dbg_npc_init() is:
>    if (is_cn20k(rvu->pdev))
>        npc_cn20k_debugfs_init(rvu);
>rvu_dbg_npc_init() is static void and discards the return value, so under
>memory pressure during probe the device completes probe with a partially
>populated debugfs hierarchy and no error is logged. Pre-patch the function
>unconditionally returned 0, so this is a new failure mode introduced by
>this change.
I dont see any problem with this.

>> -	xa_destroy(&npc_priv.xa_sb_used);
>[ ... ]
>> +	if (!npc_priv)
>> +		return;
>[ ... ]
>> +	kfree(npc_priv->sb);
>>  	kfree(subbank_srch_order);
>> -	bitmap_clear(npc_priv.en_map, 0, MAX_NUM_BANKS * MAX_NUM_SUB_BANKS *
>> +	bitmap_clear(npc_priv->en_map, 0, MAX_NUM_BANKS * MAX_NUM_SUB_BANKS *
>>  		     MAX_SUBBANK_DEPTH);
>> +	npc_defrag_list_clear();
>> +	kfree(npc_priv);
>> +	npc_priv = NULL;
>>  }
>Should npc_cn20k_deinit() also NULL out subbank_srch_order after
>kfree()ing it, to match the cleanup symmetry of the fail3 path in
>npc_priv_init()?
I dont see a need as there is only one AF device per system

>> -	const int num_banks;
>> +	int num_banks;
>>  	int num_subbanks;
>>  	int subbank_depth;
>>  	DECLARE_BITMAP(en_map, MAX_NUM_BANKS *
>> @@ -214,7 +214,6 @@ struct npc_priv_t {
>>  	struct list_head defrag_lh;
>>  	struct mutex lock; /* protect defrag nodes */
>>  	int pf_cnt;
>> -	bool init_done;
>>  };
>The init_done field is removed from struct npc_priv_t, but the
>kernel-doc block above the struct still carries the corresponding
>description:
>    * @init_done:		Indicates MCAM initialization is done.
>Should this @init_done line in the kernel-doc be removed alongside the
>field? scripts/kernel-doc and make htmldocs will warn with "Excess
>struct member init_done description in npc_priv_t".
ACK. Should i post v19 to address this ?