RE: [PATCH v6 net-next 06/15] net: enetc: add support for the "Update" operation to buffer pool table

[Wei Fang <wei.fang@nxp.com>]

CC netdev to avoid the same comments

> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to
> > consider:
> > - [High] The BPT update command incorrectly sets the `BPT_UA_BPSEU`
> > flag without providing the required BPSE payload in the DMA buffer.
> > --
> >
> > commit 04028be08f5e1d61341410aae72c568ebdb38953
> > Author: Wei Fang <wei.fang@nxp.com>
> >
> > net: enetc: add support for the "Update" operation to buffer pool
> > table
> >
> > This commit adds the ntmp_bpt_update_entry helper to support updating
> > the specified entry of the buffer pool table.
> >
> > > diff --git a/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> > b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> > > --- a/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> > > +++ b/drivers/net/ethernet/freescale/enetc/ntmp_private.h
> >
> > [ ... ]
> >
> > > @@ -175,4 +175,10 @@ struct vft_req_ua {
> > >  	struct vft_cfge_data cfge;
> > >  };
> > >
> > > +/* Buffer Pool Table Request Data Buffer Format of Update action */
> > > +struct bpt_req_update {
> > > +	struct ntmp_req_by_eid rbe;
> > > +	struct bpt_cfge_data cfge;
> > > +};
> > > +
> >
> > [ ... ]
> >
> > > diff --git a/drivers/net/ethernet/freescale/enetc/ntmp.c
> > b/drivers/net/ethernet/freescale/enetc/ntmp.c
> > > --- a/drivers/net/ethernet/freescale/enetc/ntmp.c
> > > +++ b/drivers/net/ethernet/freescale/enetc/ntmp.c
> >
> > [ ... ]
> >
> > > @@ -749,5 +755,38 @@ int ntmp_vft_add_entry(struct ntmp_user *user,
> > u16 vid,
> > >  }
> > >  EXPORT_SYMBOL_GPL(ntmp_vft_add_entry);
> > >
> > > +int ntmp_bpt_update_entry(struct ntmp_user *user, u32 entry_id,
> > > +			  const struct bpt_cfge_data *cfge) {
> > > +	struct bpt_req_update *req;
> > > +	struct netc_swcbd swcbd;
> > > +	struct netc_cbdr *cbdr;
> > > +	union netc_cbd cbd;
> > > +	int err;
> > > +
> > > +	swcbd.size = sizeof(*req);
> > > +	err = ntmp_alloc_data_mem(user->dev, &swcbd, (void **)&req);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	ntmp_fill_crd_eid(&req->rbe, user->tbl.bpt_ver, 0,
> > > +			  NTMP_GEN_UA_CFGEU | BPT_UA_BPSEU, entry_id);
> >
> > Will this command fail due to a missing payload for the Buffer Pool
> > Status Entry?
> >
> > The update action is instructed to process both CFGE and BPSE data by
> > setting NTMP_GEN_UA_CFGEU | BPT_UA_BPSEU, but the allocated struct
> > bpt_req_update shown above does not contain a struct bpt_bpse_data
> > field.
> >
> > Because the provided swcbd.size is only sizeof(*req), does this cause
> > the hardware to either reject the command with a length mismatch
> > error, or unintentionally read past the end of the provided buffer?
> 
> According to the NETC reference manual, there is no BPSE_DATA in the request
> data, this data is maintained by the hardware's internal buffer. The
> BPT_UA_BPSEU flag is used to indicate that the hardware should reset the
> BPSE_DATA of the entry.
> 
> This is a false positive, please ignore it.