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

[sashiko-bot@kernel.org]

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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509102954.4116624-1-wei.fang@nxp.com?part=7