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

[Wei Fang]

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

This is indeed a potential issue, I will add a patch to fix it.