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

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

CC netdev to avoid the same comments

> > 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.