RE: [PATCH v6 net-next 07/15] net: enetc: add support for "Add" and "Delete" operations to IPFT

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
> consider:
> - [High] The IPFT Add request specifies the STSE update flag but omits the STSE
> data in its structure, causing hardware to misread the CFGE data.
> --
> 
> commit 00a023617c26cf31c0233be20200f834604aaeaa
> Author: Wei Fang <wei.fang@nxp.com>
> 
> net: enetc: add support for "Add" and "Delete" operations to IPFT
> 
> This patch adds "Add" and "Delete" operations to the ingress port filter
> table (IPFT) for the ENETC driver. These interfaces allow the driver and
> switch to classify incoming traffic and apply specific actions based on
> a precedence value.
> 
> > 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
> [ ... ]
> > +/* Ingress Port Filter Table Request Data Buffer Format of Update and
> > + * Add actions
> > + */
> > +struct ipft_req_ua {
> > +	struct ntmp_cmn_req_data crd;
> > +	union ipft_access_key ak;
> > +	struct ipft_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
> [ ... ]
> > +int ntmp_ipft_add_entry(struct ntmp_user *user,
> > +			struct ipft_entry_data *entry)
> > +{
> [ ... ]
> > +	swcbd.size = sizeof(*resp);
> > +	err = ntmp_alloc_data_mem(user->dev, &swcbd, (void **)&req);
> > +	if (err)
> > +		return err;
> > +
> > +	ntmp_fill_crd(&req->crd, user->tbl.ipft_ver, NTMP_QA_ENTRY_ID,
> > +		      NTMP_GEN_UA_CFGEU | NTMP_GEN_UA_STSEU);
> 
> Does setting the NTMP_GEN_UA_STSEU flag here cause the hardware to
> expect
> STSE data in the request buffer?
> 
> Looking at ipft_resp_query, the STSE data (__le64 match_count) sits
> strictly between KEYE and CFGE:
> 
> struct ipft_resp_query {
> 	__le32 status;
> 	__le32 entry_id;
> 	struct ipft_keye_data keye;
> 	__le64 match_count; /* STSE_DATA */
> 	struct ipft_cfge_data cfge;
> } __packed;
> 
> However, ipft_req_ua lacks this STSE data field, placing cfge immediately
> after ak.
> 
> Since the hardware parses the buffer sequentially, could this cause it to
> misinterpret the req->cfge field as STSE data, and subsequently read past
> the valid data into the zero-initialized tail to populate the actual CFGE
> data?
> 
> If the STSE update is intended, should an STSE field be added to
> struct ipft_req_ua? Alternatively, should NTMP_GEN_UA_STSEU be removed
> from the update action flags?
> 
> > +	req->ak.keye = entry->keye;
> > +	req->cfge = entry->cfge;
> 

According to the NETC reference manual, there is no STSE data field in the
request data (ipft_req_ua). The STSE data is the statistics element data, which
is maintained by the hardware's internal buffer. NTMP_GEN_UA_STSEU is used
to indicate the hardware to the hardware to reset the statistics of the entry.

This is a false positive, please ignore it.