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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aWBqq9UKWD5ewKpA@rkannoth-OptiPlex-7090>
Date: Fri, 9 Jan 2026 08:10:43 +0530
From: Ratheesh Kannoth <rkannoth@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <sgoutham@...vell.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
        <kuba@...nel.org>, <pabeni@...hat.com>, <andrew+netdev@...n.ch>,
        <sumang@...vell.com>
Subject: Re: [PATCH net-next 01/13] octeontx2-af: npc: cn20k: Index management

On 2026-01-08 at 23:23:57, Simon Horman (horms@...nel.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.

> > +
>
> ...