Re: [PATCH v4 net-next 05/14] net: enetc: add support for the "Add" operation to VLAN filter table

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: enetc: add support for the "Add" operation to VLAN filter table

This patch adds the ntmp_vft_add_entry() helper function to support adding
entries to the VLAN filter table. It allocates a DMA buffer, formats the
request, and transmits it via the command BD ring using netc_xmit_ntmp_cmd().

> 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

[ ... ]

> @@ -652,5 +655,52 @@ int ntmp_fdbt_search_port_entry(struct ntmp_user *user, int port,
> +int ntmp_vft_add_entry(struct ntmp_user *user, u16 vid,
> +		       const struct vft_cfge_data *cfge)
> +{
> +	struct ntmp_dma_buf data = {
> +		.dev = user->dev,
> +		.size = sizeof(struct vft_req_ua),
> +	};
> +	struct vft_req_ua *req;
> +	union netc_cbd cbd;
> +	u32 len;
> +	int err;
> +
> +	err = ntmp_alloc_data_mem(&data, (void **)&req);
> +	if (err)
> +		return err;
> +
> +	/* Request data */
> +	ntmp_fill_crd(&req->crd, user->tbl.vft_ver, 0,
> +		      NTMP_GEN_UA_CFGEU);
> +	req->ak.exact.vid = cpu_to_le16(vid);
> +	req->cfge = *cfge;
> +
> +	/* Request header */
> +	len = NTMP_LEN(data.size, NTMP_STATUS_RESP_LEN);
> +	ntmp_fill_request_hdr(&cbd, data.dma, len, NTMP_VFT_ID,
> +			      NTMP_CMD_ADD, NTMP_AM_EXACT_KEY);
> +
> +	err = netc_xmit_ntmp_cmd(user, &cbd);
> +	if (err)
> +		dev_err(user->dev,
> +			"Failed to add %s entry, vid: %u, err: %pe\n",
> +			ntmp_table_name(NTMP_VFT_ID), vid, ERR_PTR(err));
> +
> +	ntmp_free_data_mem(&data);

Does this code safely handle hardware timeouts?

If netc_xmit_ntmp_cmd() times out and returns an error, the pending
transaction is not explicitly aborted before ntmp_free_data_mem()
unconditionally frees the DMA buffer.

Could the hardware eventually process the pending command and perform a
DMA write of the response to the physical address of the freed buffer?

This might lead to silent memory corruption if that memory has already
been reallocated to another subsystem. Would it be safer to explicitly
abort the pending DMA transaction, reset the device on timeout, or
intentionally delay freeing the buffer until it is proven safe?

> +
> +	return err;
> +}