Re: [v3,net-next,5/5] octeontx2-af: Add support for loading custom KPU profile from filesystem

[Simon Horman]

On Wed, Mar 11, 2026 at 08:45:40AM +0530, Ratheesh Kannoth wrote:
> On 2026-03-10 at 22:51:00, Simon Horman (horms@xxxxxxxxxx) wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> >
> > For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> > ---
> > octeontx2-af: Add support for loading custom KPU profile from filesystem
> >
> > This commit adds functionality to load custom KPU (Key Parse Unit)
> > profiles from the filesystem at module load time, providing an
> > alternative to flashing updated firmware on deployed devices.
> >
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> > > index cefc5d70f3e4..5928c4d3cc8b 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> >
> > [ ... ]
> >
> > > @@ -290,6 +303,7 @@ struct npc_kpu_profile {
> > >  	int action_entries;
> > >  	struct npc_kpu_profile_cam *cam;
> > >  	struct npc_kpu_profile_action *action;
> > > +	struct npc_kpu_profile_cam2 *cam2;
> > >  };
> >
> > [ ... ]
> >
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > > index a466181cf908..1efc53b63b67 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> >
> > [ ... ]
> >
> > > @@ -553,8 +553,8 @@ struct npc_kpu_profile_adapter {
> > >  	const char			*name;
> > >  	u64				version;
> > >  	const struct npc_lt_def_cfg	*lt_def;
> > > -	const struct npc_kpu_profile_action	*ikpu; /* array[pkinds] */
> > > -	const struct npc_kpu_profile	*kpu; /* array[kpus] */
> > > +	struct npc_kpu_profile_action	*ikpu; /* array[pkinds] */
> > > +	struct npc_kpu_profile	*kpu; /* array[kpus] */
> >
> > [ ... ]
> >
> > > @@ -564,6 +564,7 @@ struct npc_kpu_profile_adapter {
> > >  	bool				custom;
> > >  	size_t				pkinds;
> > >  	size_t				kpus;
> > > +	bool				from_fs;
> > >  };
> >
> > [ ... ]
> >
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > > index 352dc5f1d5b9..05aabe26f030 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> >
> > [ ... ]
> >
> > > @@ -1610,6 +1622,7 @@ u64 npc_enable_mask(int count)
> > >  static void npc_program_kpu_profile(struct rvu *rvu, int blkaddr, int kpu,
> > >  				    const struct npc_kpu_profile *profile)
> > >  {
> > > +	struct npc_kpu_profile_adapter *adapter = &rvu->kpu;
> > >  	int entry, num_entries, max_entries;
> > >  	u64 entry_mask;
> > >
> > > @@ -1621,10 +1634,15 @@ static void npc_program_kpu_profile(struct rvu *rvu, int blkaddr, int kpu,
> > >
> > >  	max_entries = rvu->hw->npc_kpu_entries;
> > >
> > > +	WARN(profile->cam_entries > max_entries,
> > > +	     "KPU%u: err: hw max entries=%u, input entries=%u\n",
> > > +	     kpu,  rvu->hw->npc_kpu_entries, profile->cam_entries);
> > > +
> > >  	/* Program CAM match entries for previous KPU extracted data */
> > >  	num_entries = min_t(int, profile->cam_entries, max_entries);
> > >  	for (entry = 0; entry < num_entries; entry++)
> > >  		npc_config_kpucam(rvu, blkaddr,
> > > +				  adapter->from_fs ? (void *)&profile->cam2[entry] :
> > >  				  &profile->cam[entry], kpu, entry);
> >
> > Can this dereference NULL when from_fs is true for KPUs not covered by
> > the filesystem firmware?
> >
> > Looking at the filesystem loading path in npc_apply_custom_kpu:
> >
> >     for (kpu = 0; kpu < fw->kpus; kpu++) {
> >         ...
> >         ret = npc_alloc_kpu_cam2_n_action(rvu, kpu, entries);
> >         ...
> >         for (entry = 0; entry < entries; entry++) {
> >             profile->kpu[kpu].cam2[entry] = cam2[entry];
> >             ...
> >         }
> >     }
> >
> > The code allocates cam2 and populates entries only for the first
> > fw->kpus KPUs found in the firmware blob.
> >
> > However, npc_program_kpu_profile is later called for all KPUs in
> > npc_parser_profile_init:
> >
> >     for (idx = 0; idx < num_kpus; idx++)
> >         npc_program_kpu_profile(rvu, blkaddr, idx, &rvu->kpu.kpu[idx]);
> >
> > Since adapter->from_fs is a global flag (not per-KPU), when idx >=
> > fw->kpus, profile->cam2 remains NULL (the default npc_kpu_profiles[]
> > static initializer doesn't include cam2), causing &profile->cam2[entry]
> > to compute an invalid pointer.
> >
> > Shouldn't there be a check to verify cam2 is non-NULL before
> > dereferencing it, or allocate cam2 for all KPUs when from_fs is true?
> 
> The filsystem binary contains entries for all KPUs. So we can ignore this
> case.

Maybe it is just me. But I don't think it is entirely satisfactory for
the kernel to rely on data supplied from the filesystem being well formed.
E.g. what if it is corrupted?

I think all the points in the AI generated review hinge on this.
So let's try and agree on the basic principles and move on from there.

...