Re: [PATCH net-next 01/13] octeontx2-af: npc: cn20k: Index management

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-01-08 at 23:23:57, Simon Horman (horms@kernel.org) wrote:
> On Mon, Jan 05, 2026 at 08:02:42AM +0530, Ratheesh Kannoth wrote:
>
> > +		if (strlen(t1) < 3) {
> > +			pr_err("%s:%d Bad Token %s=%s\n",
> > +			       __func__, __LINE__, t1, t2);
> > +			goto err;
> > +		}
> > +
> > +		if (t1[0] != '[' || t1[strlen(t1) - 1] != ']') {
> > +			pr_err("%s:%d Bad Token %s=%s\n",
> > +			       __func__, __LINE__, t1, t2);
>
> Hi Ratheesh,
>
> FWIIW, I would advocate slightly more descriptive and thus unique
> error messages
ACK.

>and dropping __func__ and __LINE__ from logs,
> here and elsewhere.
ACK.

>
> The __func__, and in particular __LINE__ information will only
> tend to change as the file is up dated, and so any debugging will
> need to know the source that the kernel was compiled from.
ACK.

>
> And I'd say that given the state of debugging functionality in the kernel -
> e..g dynamic tracepoints - this is not as useful as it may seem at first.
Since these represent valid error cases, they should be logged by default.
Relying on dynamic trace points would require the customer to recompile the
kernel and retest, which could lead to significant debugging delays and
multiple rounds of communication.

> ...
>
> > +			if (contig) {
> > +				cnt = 0;
> > +				rc = npc_idx_free(rvu, mcam_idx, cnt, false);
> > +				if (rc)
> > +					return rc;
>
> Claude Code with Review Prompts [1] flags that the call to npc_idx_free()
> is a noop because count is reset to 0 before (rather than after) it is
> called.
THanks, i noticed that patchwork AI code review tool also detected this.

>
> > +	if (!fslots)
> > +		return -ENOMEM;
> > +
> > +	uslots = kcalloc(cnt, sizeof(*uslots), GFP_KERNEL);
> > +	if (!uslots)
> > +		return -ENOMEM;
> > +
> > +	for (int i = 0; i < cnt; i++, arr++) {
> > +		idx = (*arr)[0];
> > +		val = (*arr)[1];
>
> FWIIW, I think this would be slightly more clearly expressed as:
>
> 		idx = arr[i][0];
> 		val = arr[i][0];
>
> Likewise for uslots and fslots below.
ACK.
>
> > +
> > +#define MAX_SUBBANK_DEPTH 256
> > +
> > +enum npc_subbank_flag {
> > +	NPC_SUBBANK_FLAG_UNINIT,	// npc_subbank is not initialized yet.
> > +	NPC_SUBBANK_FLAG_FREE = BIT(0),	// No slot allocated
> > +	NPC_SUBBANK_FLAG_USED = BIT(1), // At least one slot allocated
> > +};
>
> I would suggest using Kernel doc formatting for the documentation
> the enum above and two structs below.
>
ACK.

> > +
>
> ...